Cranelift: Remove istore{8,16,32} and {s,u}load{8,16,32} instructions - #14163
Open
fitzgen wants to merge 2 commits into
Open
Cranelift: Remove istore{8,16,32} and {s,u}load{8,16,32} instructions#14163fitzgen wants to merge 2 commits into
istore{8,16,32} and {s,u}load{8,16,32} instructions#14163fitzgen wants to merge 2 commits into
Conversation
Do not forward the value from a store at memory location `L` to a load of `L` when the store was big endian and the load is little endian, or vice versa. Similar for redundant-load elimination. Note that dead-store elimination overwrites the same range of bytes in memory regardless of byte order, so it can still happen when the dead store is big and the overwriter is little or vice versa, so long as we update the memory state to correctly record the overwriter's byte order.
…ions The former is identical to an `ireduce` followed by a regular store, and the latter is identical to a regular load of the narrow width followed by an extend. No need to have fused versions of these operations in CLIF, our lowering backends can match on these patterns to generate the desired machine code just fine. Large diff, but its mostly just golden test expectations.
Member
|
I have a vague recollection that in the past we attempted this and concluded that the increase in compile times and IR size meant that it wasn't worth the change, but I also can't seem to find that context now. Perhaps this'd be worth at least running through sightglass though? |
Member
|
I agree that this is a good simplification in principle but also want to make sure we minimize performance impact -- so +1 to a Sightglass run, but also skimming through the test expectations, I see at least a few cases (e.g., here) where load becomes a load+extend in machine code so we should make sure we have all the equivalent lowerings to re-fuse load+extend (and reduce+store). |
github-actions
Bot
added
cranelift
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The former is identical to an
ireducefollowed by a regular store, and thelatter is identical to a regular load of the narrow width followed by an
extend. No need to have fused versions of these operations in CLIF, our lowering
backends can match on these patterns to generate the desired machine code just
fine.
Large diff, but it's mostly just golden test expectations.
Depends on #14162