Wasmtime GitHub notifications bot (Apr 29 2022 at 05:56): Wasmtime GitHub notifications bot (Apr 29 2022 at 05:56):cfallin commented on issue #4088:
(For context for those not already deeply lost in the depths of the x64 backend, the
movabslowering for immediates happens when the value doesn't fit in a signed 32-bit immediate field that most reg/mem/immediate operands support. So this sort of code would not appear when doing arithmetic on small integers, but appears all the time when 64-bit bitmasks are involved, e.g. in SpiderMonkey's NaN-(un)boxing sequences.)
Wasmtime GitHub notifications bot (Apr 29 2022 at 06:09):github-actions[bot] commented on issue #4088:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "isle"Thus the following users have been cc'd because of the following labels:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Apr 29 2022 at 10:22):bjorn3 commented on issue #4088:
How will this affect disassemblers? I presume they would try to interpret the constant pool as instructions and due to the varying size of x86 instructions completely mess up disassembly of instructions following the constant pool, right? Would it be an option to allow disabling the constant pool usage?
Wasmtime GitHub notifications bot (Apr 29 2022 at 17:09):cfallin commented on issue #4088:
How will this affect disassemblers? I presume they would try to interpret the constant pool as instructions and due to the varying size of x86 instructions completely mess up disassembly of instructions following the constant pool, right? Would it be an option to allow disabling the constant pool usage?
@bjorn3 most disassemblers will sync to function boundaries at least (if they have that metadata, e.g. with an ELF image), so the constant pool at the end will at worst become garbage instructions after the final
ret.I do agree it'd be better to put the constants somewhere else if possible (in
.rodataor at least between functions, outside of the ranges/function body sizes specified by metadata) -- but two factors work against this:
- Our per-function compilation strategy means that we would need a separate pass at the end to collect all constant pools, and do fixups for rip-relative references at that point. Right now with per-function constant pools each function body is completely independent and has no relocs for constant references.
- On some platforms (e.g. aarch64) the available range for constant references is limited (+/- 128MiB, IIRC), so we need at least the possibility of constant pools among/between functions as well as in a separate
.rodata. And as long as we're doing that, it's simpler to adopt a strategy that always works (constants per function, withMachBuffer's constant-island handling) rather than choose between strategies and have a rarely-used path (i.e., less well-tested).And finally I'll note that we have constant pools already, for SIMD constants; this PR is expanding their use, not introducing them at all.
Wasmtime GitHub notifications bot (Apr 29 2022 at 17:17):bjorn3 commented on issue #4088:
@bjorn3 most disassemblers will sync to function boundaries at least (if they have that metadata, e.g. with an ELF image), so the constant pool at the end will at worst become garbage instructions after the final ret.
This PR doesn't put the constant pool in the middle in case of large functions like it does for AArch64?
On some platforms (e.g. aarch64) the available range for constant references is limited (+/- 128MiB, IIRC)
For AArch64 it is less of an issue due to the instructions having a fixed size and thus there being no risk of the disassembler desyncing for as long as the constant pool size is a multiple of the instruction length.
Wasmtime GitHub notifications bot (Apr 29 2022 at 17:20):cfallin commented on issue #4088:
@bjorn3 most disassemblers will sync to function boundaries at least (if they have that metadata, e.g. with an ELF image), so the constant pool at the end will at worst become garbage instructions after the final ret.
This PR doesn't put the constant pool in the middle in case of large functions like it does for AArch64?
It doesn't change the behavior at all; the
MachBufferwill do whatever it did before for SIMD constants. On x86-64 the available PC-relative range is +/- 2GiB, so realistically the constant pool will always be at the end of the function.
Wasmtime GitHub notifications bot (Apr 29 2022 at 22:29):cfallin commented on issue #4088:
@abrown or @fitzgen: rebased and ready for review (stacks on #4080, which stacks on #4091).
Wasmtime GitHub notifications bot (May 02 2022 at 18:06):abrown commented on issue #4088:
How about we review this after we figure out #4080?
Wasmtime GitHub notifications bot (May 02 2022 at 23:22):cfallin commented on issue #4088:
Rebased now that #4080 merged.
Wasmtime GitHub notifications bot (May 10 2022 at 03:46):cfallin commented on issue #4088:
@abrown or @fitzgen, I think this should be OK to review now (thanks!).
Wasmtime GitHub notifications bot (May 10 2022 at 17:12):alexcrichton commented on issue #4088:
I was poking around and it looks like this PR fails tests on
maindue to what looks like a "merge conflict" in test added after this PR was created, but I suspect the test probably just needs an updated output
Wasmtime GitHub notifications bot (May 10 2022 at 17:16):cfallin commented on issue #4088:
Urgh, we really need a bors-like thing... I'll take a look, sorry!
Last updated: Oct 23 2024 at 20:03 UTC