Closed Bug 1521512 Opened 5 years ago Closed 3 years ago

The ReturnReg64 definition should follow the platform ABI

Categories

(Core :: JavaScript Engine: JIT, enhancement, P3)

x86
All
enhancement

Tracking

()

RESOLVED DUPLICATE of bug 1730161

People

(Reporter: lth, Unassigned)

References

(Blocks 1 open bug)

Details

Stumbled across this for bug 1394420 comment 20: on x86 we define ReturnReg64 as edi:eax, but the compilers we use use edx:eax. I've verified this on Linux by looking at Clang 6 and GCC 8 output; Agner Fog lists MSVC as using edx:eax, https://www.agner.org/optimize/calling_conventions.pdf p 10 but gets Linux wrong.

It's likely that if we get this wrong on some platform, we'll see cppunit tests fail for 64-bit atomic operations, that's how we found it this time.

Benjamin says (on IRC) we should change this to follow platform conventions. If we do that, we need to be aware that the x86 Ion code paths for 64-bit UDiv and UMod are register-constrained and appear to depend on the output register being edi:eax, although this requires a little experimentation. (Specifically, CodeGenerator-x86.cpp asserts that the output register is ReturnReg64, but lowering appears not to take any action to ensure that that is true other than using all the other registers for something else.)

On all other tier-1 platforms, ReturnReg64 is the C/C++ ABI 64-bit return register, notably also on ARM.

To add some information here: I was wondering how our x86 wasm callouts could work, if ReturnReg64 wasn't matching the ABI return registers. The reason is that we explicitly do a move for int64 div/mod on this platform:
https://searchfox.org/mozilla-central/source/js/src/jit/x86/CodeGenerator-x86.cpp#835-836

There might something related to register allocation constraints being too hard to represent (and regalloc gets blocked or something, while there might actually be a solution that regalloc can't find). It'd be nice to try to update ReturnReg64 to the correct value, remove the hack and see if it works.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.