NJ merge: LIR_div/LIR_mod on X64

VERIFIED DUPLICATE of bug 518488

Status

Tamarin
Baseline JIT (CodegenLIR)
VERIFIED DUPLICATE of bug 518488
9 years ago
9 years ago

People

(Reporter: njn, Assigned: Edwin Smith)

Tracking

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
Created attachment 402757 [details] [diff] [review]
patch

This was TM bug 516898.
Attachment #402757 - Flags: review?(edwsmith)
(Assignee)

Comment 1

9 years ago
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)
Attachment #402757 - Flags: review?(edwsmith) → review+
(Reporter)

Comment 2

9 years ago
(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

9 years ago
I believe this a dup 518488
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 518488

Updated

9 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.