[SystemZ] SIMM32 is a signed constant

[redstar]

A follow-up to PR #117181: SIMM32 must use getSignedTargetConstant(), too.

[llvmbot]

@llvm/pr-subscribers-backend-systemz

Author: Kai Nacke (redstar)

Changes

A follow-up to PR #117181: SIMM32 must use getSignedTargetConstant(), too.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/SystemZ/SystemZOperands.td (+2-2)

diff --git a/llvm/lib/Target/SystemZ/SystemZOperands.td b/llvm/lib/Target/SystemZ/SystemZOperands.td
index 64345ca3a1394e..5349e0d9a8512c 100644
--- a/llvm/lib/Target/SystemZ/SystemZOperands.td
+++ b/llvm/lib/Target/SystemZ/SystemZOperands.td
@@ -262,8 +262,8 @@ def UIMM16 : SDNodeXForm<imm, [{
 
 // Truncate an immediate to a 32-bit signed quantity.
 def SIMM32 : SDNodeXForm<imm, [{
-  return CurDAG->getTargetConstant(int32_t(N->getZExtValue()), SDLoc(N),
-                                   MVT::i64);
+  return CurDAG->getSignedTargetConstant(int32_t(N->getZExtValue()), SDLoc(N),
+                                         MVT::i64);
 }]>;
 
 // Negate and then truncate an immediate to a 32-bit unsigned quantity.

[nikic]

This looks good to me, but it would be nice to add a test case.

[nikic]

Suggested change

	return CurDAG->getSignedTargetConstant(int32_t(N->getZExtValue()), SDLoc(N),
	return CurDAG->getSignedTargetConstant(int32_t(N->getSExtValue()), SDLoc(N),

Shouldn't make a difference here, but I think this would be more obvious...

[uweigand]

Good point. This should be done for all the SIMM variants then, I think. @redstar would you mind doing this in a follow-on PR?

[redstar]

Sure, I'll do that.

[redstar]

but it would be nice to add a test case.

Turns out to be more difficult than expected. The reason seems to be that the target type is MVT::i64, so the most significant 32 bit are always zero. However, in our not-yet-upstreamed 32 bit version of the backend, the type of the constant is MVT::i32, and then then msb matters.
Thus, the simple test

define i32 @foo(i32 %arg0) {
  %val = add i32 %arg0, -268435456
  ret i32 %val
}

crashes when targeting 32 bit mode, and compiles fine for 64 bit mode. The generated instruction is the same (afi r1,-268435456), the only difference being the type of the constant. Currently I see no way how to test this.

[uweigand]

LGTM. I think this should go in even if we don't have any simple test cases on upstream.

[uweigand]

Good point. This should be done for all the SIMM variants then, I think. @redstar would you mind doing this in a follow-on PR?