diff mbox series

Another fix for the mcore simulator

Message ID 20cefa11-9565-47f5-b1bc-bd90e8c428fc@gmail.com
State New
Headers
Series Another fix for the mcore simulator |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 fail Patch failed to apply
linaro-tcwg-bot/tcwg_gdb_build--master-arm fail Patch failed to apply

Commit Message

Jeff Law Nov. 26, 2023, 6:47 p.m. UTC 
  Here we go again.  About a month ago a change to GCC's generic 
optimizers triggered a "regression" for mcore-elf.  In simplest terms 
the change eliminated a zero extension and after a quick analysis it was 
pretty clear the zero extension should have made no difference in the 
visible behavior of the code.

Naturally I suspected another problem with the mcore-elf simulator when 
run on a 64 bit host.  I wasn't disappointed.  This time it was the 
logical right shift instruction that was clearly wrong.

This fixes around 500 more tests in the GCC testsuite and knocks about 
40 minutes off its runtime -- it's now only about 2-3X slower than other 
comparable targets.  Which is a massive improvement over where it was 
before I fixed the sext instructions.

No doubt there's more 64 bit host issues in the mcore simulator.  I 
can't justify chasing them all down preemptively.  But I'll obviously 
continue to fault in fixes if/when the GCC testsuite shows a regression.

OK for the trunk?

jeff

Comments

Tom Tromey Nov. 27, 2023, 8:06 p.m. UTC | #1 
>>>>> "Jeff" == Jeff Law <jeffreyalaw@gmail.com> writes:

Jeff> No doubt there's more 64 bit host issues in the mcore simulator.  I
Jeff> can't justify chasing them all down preemptively.  But I'll obviously
Jeff> continue to fault in fixes if/when the GCC testsuite shows a
Jeff> regression.

Jeff> OK for the trunk?

I don't have any issue with the patch, but I confess I don't understand
what the bug was or how the patch works.

Jeff>  	    unsigned imm = IMM5;
Jeff> -	    unsigned long tmp = gr[RD];
Jeff> +	    uint32_t tmp = gr[RD];

IIUC gr is a uint32_t so this patch seems like it just removes a zero
extension and then a narrowing cast.

Anyway, if it helps, IMO that's enough.
Approved-By: Tom Tromey <tom@tromey.com>

Tom
Jeff Law Nov. 28, 2023, 5:46 a.m. UTC | #2 
On 11/27/23 13:06, Tom Tromey wrote:
>>>>>> "Jeff" == Jeff Law <jeffreyalaw@gmail.com> writes:
> 
> Jeff> No doubt there's more 64 bit host issues in the mcore simulator.  I
> Jeff> can't justify chasing them all down preemptively.  But I'll obviously
> Jeff> continue to fault in fixes if/when the GCC testsuite shows a
> Jeff> regression.
> 
> Jeff> OK for the trunk?
> 
> I don't have any issue with the patch, but I confess I don't understand
> what the bug was or how the patch works.
> 
> Jeff>  	    unsigned imm = IMM5;
> Jeff> -	    unsigned long tmp = gr[RD];
> Jeff> +	    uint32_t tmp = gr[RD];
> 
> IIUC gr is a uint32_t so this patch seems like it just removes a zero
> extension and then a narrowing cast.
"gr" is actually a "int32_t", so it's signed.   But even so, if it's 
stuffed into a "unsigned long" it's going to be zero extended.  So I 
certainly see your point.

Given there's some confusion here, I'll revert it out of my tester which 
should result in seeing some failures return and I can dive into the 
simulator again to extract the relevant state in the lsri/lsr instructions.

jeff
Tom Tromey Nov. 28, 2023, 3:09 p.m. UTC | #3 
>> IIUC gr is a uint32_t so this patch seems like it just removes a
>> zero
>> extension and then a narrowing cast.

Jeff> "gr" is actually a "int32_t", so it's signed.

Aha, say no more.  Thank you.

Jeff> Given there's some confusion here, I'll revert it out of my tester
Jeff> which should result in seeing some failures return and I can dive into
Jeff> the simulator again to extract the relevant state in the lsri/lsr
Jeff> instructions.

