[SelectionDAG] Fix AArch64 machine verifier bug when expanding LOOP_DEPENDENCE_MASK

[AZero13]

Fixes: #168227

TargetConstant nodes don't match TableGen ImmLeaf patterns during instruction selection. When this zero constant flows into the AArch64 CCMP formation code, the machine verifier hits an assertion in expensive checks.

[llvmbot]

@llvm/pr-subscribers-llvm-selectiondag

@llvm/pr-subscribers-backend-aarch64

Author: AZero13 (AZero13)

Changes

We did not ensure new opcodes like mi/pl were filtered out when swapping, and TargetConstant nodes don't match TableGen ImmLeaf patterns during instruction selection. When this zero constant flows into the AArch64 CCMP formation code, the machine verifier hit an assertion in expensive checks.

---

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

3 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp (+1-1)
• (modified) llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp (+1-1)
• (modified) llvm/lib/Target/AArch64/AArch64ISelLowering.cpp (+6-3)

diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
index 7d979caa8bf82..e8d9bce43f6ea 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
@@ -1829,7 +1829,7 @@ SDValue VectorLegalizer::ExpandLOOP_DEPENDENCE_MASK(SDNode *N) {
   // If the difference is positive then some elements may alias
   EVT CmpVT = TLI.getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(),
                                      Diff.getValueType());
-  SDValue Zero = DAG.getTargetConstant(0, DL, PtrVT);
+  SDValue Zero = DAG.getConstant(0, DL, PtrVT);
   SDValue Cmp = DAG.getSetCC(DL, CmpVT, Diff, Zero,
                              IsReadAfterWrite ? ISD::SETEQ : ISD::SETLE);
 
diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
index 417122d467054..71eeee78bd868 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
@@ -413,7 +413,7 @@ SDValue DAGTypeLegalizer::ScalarizeVecRes_LOOP_DEPENDENCE_MASK(SDNode *N) {
   SDValue Diff = DAG.getNode(ISD::SUB, DL, PtrVT, SinkValue, SourceValue);
   EVT CmpVT = TLI.getSetCCResultType(DAG.getDataLayout(), *DAG.getContext(),
                                      Diff.getValueType());
-  SDValue Zero = DAG.getTargetConstant(0, DL, PtrVT);
+  SDValue Zero = DAG.getConstant(0, DL, PtrVT);
   return DAG.getNode(ISD::OR, DL, CmpVT,
                      DAG.getSetCC(DL, CmpVT, Diff, EltSize, ISD::SETGE),
                      DAG.getSetCC(DL, CmpVT, Diff, Zero, ISD::SETEQ));
diff --git a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
index 81c87ace76e56..564c7bca0a0d6 100644
--- a/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
+++ b/llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
@@ -26013,9 +26013,12 @@ static SDValue reassociateCSELOperandsForCSE(SDNode *N, SelectionDAG &DAG) {
     // Try again with the operands of the SUBS instruction and the condition
     // swapped. Due to canonicalization, this only helps for non-constant
     // operands of the SUBS instruction.
-    std::swap(CmpOpToMatch, CmpOpOther);
-    if (SDValue R = Fold(getSwappedCondition(CC), CmpOpToMatch, CmpOpToMatch))
-      return R;
+    auto NewCC = getSwappedCondition(CC);
+    if (NewCC != AArch64CC::AL) {
+      std::swap(CmpOpToMatch, CmpOpOther);
+      if (SDValue R = Fold(NewCC, CmpOpToMatch, CmpOpToMatch))
+        return R;
+    }
     return SDValue();
   }
 

[AZero13]

@arsenm @RKSimon @davemgreen @topperc

[AZero13]

@juliannagele

[topperc]

The type legalizer changes LGTM. I don't know anything about the AArch64ISelLowering changes.

[MacDue]

Does this have any relation to the LOOP_DEPENDENCE_MASK change? Looks like you've just copied in #160753.

[AZero13]

Yeah that commit was fused in by mistake.

[AZero13]

I do not have merge permissions by the way. The issue has been addressed!