Wasmtime GitHub notifications bot (Sep 01 2022 at 01:23): Wasmtime GitHub notifications bot (Sep 01 2022 at 01:23):jameysharp commented on issue #2399:
How much of this PR is still applicable today? I see that
XmmLoadConststill exists in the x64 backend, so if removing that simplifies the backend then it'd be nice to pick that up. On the other hand I see the "implement packed shifts" commit merged ages ago and subsequently got ported to ISLE. cc: @elliottt
Wasmtime GitHub notifications bot (Sep 02 2022 at 03:04):abrown commented on issue #2399:
(Wow, have I been working on Cranelift this long?!?). Thanks for asking the question; it took took me down memory lane, though I will say some things are now hazy. I was probably stacking commits so all that really matters here are the last two:
- 2d46637 shows how we could get rid of
Inst::LoadXmmConst. I did not likeInst::LoadXmmConst, though I probably wrote it, because I considered it a bit of a hack; I have felt (like I mentioned in the last meeting) that the best abstraction to work at in the backend is to use actual ISA instructions (clearer, more stable, easier for newcomers to the project, etc.). I think this change is still a good refactoring and can probably be applied as-is? It also makes the CLIF prettier.- 5df1490 shows how to implement
const_addr--I guess that's not as necessary anymore since that instruction was removed. Maybe we file this away for later reference: I remember now that my thought forconst_addrwas to use it to index into constant tables. I can't remember for what lowering I though this was helpful, but if it is ever cheaper to dynamically load a constant from a table, that's whatconst_addrwould be good for.One other thought that came back to me while looking at this was that I really wanted to implement
vconst(and all other constant accesses for vectors) with the aligned version (MOVDQA,MOVAPS,MOVAPD) instead of the unaligned version we currently use. At the time, it was impossible to ensure that the vector constants were actually aligned so these aligned loads would have trapped--that's why I abandoned that thought, IIRC. But now that may be more possible (?). Now, I'm not sure if this will result in any great latency improvement on most CPUs we use today, but I did wonder if it might improve the cache line story. Just a thought...
Wasmtime GitHub notifications bot (Sep 02 2022 at 17:40):jameysharp commented on issue #2399:
As I understand it, since #4826 merged, all constants should be aligned now. If there are any instructions that require aligned loads for floats or integers I think those should be safe too. I'm not sure if our current tests or fuzzers would detect if constants are always sufficiently aligned, but I think it's worth an experiment.
Also, do you want to open a new PR for cherry-picking 2d466370db3585f0ed78131c48cd672817d3b47a to remove
LoadXmmConst?@elliottt tells me that he's working on something else but would be interested in tackling these later if you don't get to them first.
Wasmtime GitHub notifications bot (Sep 06 2022 at 18:07):abrown commented on issue #2399:
Also, do you want to open a new PR for cherry-picking https://github.com/bytecodealliance/wasmtime/commit/2d466370db3585f0ed78131c48cd672817d3b47a to remove LoadXmmConst?
Sure, https://github.com/bytecodealliance/wasmtime/pull/4876.
Wasmtime GitHub notifications bot (Sep 23 2022 at 17:14):jameysharp commented on issue #2399:
I've opened #4948 for the idea of using aligned loads for vector constants. With that, and since #4876 is merged, I think we've covered everything from this PR, so I'm closing it.
Last updated: Oct 23 2024 at 20:03 UTC