If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nanojit: refactor hint() and registerAlloc()

RESOLVED FIXED

Status

Core Graveyard
Nanojit
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

8 years ago
Created attachment 422135 [details] [diff] [review]
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.
Attachment #422135 - Flags: review?(rreitmai)

Comment 1

8 years ago
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___' .
(Assignee)

Comment 2

8 years ago
> 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

8 years ago
Comment on attachment 422135 [details] [diff] [review]
patch

Sorry somehow missed your response; marking r+
Attachment #422135 - Flags: review?(rreitmai) → review+
(Assignee)

Comment 4

8 years ago
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
Whiteboard: fixed-in-nanojit
(Assignee)

Comment 5

8 years ago
http://hg.mozilla.org/tracemonkey/rev/1ef96de8df55
http://hg.mozilla.org/tracemonkey/rev/497c624a971d
Whiteboard: fixed-in-nanojit → fixed-in-nanojit, fixed-in-tracemonkey

Updated

8 years ago
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin

Comment 6

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

Comment 7

8 years ago
http://hg.mozilla.org/mozilla-central/rev/1ef96de8df55
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Component: Nanojit → Nanojit
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.