Bug 124552 – REGRESSION (r159395): Error compiling for ARMv7

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

[Michael Saboff]

In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:29:
In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/bytecode/CodeBlock.h:37:
In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/bytecode/CallLinkInfo.h:29:
In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/jit/ClosureCallStubRoutine.h:33:
In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/bytecode/CodeOrigin.h:32:
In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/bytecode/ValueRecovery.h:30:
After http://trac.webkit.org/changeset/159395, compiling for ARMv7 gives this error.

In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/jit/GPRInfo.h:29:
In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/assembler/MacroAssembler.h:34:
/Volumes/Data/src/webkit.work/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1382:9: error: no matching member function for
      call to 'load8'
        load8(left, addressTempRegister);
        ^~~~~
/Volumes/Data/src/webkit.work/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:536:10: note: candidate function not viable: no
      known conversion from 'JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::AbsoluteAddress' to
      'JSC::MacroAssemblerARMv7::ArmAddress' for 1st argument
    void load8(ArmAddress address, RegisterID dest)
         ^
/Volumes/Data/src/webkit.work/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:635:10: note: candidate function not viable: no
      known conversion from 'JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::AbsoluteAddress' to
      'JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::ImplicitAddress' for 1st argument
    void load8(ImplicitAddress address, RegisterID dest)
         ^
/Volumes/Data/src/webkit.work/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:645:10: note: candidate function not viable: no
      known conversion from 'JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::AbsoluteAddress' to
      'JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::BaseIndex' for 1st argument
    void load8(BaseIndex address, RegisterID dest)
         ^
1 error generated.

** BUILD FAILED **

[Michael Saboff]

Created attachment 217257 [details]
Patch

[Geoffrey Garen]

Comment on attachment 217257 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        to follow materialize and use address similar to other ARMv7 branchXX() functions.

I don't think you meant to say "follow" here.

> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1382
> +        // use addressTempRegister incase the branchTest8 we call uses dataTempRegister. :-/

"Use" should be capitalized. "in case" is two words. ":-/" seems to imply that something is still wrong with this code. Is there something wrong with this code? This function does not call branchTest8. Did you mean branch32? When would branch32 use dataTempRegister, and why would that be bad?

[Michael Saboff]

(In reply to comment #2)
> (From update of attachment 217257 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217257&action=review
> 
> > Source/JavaScriptCore/ChangeLog:9
> > +        to follow materialize and use address similar to other ARMv7 branchXX() functions.
> 
> I don't think you meant to say "follow" here.

Eliminated "follow"
 
> > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1382
> > +        // use addressTempRegister incase the branchTest8 we call uses dataTempRegister. :-/
> 
> "Use" should be capitalized. "in case" is two words. ":-/" seems to imply that something is still wrong with this code. Is there something wrong with this code? This function does not call branchTest8. Did you mean branch32? When would branch32 use dataTempRegister, and why would that be bad?

The "use", "incase" and ":-/" are present in all the other similar comments.  I copied from one of them.

Copy/paste error.  Changed it to:
        // Use addressTempRegister in case the branch32 we call uses dataTempRegister.

branch32() calls compare32() which uses dataTempRegister when the TrustedImm32 needs to be materialized in a register because we can't encode it in a compare immediate instruction.  The comment is there because we would typically load into dataTempRegister here, but can't because of the possible use in compare32().

[Geoffrey Garen]

Comment on attachment 217257 [details]
Patch

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

>>> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1382
>>> +        // use addressTempRegister incase the branchTest8 we call uses dataTempRegister. :-/
>> 
>> "Use" should be capitalized. "in case" is two words. ":-/" seems to imply that something is still wrong with this code. Is there something wrong with this code? This function does not call branchTest8. Did you mean branch32? When would branch32 use dataTempRegister, and why would that be bad?
> 
> The "use", "incase" and ":-/" are present in all the other similar comments.  I copied from one of them.
> 
> Copy/paste error.  Changed it to:
>         // Use addressTempRegister in case the branch32 we call uses dataTempRegister.
> 
> branch32() calls compare32() which uses dataTempRegister when the TrustedImm32 needs to be materialized in a register because we can't encode it in a compare immediate instruction.  The comment is there because we would typically load into dataTempRegister here, but can't because of the possible use in compare32().

Sounds like this comment should also say something about "addressTempRegister instead of dataTempRegister", then.

[Michael Saboff]

(In reply to comment #4)
> (From update of attachment 217257 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=217257&action=review
> 
> >>> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1382
> >>> +        // use addressTempRegister incase the branchTest8 we call uses dataTempRegister. :-/
> >> 
> >> "Use" should be capitalized. "in case" is two words. ":-/" seems to imply that something is still wrong with this code. Is there something wrong with this code? This function does not call branchTest8. Did you mean branch32? When would branch32 use dataTempRegister, and why would that be bad?
> > 
> > The "use", "incase" and ":-/" are present in all the other similar comments.  I copied from one of them.
> > 
> > Copy/paste error.  Changed it to:
> >         // Use addressTempRegister in case the branch32 we call uses dataTempRegister.
> > 
> > branch32() calls compare32() which uses dataTempRegister when the TrustedImm32 needs to be materialized in a register because we can't encode it in a compare immediate instruction.  The comment is there because we would typically load into dataTempRegister here, but can't because of the possible use in compare32().
> 
> Sounds like this comment should also say something about "addressTempRegister instead of dataTempRegister", then.

Does the following work?
    // Use addressTempRegister instead of dataTempRegister in case the branch32 or its callees use dataTempRegister

[Geoffrey Garen]

> Does the following work?
>     // Use addressTempRegister instead of dataTempRegister in case the branch32 or its callees use dataTempRegister

I think you can be even clearer and say, "Use addressTempRegister instead of dataTempRegister, since branch32 uses dataTempRegister.".

[Michael Saboff]

(In reply to comment #6)
> > Does the following work?
> >     // Use addressTempRegister instead of dataTempRegister in case the branch32 or its callees use dataTempRegister
> 
> I think you can be even clearer and say, "Use addressTempRegister instead of dataTempRegister, since branch32 uses dataTempRegister.".

Sold.  I updated the comment to what you suggest.

[Geoffrey Garen]

Comment on attachment 217257 [details]
Patch

Can you post the updated patch?

[Michael Saboff]

Created attachment 217312 [details]
Updated patch

[Geoffrey Garen]

Comment on attachment 217312 [details]
Updated patch

r=me

[Michael Saboff]

Committed r159521: <http://trac.webkit.org/changeset/159521>