[RISCV][GISel] Use relaxed_load/store in GISel atomic patterns. NFC #161712
+5
−5
Conversation
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
We have additional patterns for GISel because we need to make s16 and s32 legal for load/store. GISel does not distinquish integer and FP scalar types in LLT. We only know whether the load should be integer or FP after register bank selection. These patterns should have been updated to use relaxed_load/store when the patterns in RISCVInstrInfoA.td were updated. Without this we will miscompile loads/stores with strong memory ordering when Zalasr is enabled. This patch just fixes the miscompile, Zalasr will now cause a GISel abort in some cases. A follow up patch will add additional GISel patterns for Zalasr.
|
CC: @mehnadnerd |
|
@llvm/pr-subscribers-backend-risc-v Author: Craig Topper (topperc) ChangesWe have additional patterns for GISel because we need to make s16 and s32 legal for load/store. GISel does not distinquish integer and FP scalar types in LLT. We only know whether the load should be integer or FP after register bank selection. These patterns should have been updated to use relaxed_load/store when the patterns in RISCVInstrInfoA.td were updated. Without this we will miscompile loads/stores with strong memory ordering when Zalasr is enabled. This patch just fixes the miscompile, Zalasr will now cause a GISel abort in some cases. A follow up patch will add additional GISel patterns for Zalasr. Full diff: https://github.com/llvm/llvm-project/pull/161712.diff 1 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVGISel.td b/llvm/lib/Target/RISCV/RISCVGISel.td
index 19d5aff023d53..24579d4863dd8 100644
--- a/llvm/lib/Target/RISCV/RISCVGISel.td
+++ b/llvm/lib/Target/RISCV/RISCVGISel.td
@@ -110,16 +110,16 @@ def : StPat<truncstorei8, SB, GPR, i16>;
let Predicates = [HasAtomicLdSt] in {
// Prefer unsigned due to no c.lb in Zcb.
- def : LdPat<atomic_load_aext_8, LBU, i16>;
- def : LdPat<atomic_load_nonext_16, LH, i16>;
+ def : LdPat<relaxed_load<atomic_load_aext_8>, LBU, i16>;
+ def : LdPat<relaxed_load<atomic_load_nonext_16>, LH, i16>;
- def : StPat<atomic_store_8, SB, GPR, i16>;
- def : StPat<atomic_store_16, SH, GPR, i16>;
+ def : StPat<relaxed_store<atomic_store_8>, SB, GPR, i16>;
+ def : StPat<relaxed_store<atomic_store_16>, SH, GPR, i16>;
}
let Predicates = [HasAtomicLdSt, IsRV64] in {
- def : LdPat<atomic_load_nonext_32, LW, i32>;
- def : StPat<atomic_store_32, SW, GPR, i32>;
+ def : LdPat<relaxed_load<atomic_load_nonext_32>, LW, i32>;
+ def : StPat<relaxed_store<atomic_store_32>, SW, GPR, i32>;
}
//===----------------------------------------------------------------------===//
|
|
Sorry! Should we add a test so this won't regress? |
I intend to properly support Zalasr in GISel right after this. That will contain the test. If I put a test in this patch I have to test the abort case and that seemed like more work. |
lenary
approved these changes
Oct 3, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/186/builds/12885 Here is the relevant piece of the build log for the reference |
MixedMatched
pushed a commit
to MixedMatched/llvm-project
that referenced
this pull request
Oct 3, 2025
…lvm#161712) We have additional patterns for GISel because we need to make s16 and s32 legal for load/store. GISel does not distinquish integer and FP scalar types in LLT. We only know whether the load should be integer or FP after register bank selection. These patterns should have been updated to use relaxed_load/store when the patterns in RISCVInstrInfoA.td were updated. Without this we will miscompile loads/stores with strong memory ordering when Zalasr is enabled. This patch just fixes the miscompile, Zalasr will now cause a GISel abort in some cases. A follow up patch will add additional GISel patterns for Zalasr.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
We have additional patterns for GISel because we need to make s16 and s32 legal for load/store. GISel does not distinquish integer and FP scalar types in LLT. We only know whether the load should be integer or FP after register bank selection.
These patterns should have been updated to use relaxed_load/store when the patterns in RISCVInstrInfoA.td were updated. Without this we will miscompile loads/stores with strong memory ordering when Zalasr is enabled.
This patch just fixes the miscompile, Zalasr will now cause a GISel abort in some cases. A follow up patch will add additional GISel patterns for Zalasr.