1987592 - riscv64: CompareExchange fails to sign-extend int32

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[Rong Bao [:csmantle]]

Attachment: poc.js

Running the attached script triggers a bailout loop assertion:

> ../gecko-dev/obj-x86_64-pc-windows-msvc/dist/bin/js.exe --no-threads --ion-check-range-analysis --ion-extra-checks --baseline-eager --ion-eager --fast-warmup --fuzzing-safe ./poc.js
[8464] Assertion failure: cx_->hadResourceExhaustion(), at D:/Workspace/gecko-dev/js/src/jit/WarpOracle.cpp:212
#01: js::jit::WarpOracle::createSnapshot (D:\Workspace\gecko-dev\js\src\jit\WarpOracle.cpp:211)
#02: js::jit::Compile (D:\Workspace\gecko-dev\js\src\jit\Ion.cpp:2014)
#03: IonCompileScriptForBaseline (D:\Workspace\gecko-dev\js\src\jit\Ion.cpp:2267)
#04: js::jit::IonCompileScriptForBaselineOSR (D:\Workspace\gecko-dev\js\src\jit\Ion.cpp:2379)
#05: js::jit::Simulator::SoftwareInterrupt (D:\Workspace\gecko-dev\js\src\jit\riscv64\Simulator-riscv64.cpp:1628)
#06: js::jit::Simulator::InstructionDecode (D:\Workspace\gecko-dev\js\src\jit\riscv64\Simulator-riscv64.cpp:1880)
#07: js::jit::Simulator::callInternal (D:\Workspace\gecko-dev\js\src\jit\riscv64\Simulator-riscv64.cpp:4059)
#08: js::jit::Simulator::call (D:\Workspace\gecko-dev\js\src\jit\riscv64\Simulator-riscv64.cpp:4127)
#09: js::jit::MaybeEnterJit (D:\Workspace\gecko-dev\js\src\jit\Jit.cpp:260)
[...]
>

Looking at JitSpew we can see the JIT is repeatedly bailing out of an AddI instruction:

[...]
[IonBailouts] Took bailout! Snapshot offset: 373
[BaselineBailouts] Bailing to baseline ./poc.js:1:1 (IonScript=1d14a423f00) (FrameType=3)
[BaselineBailouts]   Reading from snapshot offset 373 size 389
[IonSnapshots] Creating snapshot reader
[IonSnapshots] Read snapshot header with bailout kind 1
[IonSnapshots] Read recover header with instructionCount 1
[IonSnapshots] Read RResumePoint (pc offset 179, mode ResumeAt, nslots 5)
[IonBailouts]  bailing from bytecode: JumpTarget, MIR: Add [76], LIR: addi [81]
[BaselineBailouts]   Incoming frame ptr = 1d14a6ffe80
[...]
[IonBailouts] Took bailout! Snapshot offset: 373
[BaselineBailouts] Bailing to baseline ./poc.js:1:1 (IonScript=1d14a423f00) (FrameType=3)
[BaselineBailouts]   Reading from snapshot offset 373 size 389
[IonSnapshots] Creating snapshot reader
[IonSnapshots] Read snapshot header with bailout kind 1
[IonSnapshots] Read recover header with instructionCount 1
[IonSnapshots] Read RResumePoint (pc offset 179, mode ResumeAt, nslots 5)
[IonBailouts]  bailing from bytecode: JumpTarget, MIR: Add [76], LIR: addi [81]
[BaselineBailouts]   Incoming frame ptr = 1d14a6ffe80
[...]

Compare the Ion native code against CacheIR native code:

[Codegen] js::jit::CacheIRCompiler::emitInt32IncResult
[Codegen] 00060313       mv        t1, a2
[Codegen] 0003131b       slliw     t1, t1, 0 <--
[Codegen] 00030613       mv        a2, t1
[Codegen] 00160e93       addi      t4, a2, 1
[Codegen] 0016061b       addiw     a2, a2, 1
[Codegen] .use Llabel a51cffc418 on 56
[Codegen] 01d60063       beq       a2, t4, 0 -> 000000A51CFFC12C
[Codegen] .use Llabel a51cffdf58 on 60
[Codegen] 00000f17       auipc     t5, 0x0
[Codegen] 000f0067       jr        t5
[Codegen] .set Llabel a51cffc418 68
---
[Codegen] retarget a51cffda38 -> 1d14a48d2f0
[Codegen]                                 # LIR=AddI:OverflowCheck
[Codegen] 00150e93       addi      t4, a0, 1
[Codegen] 0015029b       addiw     t0, a0, 1
[Codegen] .use Llabel a51cffd898 on 13672
[Codegen] 01d28063       beq       t0, t4, 0 -> 000000A51CFFD5AC
[Codegen] .use Llabel a51cffda08 on 13676
[Codegen] 00000f17       auipc     t5, 0x0
[Codegen] 000f0067       jr        t5
[Codegen] .set Llabel a51cffd898 13684

It is clear that CacheIRCompiler::emitInt32IncResult performs a sign extension while visitAddI doesn't.

Ion run trace (bailout triggered):

  0x02508d709ed0      00013303       ld        t1, 0(sp)          	00000000ffffffff    (57748)    int64:4294967295 uint64:4294967295 <-- [addr: 1d14a6ffe68]
