Closed Bug 540351 Opened 10 years ago Closed 10 years ago

nanojit: refactor hint() and registerAlloc()

Categories

(Core Graveyard :: Nanojit, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: njn, Assigned: njn)

Details

(Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin)

Attachments

(1 file)

Attached patch patchSplinter Review
The decision about which register set to allocate from in any given instance
is currently shared between hint() and registerAlloc().  And the variable
naming ('allow' vs. 'prefer') is confusing.  This makes it hard to work out
exactly which register set the register is allocated from in any given case.
(A concrete example:  for the findVictim() call in registerAlloc() try to
work out which register set is used.)

This patch:

- Simplifies hint() so it just returns a preferred register set, without
  having to take the allowed set into account.  (This simplification is
  particularly nice because there is one hint() per back-end.)

- Moves all the complexity of the allocation decision (ie. which registers
  are allowed, preferred, free and saved) into registerAlloc(), and applies
  a naming convention to register sets that make it obvious what is
  happening.

- In the ARM and PPC backends, makes sure the param/argRegs preference is
  only applied if it's the right paramKind.

The patch doesn't change which registers are chosen (except for the ARM/PPC
fix above), but it makes it clearer where there are different choices to be
made -- eg. in the findVictim() call we could choose to use setAP__ instead
of setA___ if it's non-empty.
Attachment #422135 - Flags: review?(rreitmai)
I like the idea and the way its implemented but I'm having a hard time convincing myself that the code picks the sames registers.  

For example, on PPC say hint() returns R3 and it is in use.  The old code would call findVictim(R3) whereas the new code would use 'setA___' .
> I like the idea and the way its implemented but I'm having a hard time
> convincing myself that the code picks the sames registers.  
>
> For example, on PPC say hint() returns R3 and it is in use.  The old code would
> call findVictim(R3) whereas the new code would use 'setA___' .

With the old code, hint() would never return 'rmask(R3)' if R3 is in use, because it checks that the hinted regset overlaps with 'allow' and '_allocator.free'.   So it would just return 'allow' unchanged.  Which means that the 'prefer' passed into registerAlloc() is equal to 'allow' (which is really confusing).  And in the new code, 'setA___' == 'allow'.  QED!

And this is exactly why it needs refactoring :)

I did the change in numerous small steps, which gives me confidence I didn't change the behaviour.  Furthermore, I have a script that I use to compare the generated native code produced by two different versions of TM, I used it on several of the SunSpider programs to confirm that this new version has the same behaviour :)
Comment on attachment 422135 [details] [diff] [review]
patch

Sorry somehow missed your response; marking r+
Attachment #422135 - Flags: review?(rreitmai) → review+
http://hg.mozilla.org/tracemonkey/rev/1ef96de8df55
http://hg.mozilla.org/tracemonkey/rev/497c624a971d
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
http://hg.mozilla.org/mozilla-central/rev/1ef96de8df55
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.