[TableGen][RISCV][AArch64][GISel] Properly implement isAnyExtLoad/isSignExtLoad/isZeroExtLoad for IsAtomic in SelectionDAG. #137096

topperc · 2025-04-24T00:21:24Z

Support isAnyExtLoad() for IsAtomic in GISel.

Modify atomic_load_az* to check for extload or zextload. And rename to atomic_load_azext*

Add atomic_load_asext* and use in RISC-V. I used "asext" rather than "as" so it wouldn't be confused with the word "as".

…oad/isZeroExtLoad for IsAtomic in SelectionDAG. Support isAnyExtLoad() for IsAtomic in GISel. Modify atomic_load_az* to check for extload or zextload. Add atomic_load_asext* and use in RISC-V. I used "asext" rather than "as" so it wouldn't be confused with the word "as".

llvmbot · 2025-04-24T00:21:56Z

@llvm/pr-subscribers-backend-aarch64
@llvm/pr-subscribers-llvm-selectiondag
@llvm/pr-subscribers-backend-risc-v
@llvm/pr-subscribers-tablegen

@llvm/pr-subscribers-llvm-globalisel

Author: Craig Topper (topperc)

Changes

Support isAnyExtLoad() for IsAtomic in GISel.

Modify atomic_load_az* to check for extload or zextload.

Add atomic_load_asext* and use in RISC-V. I used "asext" rather than "as" so it wouldn't be confused with the word "as".

Full diff: https://github.com/llvm/llvm-project/pull/137096.diff

5 Files Affected:

(modified) llvm/include/llvm/Target/TargetSelectionDAG.td (+54-2)
(modified) llvm/lib/Target/RISCV/RISCVInstrInfoA.td (+3-26)
(modified) llvm/lib/Target/RISCV/RISCVInstrInfoZalasr.td (+6-6)
(modified) llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp (+23-10)
(modified) llvm/utils/TableGen/GlobalISelEmitter.cpp (+2-1)

diff --git a/llvm/include/llvm/Target/TargetSelectionDAG.td b/llvm/include/llvm/Target/TargetSelectionDAG.td
index a807ce267aacf..afea1ea577efa 100644
--- a/llvm/include/llvm/Target/TargetSelectionDAG.td
+++ b/llvm/include/llvm/Target/TargetSelectionDAG.td
@@ -1852,6 +1852,13 @@ def atomic_load_sext :
   let IsSignExtLoad = true;
 }
 
+/// Atomic load which any extends the excess high bits.
+def atomic_load_aext :
+  PatFrag<(ops node:$ptr), (atomic_load node:$ptr)> {
+  let IsAtomic = true; // FIXME: Should be IsLoad and/or IsAtomic?
+  let IsAnyExtLoad = true;
+}
+
 def atomic_load_8 :
   PatFrag<(ops node:$ptr),
           (atomic_load node:$ptr)> {
@@ -1891,6 +1898,12 @@ def atomic_load_zext_16 :
   let MemoryVT = i16;
 }
 
+def atomic_load_zext_32 :
+  PatFrag<(ops node:$ptr), (atomic_load_zext node:$ptr)> {
+  let IsAtomic = true; // FIXME: Should be IsLoad and/or IsAtomic?
+  let MemoryVT = i32;
+}
+
 def atomic_load_sext_8 :
   PatFrag<(ops node:$ptr), (atomic_load_sext node:$ptr)> {
   let IsAtomic = true; // FIXME: Should be IsLoad and/or IsAtomic?
@@ -1903,16 +1916,55 @@ def atomic_load_sext_16 :
   let MemoryVT = i16;
 }
 