[...]
  0x02508d70a754      00130e93       addi      t4, t1, 1          	0000000100000000    (57797)    int64:4294967296 uint64:4294967296
  0x02508d70a758      0013029b       addiw     t0, t1, 1          	0000000000000000    (57798)    int64:0 uint64:0
  0x02508d70a75c      01d28663       beq       t0, t4, 12 -> 000002508D70A768	
[...] 

CacheIR run trace (no bailouts):

  0x02508d705634      00060313       mv        t1, a2             	fff88000ffffffff    (58209)    int64:-2111058030362625 uint64:18444633015679188991
  0x02508d705638      0003131b       slliw     t1, t1, 0          	ffffffffffffffff    (58210)    int64:-1 uint64:18446744073709551615
  0x02508d70563c      00030613       mv        a2, t1             	ffffffffffffffff    (58211)    int64:-1 uint64:18446744073709551615
  0x02508d705640      00160e93       addi      t4, a2, 1          	0000000000000000    (58212)    int64:0 uint64:0
  0x02508d705644      0016061b       addiw     a2, a2, 1          	0000000000000000    (58213)    int64:0 uint64:0
  0x02508d705648      01d60663       beq       a2, t4, 12 -> 000002508D705654	 

Comment 1

[Rong Bao [:csmantle]]

I don't think it's a visitAddI implementation bug. It seems that Ion is loading the I32 as an I64, which skipped sign extension. Curiously, I can't find where this ld is assembled. The snippet

  0x02508d709ed0      00013303       ld        t1, 0(sp)          	00000000ffffffff    (37163)    int64:4294967295 uint64:4294967295 <-- [addr: 1d14a6ffe68]
  0x02508d709ed4      00810113       addi      sp, sp, 8          	000001d14a6ffe70    (37164)    int64:1998408646256 uint64:1998408646256
  0x02508d709ed8      0082b483       ld        s1, 8(t0)          	00002852ed50d080    (37165)    int64:44336633925760 uint64:44336633925760 <-- [addr: 880e593f048]
  0x02508d709edc      0004be83       ld        t4, 0(s1)          	fff880000000000a    (37166)    int64:-2111062325329910 uint64:18444633011384221706 <-- [addr: 2852ed50d080]
[...]

... appears in JitSpew as this:

[Codegen] .set Llabel a51cffd838 13616
[Codegen] .set Llabel a51cffda54 13616
[Codegen] 00013303       ld        t1, 0(sp)
[Codegen] 00810113       addi      sp, sp, 8
[Codegen]                                 # LIR=Slots
[Codegen] 0082b483       ld        s1, 8(t0)
[Codegen]                                 # LIR=LoadDynamicSlotAndUnbox
[Codegen] 0004be83       ld        t4, 0(s1)
[Codegen] 000e9e93       slli      t4, t4, 0
[Codegen] 02fede93       srli      t4, t4, 47
[Codegen] 00020f37       lui       t5, 0x20
[Codegen] ff1f0f1b       addiw     t5, t5, -15

This ld t1, 0(sp) likely belongs to a pop sequence, but no matching LIR is recorded. The last # LIR= marker preceding this is # LIR=Pointer, which is a plain li. Jan, do you have ideas about where this ld comes from?

I am attaching the complete sim trace and JitSpew in later attachments.

Comment 2

[Rong Bao [:csmantle]]

Attachment: JitSpew with $IONFLAGS='all,bl-all'

Comment 3

[Rong Bao [:csmantle]]

Attachment: Simulator trace

Comment 4

[Rong Bao [:csmantle]]

I think I know why this crashes: The implementation of CompareExchange on riscv64 truncates the int32 rather than sign-extends it. The latter seems to be the intended behavior.

Compare:

• riscv64: https://searchfox.org/firefox-main/rev/3c23ce1368431d49bae08e8e211f7f2bf4e4829d/js/src/jit/riscv64/MacroAssembler-riscv64.cpp#4217-4222
• loong64: https://searchfox.org/firefox-main/rev/3c23ce1368431d49bae08e8e211f7f2bf4e4829d/js/src/jit/loong64/MacroAssembler-loong64.cpp#3560-3564

Comment 5

[Rong Bao [:csmantle]]

Attachment: Bug 1987592 - [riscv64] Part 1: Sign-extend Int32 in MacroAssembler-risv64 CompareExchange. r=jandem

Comment 6

[Rong Bao [:csmantle]]

Attachment: Bug 1987592 - Part 2: Add jit-test ion/bug1987592.js test case. r=jandem

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/cb69d87fead3
https://hg.mozilla.org/integration/autoland/rev/c75aad578252
[riscv64] Part 1: Sign-extend Int32 in MacroAssembler-risv64 CompareExchange. r=jandem
https://github.com/mozilla-firefox/firefox/commit/ef981982c9c2
https://hg.mozilla.org/integration/autoland/rev/5e3e47bbdc7c
Part 2: Add jit-test ion/bug1987592.js test case. r=jandem

Comment 8

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/c75aad578252
https://hg.mozilla.org/mozilla-central/rev/5e3e47bbdc7c