abrown commented on issue #3976:
cc: @bjorn3, @bnjbvr, @sunfishcode (people I think who may have used these instructions at some point): this PR came out of a discussion with @cfallin as I was thinking about porting the x64 memory access instructions to ISLE. He suggested we try removing these
*_complex
variants and I thought that seemed a good idea--less complexity! Please let me know if you have seen uses of these in the wild or if the rationale for their original inclusion is something other than the one I came up with in the description.
abrown commented on issue #3976:
cc: @alexcrichton, this is a significant change to the CLIF surface; if merged, we may want to think carefully about how this is versioned/released/communicated/etc.
github-actions[bot] commented on issue #3976:
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", "cranelift:meta", "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>
cfallin commented on issue #3976:
I think this makes a lot of sense; as Andrew and I discussed, these opcodes are unused at least in the Wasm frontend, and a quick grep through cg\_clif appears to show
load_complex
andstore_complex
are also unused there (@bjorn3 please correct us if wrong). Given no known users, and complexity in handling the variadic arguments in pattern-matching, and the non-orthogonality (the operation can be expressed asiadd
s to compute the address, then a simpleload
/store
), it seems reasonable to make this refactor. But anyone please let us know if there is a good reason not to!
cfallin commented on issue #3976:
Also, re: CLIF and communicating changes: we don't make any stability guarantees about the CLIF opcode set between releases (yet -- this will change when we get closer to thinking about a 1.0 release I suppose). So I don't think we need to do anything other than note the change in release notes. (Ensuring that no known users will be inconvenienced is a good step that we usually like to take though, if we can!)
bjorn3 commented on issue #3976:
Cg_clif indeed doesn't use load_complex and store_complex.
bnjbvr commented on issue #3976:
Retroactive +1 to this change! This whole family of instructions existed because ISLE didn't.
Last updated: Nov 22 2024 at 17:03 UTC