[SystemZ] Use getSignedConstant() where necessary

[nikic]

This will avoid assertion failures once we disable implicit truncation in getConstant().

Inside adjustSubwordCmp() I ended up suppressing the issue with an explicit cast, because this code deals with a mix of unsigned and signed immediates.

[llvmbot]

@llvm/pr-subscribers-backend-systemz

Author: Nikita Popov (nikic)

Changes

This will avoid assertion failures once we disable implicit truncation in getConstant().

Inside adjustSubwordCmp() I ended up suppressing the issue with an explicit cast, because this code deals with a mix of unsigned and signed immediates.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp (+4-3)
• (modified) llvm/lib/Target/SystemZ/SystemZISelLowering.cpp (+7-6)

diff --git a/llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp b/llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp
index 90d7bd934af405..403d238aa5b528 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp
@@ -671,7 +671,7 @@ void SystemZDAGToDAGISel::getAddressOperands(const SystemZAddressingMode &AM,
   }
 
   // Lower the displacement to a TargetConstant.
-  Disp = CurDAG->getTargetConstant(AM.Disp, SDLoc(Base), VT);
+  Disp = CurDAG->getSignedTargetConstant(AM.Disp, SDLoc(Base), VT);
 }
 
 void SystemZDAGToDAGISel::getAddressOperands(const SystemZAddressingMode &AM,
@@ -2024,8 +2024,9 @@ SDValue SystemZDAGToDAGISel::expandSelectBoolean(SDNode *Node) {
                              CurDAG->getConstant(IPM.XORValue, DL, MVT::i32));
 
   if (IPM.AddValue)
-    Result = CurDAG->getNode(ISD::ADD, DL, MVT::i32, Result,
-                             CurDAG->getConstant(IPM.AddValue, DL, MVT::i32));
+    Result =
+        CurDAG->getNode(ISD::ADD, DL, MVT::i32, Result,
+                        CurDAG->getSignedConstant(IPM.AddValue, DL, MVT::i32));
 
   EVT VT = Node->getValueType(0);
   if (VT == MVT::i32 && IPM.Bit == 31) {
diff --git a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
index 78d91299a357dd..abeabab69cb9c9 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
@@ -1444,15 +1444,15 @@ void SystemZTargetLowering::LowerAsmOperandForConstraint(
     case 'K': // Signed 16-bit constant
       if (auto *C = dyn_cast<ConstantSDNode>(Op))
         if (isInt<16>(C->getSExtValue()))
-          Ops.push_back(DAG.getTargetConstant(C->getSExtValue(), SDLoc(Op),
-                                              Op.getValueType()));
+          Ops.push_back(DAG.getSignedTargetConstant(
+              C->getSExtValue(), SDLoc(Op), Op.getValueType()));
       return;
 
     case 'L': // Signed 20-bit displacement (on all targets we support)
       if (auto *C = dyn_cast<ConstantSDNode>(Op))
         if (isInt<20>(C->getSExtValue()))
-          Ops.push_back(DAG.getTargetConstant(C->getSExtValue(), SDLoc(Op),
-                                              Op.getValueType()));
+          Ops.push_back(DAG.getSignedTargetConstant(
+              C->getSExtValue(), SDLoc(Op), Op.getValueType()));
       return;
 
     case 'M': // 0x7fffffff
@@ -2578,7 +2578,7 @@ static void adjustSubwordCmp(SelectionDAG &DAG, const SDLoc &DL,
   // Make sure that the second operand is an i32 with the right value.
   if (C.Op1.getValueType() != MVT::i32 ||
       Value != ConstOp1->getZExtValue())
-    C.Op1 = DAG.getConstant(Value, DL, MVT::i32);
+    C.Op1 = DAG.getConstant((uint32_t)Value, DL, MVT::i32);
 }
 
 // Return true if Op is either an unextended load, or a load suitable
@@ -4623,7 +4623,8 @@ SDValue SystemZTargetLowering::lowerATOMIC_LOAD_OP(SDValue Op,
   if (Opcode == SystemZISD::ATOMIC_LOADW_SUB)
     if (auto *Const = dyn_cast<ConstantSDNode>(Src2)) {
       Opcode = SystemZISD::ATOMIC_LOADW_ADD;
-      Src2 = DAG.getConstant(-Const->getSExtValue(), DL, Src2.getValueType());
+      Src2 = DAG.getSignedConstant(-Const->getSExtValue(), DL,
+                                   Src2.getValueType());
     }
 
   SDValue AlignedAddr, BitShift, NegBitShift;

[uweigand]

I guess these changes look fine - the more interesting question is, does this catch all instances of getConstant that need to be updated? There's a lot more of those in the back-end ...

[nikic]

I guess these changes look fine - the more interesting question is, does this catch all instances of getConstant that need to be updated? There's a lot more of those in the back-end ...

It's all the instances that result in test failures :) I definitely wouldn't be surprised if there's more of these not covered by tests...

[uweigand]

Not sure what exactly the rule is, but if we're now supposed to use getSignedConstant in all cases where the constant is signed (possibly negative), then I see the following additional places that might need to be changed:

In SystemZSelectionDAGInfo.cpp - emitMemMemReg
In SystemZISelDAGToDAG.cpp - tryFoldLoadStoreIntoMemOperand
In SystemZISelLowering.cpp - lowerVectorSETCC, lowerGlobalAddress, lowerRETURNADDR, getCSAddressAndShifts
In SystemZOperands.td - SIMM8, SIMM16, NEGSIMM16, SIMM32, NEGSIMM32

[nikic]

Thanks, I've adjusted the additional places you pointed out (apart from tryFoldLoadStoreIntoMemOperand, which works on APInt).

Most of these get away with not using getSignedConstant() because they work on i64, which is the one width where signedness is irrelevant because it matches the native width of APInt.

This doesn't apply to the SIMM8 etc ones -- presumably those just don't have test coverage with negative values.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[uweigand]

LGTM now, thanks for taking care of SystemZ!