540351 - nanojit: refactor hint() and registerAlloc()

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect)

Description

[Nicholas Nethercote [inactive]]

Attachment: patch

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.

Comment 1

[Rick Reitmaier]

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___' .

Comment 2

[Nicholas Nethercote [inactive]]

> 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 3

[Rick Reitmaier]

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

Sorry somehow missed your response; marking r+

Comment 4

[Nicholas Nethercote [inactive]]

Landed with a follow-up ARM bustage fix:

http://hg.mozilla.org/projects/nanojit-central/rev/fa4f34a51239
http://hg.mozilla.org/projects/nanojit-central/rev/4a6d8a20be25

Comment 5

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/1ef96de8df55
http://hg.mozilla.org/tracemonkey/rev/497c624a971d

Comment 6

[Steven Johnson]

http://hg.mozilla.org/tamarin-redux/rev/c0ac39387b8e

Comment 7

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1ef96de8df55