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 **
Created attachment 217257 [details] Patch
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?
(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().
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.
(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
> 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.".
(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.
Comment on attachment 217257 [details] Patch Can you post the updated patch?
Created attachment 217312 [details] Updated patch
Comment on attachment 217312 [details] Updated patch r=me
Committed r159521: <http://trac.webkit.org/changeset/159521>