Bug 133952 – fix css jit register usage on armv7

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alex Christensen]

I did a few things wrong with registers on armv7 in the css jit.  This not only passes the tests, but it doesn't crash when running https://www.webkit.org/blog-files/css-jit-introduction/html5-single-page-microbenchmark.html

Also, I'm not sure that my register allocation in modulo for arm64 and armv7 is counted correctly.  Does this need to be changed somewhere, too?

[Alex Christensen]

Created attachment 233180 [details]
Patch

[Benjamin Poulain]

Comment on attachment 233180 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=233180&action=review

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1423
> -        load32(left, dataTempRegister);
> -        return branch32(cond, dataTempRegister, right);
> +        // use addressTempRegister incase the branch32 we call uses dataTempRegister. :-/
> +        load32(left, addressTempRegister);
> +        return branch32(cond, addressTempRegister, right);

I don't get this. We know exactly which branch32() we want to use (Jump branch32(RelationalCondition cond, RegisterID left, RegisterID right)). What am I missing?

> Source/WebCore/cssjit/SelectorCompiler.cpp:1096
> +    // r6 is tempRegister in RegisterAllocator.h and addressTempRegister in MacroAssemblerARMv7.h and must be preserved by the callee.
> +    prologueRegisters.append(JSC::ARMRegisters::r6);

I did not think of that. One more reason I hate the registers implicitly used by the MacroAssembler.

[Alex Christensen]

(In reply to comment #2)
> (From update of attachment 233180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233180&action=review
> 
> > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1423
> > -        load32(left, dataTempRegister);
> > -        return branch32(cond, dataTempRegister, right);
> > +        // use addressTempRegister incase the branch32 we call uses dataTempRegister. :-/
> > +        load32(left, addressTempRegister);
> > +        return branch32(cond, addressTempRegister, right);
> 
> I don't get this. We know exactly which branch32() we want to use (Jump branch32(RelationalCondition cond, RegisterID left, RegisterID right)). What am I missing?
I think you're right.  I'll test it in the morning without this change.  For some reason I though it was necessary, but now I'm not sure.

[Alex Christensen]

I confirmed that branchPtr is used and works without this change, so I'm committing the non-JavaScriptCore part of this patch.

[Alex Christensen]

http://trac.webkit.org/changeset/170059