[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?