+def atomic_load_sext_32 :
+  PatFrag<(ops node:$ptr), (atomic_load_sext node:$ptr)> {
+  let IsAtomic = true; // FIXME: Should be IsLoad and/or IsAtomic?
+  let MemoryVT = i32;
+}
+
+def atomic_load_aext_8 :
+  PatFrag<(ops node:$ptr), (atomic_load_aext node:$ptr)> {
+  let IsAtomic = true; // FIXME: Should be IsLoad and/or IsAtomic?
+  let MemoryVT = i8;
+}
+
+def atomic_load_aext_16 :
+  PatFrag<(ops node:$ptr), (atomic_load_aext node:$ptr)> {
+  let IsAtomic = true; // FIXME: Should be IsLoad and/or IsAtomic?
+  let MemoryVT = i16;
+}
+
+def atomic_load_aext_32 :
+  PatFrag<(ops node:$ptr), (atomic_load_aext node:$ptr)> {
+  let IsAtomic = true; // FIXME: Should be IsLoad and/or IsAtomic?
+  let MemoryVT = i32;
+}
+
 // Atomic load which zeroes or anyextends the high bits.
 def atomic_load_az_8 : PatFrags<(ops node:$op),
-                                [(atomic_load_8 node:$op),
+                                [(atomic_load_aext_8 node:$op),
                                  (atomic_load_zext_8 node:$op)]>;
 
 // Atomic load which zeroes or anyextends the high bits.
 def atomic_load_az_16 : PatFrags<(ops node:$op),
-                                 [(atomic_load_16 node:$op),
+                                 [(atomic_load_aext_16 node:$op),
                                   (atomic_load_zext_16 node:$op)]>;
 
+// Atomic load which sign extends or anyextends the high bits.
+def atomic_load_asext_8 : PatFrags<(ops node:$op),
+                                   [(atomic_load_aext_8 node:$op),
+                                    (atomic_load_sext_8 node:$op)]>;
+
+// Atomic load which sign extends or anyextends the high bits.
+def atomic_load_asext_16 : PatFrags<(ops node:$op),
+                                    [(atomic_load_aext_16 node:$op),
+                                     (atomic_load_sext_16 node:$op)]>;
+
+// Atomic load which sign extends or anyextends the high bits.
+def atomic_load_asext_32 : PatFrags<(ops node:$op),
+                                    [(atomic_load_aext_32 node:$op),
+                                     (atomic_load_sext_32 node:$op)]>;
+
 def nonext_masked_gather :
   PatFrag<(ops node:$def, node:$pred, node:$ptr, node:$idx),
           (masked_gather node:$def, node:$pred, node:$ptr, node:$idx), [{
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoA.td b/llvm/lib/Target/RISCV/RISCVInstrInfoA.td
index 6600b33d638c3..b348e774d50b8 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoA.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoA.td
@@ -118,29 +118,6 @@ defm AMOMAXU_D  : AMO_rr_aq_rl<0b11100, 0b011, "amomaxu.d">,
 // Pseudo-instructions and codegen patterns
 //===----------------------------------------------------------------------===//
 
-def riscv_atomic_asextload : PatFrag<(ops node:$ptr), (atomic_load node:$ptr), [{
-  ISD::LoadExtType ETy = cast<AtomicSDNode>(N)->getExtensionType();
-  return ETy == ISD::EXTLOAD || ETy == ISD::SEXTLOAD;
-}]>;
-
-def riscv_atomic_asextload_8 : PatFrag<(ops node:$ptr),
-                                       (riscv_atomic_asextload node:$ptr)> {
-  let IsAtomic = true;
-  let MemoryVT = i8;
-}
-
-def riscv_atomic_asextload_16 : PatFrag<(ops node:$ptr),
-                                        (riscv_atomic_asextload node:$ptr)> {
-  let IsAtomic = true;
-  let MemoryVT = i16;
-}
-
-def riscv_atomic_asextload_32 : PatFrag<(ops node:$ptr),
-                                        (riscv_atomic_asextload node:$ptr)> {
-  let IsAtomic = true;
-  let MemoryVT = i32;
-}
-
 let IsAtomic = 1 in {
 // An atomic load operation that does not need either acquire or release
 // semantics.
@@ -188,8 +165,8 @@ class seq_cst_store<PatFrag base>
 // any ordering. This is necessary because AtomicExpandPass has added fences to
 // atomic load/stores and changed them to unordered ones.
 let Predicates = [HasAtomicLdSt] in {
-  def : LdPat<relaxed_load<riscv_atomic_asextload_8>,  LB>;
-  def : LdPat<relaxed_load<riscv_atomic_asextload_16>, LH>;
+  def : LdPat<relaxed_load<atomic_load_asext_8>,  LB>;
+  def : LdPat<relaxed_load<atomic_load_asext_16>, LH>;
 
   def : StPat<relaxed_store<atomic_store_8>,  SB, GPR, XLenVT>;
   def : StPat<relaxed_store<atomic_store_16>, SH, GPR, XLenVT>;
@@ -201,7 +178,7 @@ let Predicates = [HasAtomicLdSt, IsRV32] in {
 }
 
 let Predicates = [HasAtomicLdSt, IsRV64] in {
-  def : LdPat<relaxed_load<riscv_atomic_asextload_32>, LW>;
+  def : LdPat<relaxed_load<atomic_load_asext_32>, LW>;
   def : LdPat<relaxed_load<atomic_load_64>, LD, i64>;
   def : StPat<relaxed_store<atomic_store_64>, SD, GPR, i64>;
 }
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZalasr.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZalasr.td
index f42352d1716b0..837aa7f1005af 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoZalasr.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZalasr.td
@@ -76,11 +76,11 @@ class PatSRL<SDPatternOperator OpNode, RVInst Inst, ValueType vt = XLenVT>
 let Predicates = [HasStdExtZalasr] in {
   // the sequentially consistent loads use
   //  .aq instead of .aqrl to match the psABI/A.7
-  def : PatLAQ<acquiring_load<riscv_atomic_asextload_8>, LB_AQ>;
-  def : PatLAQ<seq_cst_load<riscv_atomic_asextload_8>, LB_AQ>;
+  def : PatLAQ<acquiring_load<atomic_load_asext_8>, LB_AQ>;
+  def : PatLAQ<seq_cst_load<atomic_load_asext_8>, LB_AQ>;
 
-  def : PatLAQ<acquiring_load<riscv_atomic_asextload_16>, LH_AQ>;
-  def : PatLAQ<seq_cst_load<riscv_atomic_asextload_16>, LH_AQ>;
+  def : PatLAQ<acquiring_load<atomic_load_asext_16>, LH_AQ>;
+  def : PatLAQ<seq_cst_load<atomic_load_asext_16>, LH_AQ>;
 
   // the sequentially consistent stores use
   //  .rl instead of .aqrl to match the psABI/A.7
@@ -101,8 +101,8 @@ let Predicates = [HasStdExtZalasr, IsRV32] in {
 } // Predicates = [HasStdExtZalasr, IsRV64]
 
 let Predicates = [HasStdExtZalasr, IsRV64] in {
-  def : PatLAQ<acquiring_load<riscv_atomic_asextload_32>, LW_AQ>;
-  def : PatLAQ<seq_cst_load<riscv_atomic_asextload_32>, LW_AQ>;
+  def : PatLAQ<acquiring_load<atomic_load_asext_32>, LW_AQ>;
+  def : PatLAQ<seq_cst_load<atomic_load_asext_32>, LW_AQ>;
 
   def : PatLAQ<acquiring_load<atomic_load_64>, LD_AQ>;
   def : PatLAQ<seq_cst_load<atomic_load_64>, LD_AQ>;
diff --git a/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp b/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp
index 7f58c4a88c76d..dac88ced4ac36 100644
--- a/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp
+++ b/llvm/utils/TableGen/Common/CodeGenDAGPatterns.cpp
@@ -910,7 +910,7 @@ std::string TreePredicateFn::getPredCode() const {
 
   if (!isLoad() && !isStore() && !isAtomic() && getMemoryVT())
     PrintFatalError(getOrigPatFragRecord()->getRecord()->getLoc(),
-                    "MemoryVT requires IsLoad or IsStore");
+                    "MemoryVT requires IsLoad or IsStore or IsAtomic");
 
   if (!isLoad() && !isStore()) {
     if (isUnindexed())
@@ -937,11 +937,10 @@ std::string TreePredicateFn::getPredCode() const {
     if (isNonExtLoad())
       PrintFatalError(getOrigPatFragRecord()->getRecord()->getLoc(),
                       "IsNonExtLoad requires IsLoad");
-    if (isAnyExtLoad())
-      PrintFatalError(getOrigPatFragRecord()->getRecord()->getLoc(),
-                      "IsAnyExtLoad requires IsLoad");
-
     if (!isAtomic()) {
+      if (isAnyExtLoad())
+        PrintFatalError(getOrigPatFragRecord()->getRecord()->getLoc(),
+                        "IsAnyExtLoad requires IsLoad or IsAtomic");
       if (isSignExtLoad())
         PrintFatalError(getOrigPatFragRecord()->getRecord()->getLoc(),
                         "IsSignExtLoad requires IsLoad or IsAtomic");
@@ -970,8 +969,9 @@ std::string TreePredicateFn::getPredCode() const {
     if (getMemoryVT() == nullptr && !isAtomicOrderingMonotonic() &&
         getAddressSpaces() == nullptr &&
         // FIXME: Should atomic loads be IsLoad, IsAtomic, or both?
-        !isZeroExtLoad() && !isSignExtLoad() && !isAtomicOrderingAcquire() &&
-        !isAtomicOrderingRelease() && !isAtomicOrderingAcquireRelease() &&
+        !isAnyExtLoad() && !isZeroExtLoad() && !isSignExtLoad() &&
+        !isAtomicOrderingAcquire() && !isAtomicOrderingRelease() &&
+        !isAtomicOrderingAcquireRelease() &&
         !isAtomicOrderingSequentiallyConsistent() &&
         !isAtomicOrderingAcquireOrStronger() &&
         !isAtomicOrderingReleaseOrStronger() &&
@@ -1075,9 +1075,22 @@ std::string TreePredicateFn::getPredCode() const {
         "if (isReleaseOrStronger(cast<AtomicSDNode>(N)->getMergedOrdering())) "
         "return false;\n";
 
-  // TODO: Handle atomic sextload/zextload normally when ATOMIC_LOAD is removed.
-  if (isAtomic() && (isZeroExtLoad() || isSignExtLoad()))
-    Code += "return false;\n";
+  if (isAtomic()) {
+    if ((isAnyExtLoad() + isSignExtLoad() + isZeroExtLoad()) > 1)
+      PrintFatalError(getOrigPatFragRecord()->getRecord()->getLoc(),
+                      "IsNonExtLoad, IsAnyExtLoad, IsSignExtLoad, and "
+                      "IsZeroExtLoad are mutually exclusive");
+
+    if (isAnyExtLoad())
+      Code += "if (cast<AtomicSDNode>(N)->getExtensionType() != ISD::EXTLOAD) "
+              "return false;\n";
+    if (isSignExtLoad())
+      Code += "if (cast<AtomicSDNode>(N)->getExtensionType() != ISD::SEXTLOAD) "
+              "return false;\n";
+    if (isZeroExtLoad())
+      Code += "if (cast<AtomicSDNode>(N)->getExtensionType() != ISD::ZEXTLOAD) "
+              "return false;\n";
+  }
 
   if (isLoad() || isStore()) {
     StringRef SDNodeName = isLoad() ? "LoadSDNode" : "StoreSDNode";
diff --git a/llvm/utils/TableGen/GlobalISelEmitter.cpp b/llvm/utils/TableGen/GlobalISelEmitter.cpp
index ccc4c00fca047..affecc34468ec 100644
--- a/llvm/utils/TableGen/GlobalISelEmitter.cpp
+++ b/llvm/utils/TableGen/GlobalISelEmitter.cpp
@@ -624,7 +624,8 @@ Expected<InstructionMatcher &> GlobalISelEmitter::addBuiltinPredicates(
         0, MemoryVsLLTSizePredicateMatcher::EqualTo, 0);
     return InsnMatcher;
   }
-  if (Predicate.isLoad() && Predicate.isAnyExtLoad()) {
+  if ((Predicate.isLoad() || Predicate.isAtomic()) &&
+      Predicate.isAnyExtLoad()) {
     InsnMatcher.addPredicate<MemoryVsLLTSizePredicateMatcher>(
         0, MemoryVsLLTSizePredicateMatcher::LessThan, 0);
     return InsnMatcher;

s-barannikov

The title should mention GISel?

llvm/include/llvm/Target/TargetSelectionDAG.td

s-barannikov

LGTM, but I think @arsenm should take a look too.

arsenm · 2025-04-24T13:23:12Z

llvm/include/llvm/Target/TargetSelectionDAG.td

+
+def atomic_load_aext_16 :
+  PatFrag<(ops node:$ptr), (atomic_load_aext node:$ptr)> {
+  let IsAtomic = true; // FIXME: Should be IsLoad and/or IsAtomic?


There's some wonky logic in both emitters that seem to assume you only set one of these bits per pat frag. I don't think it works to set both, should probably do something about that.

The SelectionDAG emitter has some checks for multiple bits being set, but I don't know if it is complete.

…ignExtLoad/isZeroExtLoad for IsAtomic in SelectionDAG. (llvm#137096) Support isAnyExtLoad() for IsAtomic in GISel. Modify atomic_load_az* to check for extload or zextload. And rename to atomic_load_azext* Add atomic_load_asext* and use in RISC-V. I used "asext" rather than "as" so it wouldn't be confused with the word "as".

topperc requested review from aemerson, arsenm and s-barannikov April 24, 2025 00:21

llvmbot added backend:RISC-V tablegen llvm:globalisel llvm:SelectionDAG SelectionDAGISel as well labels Apr 24, 2025

topperc mentioned this pull request Apr 24, 2025

[RISCV] Remove AND mask generated by ( zext ( atomic_load ) ) by replacing the load with zextload for orderings not stronger then monotonic. #136502

Merged

s-barannikov reviewed Apr 24, 2025

View reviewed changes

llvm/include/llvm/Target/TargetSelectionDAG.td Show resolved Hide resolved

topperc changed the title ~~[TableGen][RISCV][AArch64] Properly implement isAnyExtLoad/isSignExtLoad/isZeroExtLoad for IsAtomic in SelectionDAG.~~ [TableGen][RISCV][AArch64][GISel] Properly implement isAnyExtLoad/isSignExtLoad/isZeroExtLoad for IsAtomic in SelectionDAG. Apr 24, 2025

fixup! Rename atomic_load_az*

6ab9b56

llvmbot added the backend:AArch64 label Apr 24, 2025

s-barannikov approved these changes Apr 24, 2025

View reviewed changes

arsenm approved these changes Apr 24, 2025

View reviewed changes

fixup! fix copy paste mistake in error message.

46a24fa

topperc merged commit 2ca071b into llvm:main Apr 24, 2025
6 of 10 checks passed

topperc deleted the pr/atomic-load-ext-sdag branch April 24, 2025 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TableGen][RISCV][AArch64][GISel] Properly implement isAnyExtLoad/isSignExtLoad/isZeroExtLoad for IsAtomic in SelectionDAG. #137096

[TableGen][RISCV][AArch64][GISel] Properly implement isAnyExtLoad/isSignExtLoad/isZeroExtLoad for IsAtomic in SelectionDAG. #137096

Uh oh!

topperc commented Apr 24, 2025 •

edited

Loading

Uh oh!

llvmbot commented Apr 24, 2025 •

edited

Loading

Uh oh!

s-barannikov left a comment

Uh oh!

Uh oh!

s-barannikov left a comment

Uh oh!

arsenm Apr 24, 2025

Uh oh!

topperc Apr 24, 2025

Uh oh!

Uh oh!

Uh oh!

[TableGen][RISCV][AArch64][GISel] Properly implement isAnyExtLoad/isSignExtLoad/isZeroExtLoad for IsAtomic in SelectionDAG. #137096

[TableGen][RISCV][AArch64][GISel] Properly implement isAnyExtLoad/isSignExtLoad/isZeroExtLoad for IsAtomic in SelectionDAG. #137096

Uh oh!

Conversation

topperc commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-barannikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

s-barannikov left a comment

Choose a reason for hiding this comment

Uh oh!

arsenm Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

topperc Apr 24, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

topperc commented Apr 24, 2025 •

edited

Loading

llvmbot commented Apr 24, 2025 •

edited

Loading