[TableGen] Avoid warnings with INT64_MIN

[pfusik]

The number 0x8000000000000000 caused:

warning: integer constant is so large that it is unsigned

Emit the INT64_MIN macro instead of a negative literal.

Triggered in #151687

[llvmbot]

@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-tablegen

Author: Piotr Fusik (pfusik)

Changes

The number 0x8000000000000000 caused:

warning: integer constant is so large that it is unsigned

Emit the INT64_MIN macro instead of a negative literal.

Triggered in #151687

---

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

2 Files Affected:

• (added) llvm/test/TableGen/GlobalISelEmitter/int64min.td (+30)
• (modified) llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp (+1-1)

diff --git a/llvm/test/TableGen/GlobalISelEmitter/int64min.td b/llvm/test/TableGen/GlobalISelEmitter/int64min.td
new file mode 100644
index 0000000000000..7ca4cb44318ec
--- /dev/null
+++ b/llvm/test/TableGen/GlobalISelEmitter/int64min.td
@@ -0,0 +1,30 @@
+// RUN: llvm-tblgen -gen-global-isel -optimize-match-table=false -I %p/../../../include -I %p/../Common %s | FileCheck %s
+
+include "llvm/Target/Target.td"
+include "GlobalISelEmitterCommon.td"
+
+def GPR : RegisterClass<"MyTarget", [i64], 64, (add R0)>;
+def ANDI : I<(outs GPR:$dst), (ins GPR:$src1, i64imm:$src2), []>;
+
+// CHECK-LABEL: GIM_Try, /*On fail goto*//*Label 0*/ GIMT_Encode4(59), // Rule ID 0 //
+// CHECK-NEXT: GIM_CheckNumOperands, /*MI*/0, /*Expected*/3,
+// CHECK-NEXT: GIM_CheckOpcode, /*MI*/0, GIMT_Encode2(TargetOpcode::G_AND),
+// CHECK-NEXT: // MIs[0] DstI[dst]
+// CHECK-NEXT: GIM_RootCheckType, /*Op*/0, /*Type*/GILLT_s64,
+// CHECK-NEXT: GIM_RootCheckRegBankForClass, /*Op*/0, /*RC*/GIMT_Encode2(MyTarget::GPRRegClassID),
+// CHECK-NEXT: // MIs[0] rs1
+// CHECK-NEXT: GIM_RootCheckType, /*Op*/1, /*Type*/GILLT_s64,
+// CHECK-NEXT: GIM_RootCheckRegBankForClass, /*Op*/1, /*RC*/GIMT_Encode2(MyTarget::GPRRegClassID),
+// CHECK-NEXT: // MIs[0] Operand 2
+// CHECK-NEXT: GIM_RootCheckType, /*Op*/2, /*Type*/GILLT_s64,
+// CHECK-NEXT: GIM_CheckConstantInt, /*MI*/0, /*Op*/2, GIMT_Encode8(INT64_MIN),
+// CHECK-NEXT: // (and:{ *:[i64] } GPR:{ *:[i64] }:$rs1, -9223372036854775808:{ *:[i64] })  =>  (ANDI:{ *:[i64] } GPR:{ *:[i64] }:$rs1, -9223372036854775808:{ *:[i64] })
+// CHECK-NEXT: GIR_BuildRootMI, /*Opcode*/GIMT_Encode2(MyTarget::ANDI),
+// CHECK-NEXT: GIR_RootToRootCopy, /*OpIdx*/0, // DstI[dst]
+// CHECK-NEXT: GIR_RootToRootCopy, /*OpIdx*/1, // rs1
+// CHECK-NEXT: GIR_AddImm, /*InsnID*/0, /*Imm*/GIMT_Encode8(INT64_MIN),
+// CHECK-NEXT: GIR_RootConstrainSelectedInstOperands,
+// CHECK-NEXT: // GIR_Coverage, 0,
+// CHECK-NEXT: GIR_EraseRootFromParent_Done,
+def : Pat<(and GPR:$rs1, 0x8000000000000000),
+          (ANDI GPR:$rs1, 0x8000000000000000)>;
diff --git a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
index 70141ba738bdb..45b896c4091cf 100644
--- a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
+++ b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
@@ -237,7 +237,7 @@ MatchTableRecord MatchTable::NamedValue(unsigned NumBytes, StringRef Namespace,
 
 MatchTableRecord MatchTable::IntValue(unsigned NumBytes, int64_t IntValue) {
   assert(isUIntN(NumBytes * 8, IntValue) || isIntN(NumBytes * 8, IntValue));
-  auto Str = llvm::to_string(IntValue);
+  auto Str = IntValue == INT64_MIN ? "INT64_MIN" : llvm::to_string(IntValue);
   if (NumBytes == 1 && IntValue < 0)
     Str = "uint8_t(" + Str + ")";
   // TODO: Could optimize this directly to save the compiler some work when

[ornata]

in this code, how do we know when the user intends for a signed vs unsigned integer?

like if we pass INT64_MIN - 1, then I suppose we can assume that we want an unsigned value, based off the fact that the assert allows for isUIntN(NumBytes * 8, IntValue). But it also seems wrong to assume that.

do we need to have a separate function that allows for an unsigned uint64_t IntValue? Maybe I'm missing something here, but this code seems kind of fishy to me at a first glance.

[arsenm]

I'd expect to just emit the value with either cast / literal macro or type suffix

[pfusik]

I do not understand your comments regarding signed vs unsigned. llvm::to_string(IntValue) clearly accepts a signed integer and returns its decimal representation, possibly with a minus.
The problem is that -9223372036854775808 in C/C++ are two tokens, namely a unary minus and an unsigned integer literal, resulting in an unsigned integer and a warning. I don't see a reason why this particular constant should have a different type.
There are multiple ways to spell this constant ((-9223372036854775807 - 1), (int64_t(1) << 63)), but INT64_MIN is the most succint one. This macro is from stdint.h which is already used in the emitted file for uint32_t etc.

like if we pass INT64_MIN - 1

INT64_MIN - 1 equals INT64_MAX which is a signed integer.

[arsenm]

Suggested change

	if (NumBytes == 1 && IntValue < 0)
	Str = "uint8_t(" + Str + ")";
	if (IntValue < 0)
	Str = "uint" << NumBytes * 8 << "_t(" + Str + ")";

Simpler to stop special casing the 1 byte case?

[topperc]

I don't think that will fix the issue. Won't it print uint64_t(-9223372036854775808) which will give the same warning?

[pfusik]

It will keep the warning.

[arsenm]

also will need a cast to unsigned for the input to to_string

[pfusik]

Yes, converting as unsigned looks cleaner: #153391

[pfusik]

Alternative solution based on the review: #153391