Closed Bug 540351 Opened 10 years ago Closed 10 years ago
nanojit: refactor hint() and register
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+
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 → fixed-in-nanojit, fixed-in-tracemonkey
Whiteboard: fixed-in-nanojit, fixed-in-tracemonkey → fixed-in-nanojit, fixed-in-tracemonkey, fixed-in-tamarin
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.