[Webkit-unassigned] [Bug 67176] JavaScriptCore does not have tiered compilation

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=67176





--- Comment #38 from Filip Pizlo <fpizlo at apple.com>  2011-09-05 21:09:18 PST ---
(In reply to comment #37)
> > branchAdd32 should use scratchRegister not r11:
> >     I renamed it to branchAdd32Absolute and moved it to the subclasses.
> 
> You forgot to remove branchAdd32 from MacroAssemblerX86Common.h!
> Also branchAdd32Absolute should be named branchAdd32, other AbsoluteAddress'ed methods aren't named in this fashion.

If I rename it to branchAdd32() then I get overload hiding madness.  If I just change the name to branchAdd32() but leave it in the subclasses, I get the following errors for uses of the other branchAdd32() overloads:

/Volumes/Data/pizlo/quartary/OpenSource/Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:686:102:{686:34-686:51}{686:102-686:114}: error: too many arguments to function call, expected 3, have 4 [2]
                 speculationCheck(m_jit.branchAdd32(MacroAssembler::Overflow, op2.gpr(), Imm32(imm1), result.gpr()));
                                  ~~~~~~~~~~~~~~~~~                                                   ^~~~~~~~~~~~

Notice that here we're trying to call MacroAssemblerX86Common::branchAdd32(ResultCondition, RegisterID, TrustedImm32, RegisterID).  But the compiler assumes that MacroAssemblerX86_64::branchAdd32(ResultCondition, RegisterID, AbsoluteAddress) is hiding that method, along with the other overloads.  This is because C++ apparently dictates that name resolution is done before overload resolution, so as soon as MacroAssemblerX86_64::branchAdd32 is found, none of the overloads of MacroAssemblerX86Common::branchAdd32are visible.

> I'm fine with the other changes / review push-back, r+ with MacroAssemblerX86Common::branchAdd32 removed, branchAdd32Absolute renamed to branchAdd32.

OK, what would you prefer? :-)  Once again, the options are:

1) Do as I did before.  Move branchAdd32Absolute to MacroAssemblerX86Common, but then I have to use r11 directly instead of via the scratchRegister alias.

2) As with (1) but move the scratchRegister declaration to MacroAssemblerX86Common.

3) Move all branchAdd32 overloads to the MacroAssemblerX86_64 and MacroAssemblerX86.  I don't like this option because that's a lot of code duplication.

4) Rename this particular overload of branchAdd32 to branchAdd32Absolute and put it in MacroAssemblerX86_64, which is what I had done for this patch.

I tend to think that (4) is the lesser of the various evils, though I'm happy with any of the other approaches.  I'm not trying to advocate styles here ... I totally agree that branchAdd32Absolute "should" be called branchAdd32, and that we "should" refer to scratchRegister instead of directly to r11, but we're pushing up against bizarre corner cases of C++ here.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.