Bug 144680

Although the adrp instruction is never used from ARM64Assembler.h, it is possible to add a simple workaround for this. I will upload a patch soon.

Created attachment 252464 [details] Patch

Comment on attachment 252464 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=252464&action=review > Source/JavaScriptCore/assembler/ARM64Assembler.h:3668 > + nop(); Why this third nop? I thought that the bug was an issue only for a dependent instruction at the page boundary. The check looks for the page boundary and inserts two nop's that should take care of the problem, making the last nop unnecessary. > Source/JavaScriptCore/assembler/ARM64Assembler.h:3670 > + ALWAYS_INLINE void nopCortexA53Fix843419() > + { > +#if CPU(ARM64_CORTEXA53) > + if (UNLIKELY((m_buffer.codeSize() & 0xFF8) == 0xFF8)) { > + nop(); > + nop(); > + } > + nop(); > +#endif > + } This nop padding needs to happen at link time. I don't think we have any guarantees that the page offset at assemble time will be the same after we link. Even if we did, there is some code compaction that can happen at link time, primarily due to branch type specialization.

There are two cases: 1) If the ADRP is near to the page boundary (last two instruction) you will need more than two instructions between the ADRP and the load or store. 2) If the ADRP is followed by an instruction which writes the result register you will need an addition register. So, there is no unnecessary nops. > This nop padding needs to happen at link time. I don't think we have any guarantees that the page offset at assemble time will be the same after we link. > Even if we did, there is some code compaction that can happen at link time, primarily due to branch type specialization. Sad to hear that. It was a long time when I last touched the JSC. I am sure that the appropriate fix will be to check all the conditions which cause the issue, but it surely slow down the code generation - because the load and store instructions are very frequent. If we do these checks as a later pass after code generation (link time), the effect to the performance will be the same. I think the most easiest way to fix this - which might not influence the performance much - is to generate three nops after ADRP. How does it sound?

Created attachment 252858 [details] Patch

(In reply to comment #4) > There are two cases: > 1) If the ADRP is near to the page boundary (last two instruction) you will > need more than two instructions between the ADRP and the load or store. > 2) If the ADRP is followed by an instruction which writes the result > register you will need an addition register. > > So, there is no unnecessary nops. > > > This nop padding needs to happen at link time. I don't think we have any guarantees that the page offset at assemble time will be the same after we link. > > Even if we did, there is some code compaction that can happen at link time, primarily due to branch type specialization. > > Sad to hear that. It was a long time when I last touched the JSC. > > I am sure that the appropriate fix will be to check all the conditions which > cause the issue, but it surely slow down the code generation - because the > load and store instructions are very frequent. If we do these checks as a > later pass after code generation (link time), the effect to the performance > will be the same. > > I think the most easiest way to fix this - which might not influence the > performance much - is to generate three nops after ADRP. > > How does it sound? Sounds fine. As a heads up, there has been some discussion to change the current movz/movk/movk absolute address materialization to adrp/add.

Comment on attachment 252858 [details] Patch r=me

Comment on attachment 252858 [details] Patch Clearing flags on attachment: 252858 Committed r184170: <http://trac.webkit.org/changeset/184170>

All reviewed patches have been landed. Closing bug.