FWIW I think your patch is fine as-is.

Tom
Jeff Law Dec. 1, 2023, 2:45 a.m. UTC | #4 
On 11/28/23 08:09, Tom Tromey wrote:
>>> IIUC gr is a uint32_t so this patch seems like it just removes a
>>> zero
>>> extension and then a narrowing cast.
> 
> Jeff> "gr" is actually a "int32_t", so it's signed.
> 
> Aha, say no more.  Thank you.
So just to close the loop here.  As I noted "gr" is an int32_t.   So in 
the old code when we load that into an unsigned long, we get a sign bit 
extension:

Breakpoint 3, step_once (cpu=0x555555674860, sd=<optimized out>) at 
/home/jlaw/test/binutils-gdb/sim/mcore/interp.c:1062
1062                unsigned imm = IMM5;
(gdb) p/x ((struct mcore_sim_cpu *)cpu.arch_data).active_gregs[5]
$40 = 0x9abcdef0
(gdb) n
1063                unsigned long tmp = gr[RD];
(gdb)
1064                if (imm == 0)
(gdb) p/x tmp
$41 = 0xffffffff9abcdef0


And when we shift that value right, those 1's in bits 32..64 get shifted 
down into the result.


There's actually two instances of this exact same problem.  One for lsr 
and the other for lsrc,lsri.  So if there's no objection I'd like to fix 
both at the same time.

Cheers,
Jeff
Tom Tromey Dec. 1, 2023, 3:56 a.m. UTC | #5 
>>>>> "Jeff" == Jeff Law <jeffreyalaw@gmail.com> writes:

Jeff> There's actually two instances of this exact same problem.  One for
Jeff> lsr and the other for lsrc,lsri.  So if there's no objection I'd like
Jeff> to fix both at the same time.

Totally fine, thank you.

Tom
Jeff Law Dec. 1, 2023, 2:27 p.m. UTC | #6 
On 11/30/23 20:56, Tom Tromey wrote:
>>>>>> "Jeff" == Jeff Law <jeffreyalaw@gmail.com> writes:
> 
> Jeff> There's actually two instances of this exact same problem.  One for
> Jeff> lsr and the other for lsrc,lsri.  So if there's no objection I'd like
> Jeff> to fix both at the same time.
> 
> Totally fine, thank you.
Done.  Next up, speed up the H8 simulator by 2X or more.  Linear 
searching large arrays is bad bad bad :(

jeff
diff mbox series

Patch

diff --git a/sim/mcore/.interp.c.swp b/sim/mcore/.interp.c.swp
new file mode 100644
index 00000000000..a11d2df9077
Binary files /dev/null and b/sim/mcore/.interp.c.swp differ
diff --git a/sim/mcore/interp.c b/sim/mcore/interp.c
index 48d9ff8645a..0bbb2c399b2 100644
--- a/sim/mcore/interp.c
+++ b/sim/mcore/interp.c
@@ -1060,7 +1060,7 @@  step_once (SIM_DESC sd, SIM_CPU *cpu)
 	case 0x3E: case 0x3F:				/* lsrc, lsri */
 	  {
 	    unsigned imm = IMM5;
-	    unsigned long tmp = gr[RD];
+	    uint32_t tmp = gr[RD];
 	    if (imm == 0)
 	      {
 		NEW_C (tmp);
diff --git a/sim/testsuite/mcore/lsri.s b/sim/testsuite/mcore/lsri.s
new file mode 100644
index 00000000000..fe15213d6b0
--- /dev/null
+++ b/sim/testsuite/mcore/lsri.s
@@ -0,0 +1,21 @@ 
+# check that sext.b/sext.h work correctly
+# mach: mcore
+
+.include "testutils.inc"
+
+	start
+	# Construct -1
+	bmaski	r2, 32
+
+	# logical shift right by 24
+	lsri	r2, 24
+
+	# Construct 255
+	bmaski	r1, 8
+
+	# Compare them, they should be equal
+	cmpne	r2,r1
+	jbt	.L1
+	pass
+.L1:
+	fail