[X86] Add verifyTargetSDNode for x86 target specific nodes

[RKSimon]

This allows us to verify/assert the node's validity on creation instead of waiting for when its used in a combine/analysis.

Added some initial verification for X86ISD::KSHIFL\R and X86ISD::PSADBW types - more to follow.

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: Simon Pilgrim (RKSimon)

Changes

This allows us to verify/assert the node's validity on creation instead of waiting for when its used in a combine/analysis.

Added some initial verification for X86ISD::KSHIFL\R and X86ISD::PSADBW types - more to follow.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/X86/X86ISelLowering.cpp (+28)
• (modified) llvm/lib/Target/X86/X86ISelLowering.h (+4)

diff --git a/llvm/lib/Target/X86/X86ISelLowering.cpp b/llvm/lib/Target/X86/X86ISelLowering.cpp
index 33ddcb57e9b08b..8a4844bc0afce8 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.cpp
+++ b/llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -61009,3 +61009,31 @@ Align X86TargetLowering::getPrefLoopAlignment(MachineLoop *ML) const {
     return Align(1ULL << ExperimentalPrefInnermostLoopAlignment);
   return TargetLowering::getPrefLoopAlignment();
 }
+
+#ifndef NDEBUG
+void X86TargetLowering::verifyTargetSDNode(const SDNode *N) const {
+  switch (N->getOpcode()) {
+  default:
+    break;
+  case X86ISD::KSHIFTL:
+  case X86ISD::KSHIFTR: {
+    EVT VT = N->getValueType(0);
+    auto *Amt = cast<ConstantSDNode>(N->getOperand(1));
+    assert(Amt->getAPIntValue().ult(VT.getVectorNumElements()) &&
+           "Out of range KSHIFT shift amount");
+    break;
+  }
+  case X86ISD::PSADBW: {
+    EVT VT = N->getValueType(0);
+    SDValue LHS = N->getOperand(0);
+    SDValue RHS = N->getOperand(1);
+    assert((VT == MVT::v2i64 || VT == MVT::v4i64 || VT == MVT::v8i64) &&
+           LHS.getValueType() == RHS.getValueType() &&
+           LHS.getValueSizeInBits() == VT.getSizeInBits() &&
+           LHS.getValueType().getScalarType() == MVT::i8 &&
+           "Unexpected PSADBW types");
+    break;
+  }
+  }
+}
+#endif
diff --git a/llvm/lib/Target/X86/X86ISelLowering.h b/llvm/lib/Target/X86/X86ISelLowering.h
index 03f10a3c83e30c..b8517018067d8b 100644
--- a/llvm/lib/Target/X86/X86ISelLowering.h
+++ b/llvm/lib/Target/X86/X86ISelLowering.h
@@ -1658,6 +1658,10 @@ namespace llvm {
       return TargetLoweringBase::getTypeToTransformTo(Context, VT);
     }
 
+#ifndef NDEBUG
+    void verifyTargetSDNode(const SDNode *N) const override;
+#endif
+
   protected:
     std::pair<const TargetRegisterClass *, uint8_t>
     findRepresentativeClass(const TargetRegisterInfo *TRI,

[phoebewang]

We have defined the node with
SDTCVecEltisVT<0, i64>, SDTCVecEltisVT<1, i8> and SDTCisSameAs<1,2>.
Do we still need these check here?

[RKSimon]

That would only check at selection - it'd be nice to be able to auto generate these checks for verifyTargetSDNode but it'd be a rather large job.

What I'm finding is that verifyTargetSDNode is fantastic for tracing problems, especially during debugging, as its called as part of getNode() - so the assertion occurs in the code stack where the node is created - we have some similar asserts in the node combines / known bits etc. but working out where the node came from isn't always straightforward (or requires trawling through logs).

I'm tempted to remove the equivalent asserts from other locations as verifyTargetSDNode should have caught most cases.

[s-barannikov]

it'd be nice to be able to auto generate these checks for verifyTargetSDNode but it'd be a rather large job.

FYI I'm doing that in #119709.
Although as-is it doesn't catch everything yet.

[phoebewang]

more to follow

Is it worth putting them in a separate file if there are a lot of nodes to check?

[RKSimon]

Going to leave this as draft and see what we still need to do after #119709