1699155 - Establish invariants about the upper bits of 64-bit registers with 32-bit values

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Lars T Hansen [:lth]]

I believe the following to be true on our 64-bit platforms and I would like to find out if others believe the same or if there are known counterexamples.

At least at the end of every JS or Wasm operation (= SpiderMonkey bytecode or wasm bytecode; discuss), if a 64-bit GPR has a 32-bit value, then the upper 32 bits of the register are predictable in a platform-specific manner, as follows.

• on x64 and arm64, the upper bits are zero
• on mips64, the upper bits are the sign extension of the lower bits

In most cases this follows from the ISA descriptions: 32-bit operations on x64 and arm64 zero-extend to 64 bits while 32-bit operations on mips64 sign-extend. Additionally, mips64 has the constraint that the inputs to many operations must be properly sign-extended, and the simulators check that they are, so there's reason to believe that the invariant is upheld properly; no extra code is generated to maintain it. Furthermore, we have some comments in the code that note that eg masm.move32(Imm32, reg) zero-extends on x64/arm64 and sign-extends on mips64.

On x64 and arm64 the upper bits of the inputs are just ignored.

(RiscV, being little more than a rebranded MIPS, is believed to sign-extend also, but I don't know anything about the input constraints; it's not important.)

The reason I want to establish this is that with 4GB memories with explicit bounds checks, I have to zero-extend the 32-bit index to 64 bits before the check (because the bounds limit is a 64-bit value, in order to accomodate exactly 4GB) and then wrap it back to 32 bits after the check (because spectre masking must be allowed to poison the value, I can't necessarily reuse the input, though we can discuss this). If the suspected invariant holds, both the sign-extend and the wrap are no-ops on x64 and arm64, the platforms I care the most about.

Comment 1

[Jan de Mooij [:jandem]]

This makes sense. We rely on this already (at least on x64/arm64), for example if you read an element from a JS array, string, etc, we use a BaseIndex which interprets the index as a word-size value, so it better has its upper 32 bits cleared!

(In reply to Lars T Hansen [:lth] from comment #0)

The reason I want to establish this is that with 4GB memories with explicit bounds checks, I have to zero-extend the 32-bit index to 64 bits before the check (because the bounds limit is a 64-bit value, in order to accomodate exactly 4GB) and then wrap it back to 32 bits after the check (because spectre masking must be allowed to poison the value, I can't necessarily reuse the input, though we can discuss this). If the suspected invariant holds, both the sign-extend and the wrap are no-ops on x64 and arm64, the platforms I care the most about.

I'd suggest looking at how I implemented this for typed arrays, it has a similar optimization. See the MInt32ToIntPtr lowering code here. The spectre index masking happens after this and uses the IntPtr value.

Comment 2

[Jan de Mooij [:jandem]]

See also codegen for LInt32ToIntPtr.

Comment 3

[Lars T Hansen [:lth]]

Attachment: Bug 1699155 - Document a code generation invariant. r?jandem

Document the rules for the upper bits of 64-bit GPRs carrying 32-bit
values.

Comment 4

[Nicolas B. Pierron [:nbp] {away}]

I think the generic invariant would be that the upper bits are undefined. Then this invariant can be refined per-architecture, as we do assume when boxing on x64.

Having an invariant which is other than undefined would necessarily adds complexity for some architectures or others.

Having these be undefined implies that we cannot make use of these upper bits in generic code, but we could in architecture specific code.
To check these at runtime we could have a red-zone for the upper bits of registers, which asserts if generic code is calling a function which would take these upper bits as inputs.

Comment 5

[Lars T Hansen [:lth]]

I see, I guess that's a tweak on what I wrote. Since we only have the three architectures and these have known rules, it becomes a question of whether we should allow for some hypothetical architecture to have undefined / unspecified behavior.

Personally I doubt that there are architectures out there that do not have rules for what happens in this case, but of course I've not surveyed them all, and I don't think I need to make the claim. But I can tweak my prose to suggest that things may be more open-ended.

Comment 6

[Nicolas B. Pierron [:nbp] {away}]

The other option is to have explicit names about zero-extend or sign-extend as we already have, and accept that one architecture would be slower until it is specialize.

This would require less code and plug nicely in our MacroAssembler.
I guess either is fine.

Comment 7

[Lars T Hansen [:lth]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)

The other option is to have explicit names about zero-extend or sign-extend as we already have, and accept that one architecture would be slower until it is specialize.

Yes. I was hoping to avoid that, it seems like a lot of work (maybe only for MIPS if we assume zero-extend is the default and we need to introduce sign-extend variants or add zero-extension everywhere in codegen to conform to the Ion API). Unless I misunderstand what you mean.

The third option I had was to reduce the scope of this invariant to wasm, because that's a lot easier to audit and/or ensure, but that might require me to fork all the MIR used by wasm, basically, and I would prefer not to do that too.

(I mean, for my current need I could just add the extend and wrap instructions and not worry about it at all, but bounds checking in wasm is essentially everywhere, and the resulting slowdown and code size increase is not appealing at all. With the invariant, I get to do better on ARM64 and x64.)

Comment 8

[Lars T Hansen [:lth]]

(In reply to Jan de Mooij [:jandem] from comment #1)

I'd suggest looking at how I implemented this for typed arrays, it has a similar optimization. See the MInt32ToIntPtr lowering code here. The spectre index masking happens after this and uses the IntPtr value.

I see. That's pretty conservative, but it would seem to cover a lot of cases even so.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60eca49593ff
Document a code generation invariant. r=jandem DONTBUILD

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/60eca49593ff