518759 - NJ merge: LIR_div/LIR_mod on X64

Status: VERIFIED DUPLICATE of bug 518488

(Tamarin Graveyard :: Baseline JIT (CodegenLIR), defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

This was TM bug 516898.

Comment 1

[Edwin Smith]

Comment on attachment 402757 [details] [diff] [review]
patch

NativeX64.cpp:444
* ultra fine point:  GpRegs ^ (rmask(RAX)|rmask(RDX)) will be wrong if RAX and RDX aren't in GpRegs.  normally they are but a useful way to stress test the register allocator is to make GpRegs be the minimal set.   i admit the minimal set probably has to include RAX|RDX... but this just caught my attention.  GpRegs & ~(rmask(RAX)|rmask(RDX)) would be an iota safer.  perhaps a static assert when we set up GpRegs, to make sure at least a minimal set of registers are included?  

(probably RAX,RDX,RCX, since those are all special in one way or another)

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> (From update of attachment 402757 [details] [diff] [review])
> NativeX64.cpp:444
> * ultra fine point:  GpRegs ^ (rmask(RAX)|rmask(RDX)) will be wrong if RAX and
> RDX aren't in GpRegs.  normally they are but a useful way to stress test the
> register allocator is to make GpRegs be the minimal set.   i admit the minimal
> set probably has to include RAX|RDX... but this just caught my attention. 
> GpRegs & ~(rmask(RAX)|rmask(RDX)) would be an iota safer.  perhaps a static
> assert when we set up GpRegs, to make sure at least a minimal set of registers
> are included?  
> 
> (probably RAX,RDX,RCX, since those are all special in one way or another)

It's true about the XOR, and '~' is used everywhere else for masking so it would be better for consistency.  But I know that EAX|EDX|ECX is the minimal set for x86, so I imagine RAX|RDX|RCX is the minimal set for X64.

A static assert would be good.  I've filed a bug about making the stress testing easier:  bug 518928.

Comment 3

[Rick Reitmaier]

I believe this a dup 518488