Final, small patch to get basic/QuadFPCmp finally passing tests: ``` [sstangl@localhost src]$ python jit-test/jit_test.py --tbpl dbgarm64/js/src/js basic/FPQuadCmp [5|0|0|0] 100% ======================================================>| 3.7s PASSED ALL ``` I'll count the number of remaining failures overnight -- it's probably still very high, but this should knock out most of the simple failures related to `--ion-eager --ion-offthread-compilation=off`. After this it's just a matter of picking a test from the current failure list and getting it to pass, again and again until it's done.

Comment on attachment 9015714 [details] [diff] [review] 0001-Get-basic-FPQuadCmp-passing-tests.-r.patch Review of attachment 9015714 [details] [diff] [review]: ----------------------------------------------------------------- A few questions before final rubberstamp, patch looks good but the comments make me doubt. ::: js/src/jit/arm64/Assembler-arm64.cpp @@ +369,2 @@ > void > PatchJump(CodeLocationJump& jump_, CodeLocationLabel label) Could the variable named just be named `jump`? @@ +369,4 @@ > void > PatchJump(CodeLocationJump& jump_, CodeLocationLabel label) > { > + // ARM64 is JS_SMALL_BRANCH. Can you statically assert this somehow? Also, doesn't it mean we need to check that the jumped-to location is in range of the jump? @@ +369,5 @@ > void > PatchJump(CodeLocationJump& jump_, CodeLocationLabel label) > { > + // ARM64 is JS_SMALL_BRANCH. > + // The jump always refers to an address stored in a jump table. Is that true? I see the function `jumpWithPatch` is used in an Ion IC: https://searchfox.org/mozilla-central/source/js/src/jit/IonCacheIRCompiler.cpp#2324 ::: js/src/jit/arm64/CodeGenerator-arm64.cpp @@ +830,4 @@ > ValueOperand input = ToValue(unbox, LUnbox::Input); > ScratchTagScope scratch(masm, input); > masm.splitTagForTest(input, scratch); > + masm.branch32(Assembler::Condition::Equal, scratch, Imm32(tag), &ok); Good catch.

Comment on attachment 9015714 [details] [diff] [review] 0001-Get-basic-FPQuadCmp-passing-tests.-r.patch Clearing review flag until review questions are answered.

This patch is a partial implementation of the patch that bbouvier reviewed. I took out the sketchy stuff. I wasn't able to find a testcase that requires the more complicated logic, and I don't really want to commit code that I can't test, so for the moment I've left that as a MOZ_CRASH(). It will pop up again later in a testable environment, and then we can fix the patching logic for real. This patch is *very* important to get landed ASAP, because Ion ICs are the sole consumer of PatchJump(). Therefore, before this patch is landed, basically every test fails in ICs, and after this patch is landed, tests fail in more useful locations. nbp, when working locally, before this is landed, you may want to apply this on your local branch.

Comment on attachment 9031297 [details] [diff] [review] 0001-Bug-1497698-Partial-Implementation-of-PatchJump-.-r-.patch Review of attachment 9031297 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/arm64/Assembler-arm64.cpp @@ +305,5 @@ > + MOZ_ASSERT(branch->IsUncondB()); > + > + // The branch must be able to reach the label. > + ptrdiff_t relativeByteOffset = next.getOffset() - branchOffset; > + MOZ_ASSERT(branch->IsTargetReachable(branch + relativeByteOffset)); nit: MOZ_RELEASE_ASSERT.

Thanks! SetImmPCOffsetTarget() already has that assertion internally -- I just added a redundant MOZ_ASSERT() to get it to fail in a location that was a little easier for debugging.

Pushed by ccoroiu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bcd3b9fa9eac Partial Implementation of PatchJump(). r=nbp