Skip to content

Conversation

@topperc
Copy link
Collaborator

@topperc topperc commented Apr 21, 2025

If an ATOMIC_LOAD has ZEXTLOAD/SEXTLOAD extension type we should trust that over getExtendForAtomicOps().

SystemZ is the only target that uses setAtomicLoadExtAction and they return ANY_EXTEND from getExtendForAtomicOps(). So I'm not sure there's a way to get a contradiction currently.

Note, type legalization uses getExtendForAtomicOps() when promoting ATOMIC_LOAD so we may not need to check getExtendForAtomicOps() for ATOMIC_LOAD. I have not done much investigating of this.

…dForAtomicOps() in computeKnownBits/ComputeNumSignBits.

If an ATOMIC_LOAD has ZEXTLOAD/SEXTLOAD extension type we should trust that
over getExtendForAtomicOps().

SystemZ is the only target that usesa setAtomicLoadExtAction and they return
ANY_EXTEND from getExtendForAtomicOps(). So I'm not sure there's a way to get
a contradiction currently.

Note, type legalization uses getExtendForAtomicOps() when promoting ATOMIC_LOAD
so we may not even need to check getExtendForAtomicOps() for ATOMIC_LOAD. I
have not done much investigating of this.
@llvmbot llvmbot added the llvm:SelectionDAG SelectionDAGISel as well label Apr 21, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 21, 2025

@llvm/pr-subscribers-llvm-selectiondag

Author: Craig Topper (topperc)

Changes

If an ATOMIC_LOAD has ZEXTLOAD/SEXTLOAD extension type we should trust that over getExtendForAtomicOps().

SystemZ is the only target that usesa setAtomicLoadExtAction and they return ANY_EXTEND from getExtendForAtomicOps(). So I'm not sure there's a way to get a contradiction currently.

Note, type legalization uses getExtendForAtomicOps() when promoting ATOMIC_LOAD so we may not need to check getExtendForAtomicOps() for ATOMIC_LOAD. I have not done much investigating of this.


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (+26-14)
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 09d73633462b6..ba6c5d884d381 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -4406,14 +4406,19 @@ KnownBits SelectionDAG::computeKnownBits(SDValue Op, const APInt &DemandedElts,
   case ISD::ATOMIC_LOAD_UMIN:
   case ISD::ATOMIC_LOAD_UMAX:
   case ISD::ATOMIC_LOAD: {
-    unsigned MemBits =
-        cast<AtomicSDNode>(Op)->getMemoryVT().getScalarSizeInBits();
     // If we are looking at the loaded value.
     if (Op.getResNo() == 0) {
-      if (TLI->getExtendForAtomicOps() == ISD::ZERO_EXTEND)
-        Known.Zero.setBitsFrom(MemBits);
-      else if (Op->getOpcode() == ISD::ATOMIC_LOAD &&
-               cast<AtomicSDNode>(Op)->getExtensionType() == ISD::ZEXTLOAD)
+      auto *AT = cast<AtomicSDNode>(Op);
+      unsigned MemBits = AT->getMemoryVT().getScalarSizeInBits();
+
+      // For atomic_load, prefer to use the extension type.
+      if (Op->getOpcode() == ISD::ATOMIC_LOAD) {
+        if (AT->getExtensionType() == ISD::ZEXTLOAD)
+          Known.Zero.setBitsFrom(MemBits);
+        else if (AT->getExtensionType() != ISD::SEXTLOAD &&
+                 TLI->getExtendForAtomicOps() == ISD::ZERO_EXTEND)
+          Known.Zero.setBitsFrom(MemBits);
+      } else if (TLI->getExtendForAtomicOps() == ISD::ZERO_EXTEND)
         Known.Zero.setBitsFrom(MemBits);
     }
     break;
@@ -5252,22 +5257,29 @@ unsigned SelectionDAG::ComputeNumSignBits(SDValue Op, const APInt &DemandedElts,
   case ISD::ATOMIC_LOAD_UMIN:
   case ISD::ATOMIC_LOAD_UMAX:
   case ISD::ATOMIC_LOAD: {
-    Tmp = cast<AtomicSDNode>(Op)->getMemoryVT().getScalarSizeInBits();
+    auto *AT = cast<AtomicSDNode>(Op);
     // If we are looking at the loaded value.
     if (Op.getResNo() == 0) {
+      Tmp = AT->getMemoryVT().getScalarSizeInBits();
       if (Tmp == VTBits)
         return 1; // early-out
-      if (TLI->getExtendForAtomicOps() == ISD::SIGN_EXTEND)
-        return VTBits - Tmp + 1;
-      if (TLI->getExtendForAtomicOps() == ISD::ZERO_EXTEND)
-        return VTBits - Tmp;
+
+      // For atomic_load, prefer to use the extension type.
       if (Op->getOpcode() == ISD::ATOMIC_LOAD) {
-        ISD::LoadExtType ETy = cast<AtomicSDNode>(Op)->getExtensionType();
-        if (ETy == ISD::SEXTLOAD)
+        switch (AT->getExtensionType()) {
+        default:
+          break;
+        case ISD::SEXTLOAD:
           return VTBits - Tmp + 1;
-        if (ETy == ISD::ZEXTLOAD)
+        case ISD::ZEXTLOAD:
           return VTBits - Tmp;
+        }
       }
+
+      if (TLI->getExtendForAtomicOps() == ISD::SIGN_EXTEND)
+        return VTBits - Tmp + 1;
+      if (TLI->getExtendForAtomicOps() == ISD::ZERO_EXTEND)
+        return VTBits - Tmp;
     }
     break;
   }

@topperc topperc merged commit 704fc65 into llvm:main Apr 21, 2025
13 checks passed
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
…dForAtomicOps() in computeKnownBits/ComputeNumSignBits. (llvm#136600)

If an ATOMIC_LOAD has ZEXTLOAD/SEXTLOAD extension type we should trust
that over getExtendForAtomicOps().

SystemZ is the only target that uses setAtomicLoadExtAction and they
return ANY_EXTEND from getExtendForAtomicOps(). So I'm not sure there's
a way to get a contradiction currently.

Note, type legalization uses getExtendForAtomicOps() when promoting
ATOMIC_LOAD so we may not need to check getExtendForAtomicOps() for
ATOMIC_LOAD. I have not done much investigating of this.
@topperc topperc deleted the pr/atomic-knownbits branch May 28, 2025 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llvm:SelectionDAG SelectionDAGISel as well

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants