[TableGen] Avoid warnings with INT64_MIN #152996
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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)>; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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 = IntValue == INT64_MIN ? "INT64_MIN" : llvm::to_string(IntValue); | ||||||||||
| if (NumBytes == 1 && IntValue < 0) | ||||||||||
| Str = "uint8_t(" + Str + ")"; | ||||||||||
|
Comment on lines
241
to
242
Contributor
There was a problem hiding this comment.
Suggested change
Simpler to stop special casing the 1 byte case?
Collaborator
There was a problem hiding this comment. I don't think that will fix the issue. Won't it print
Contributor
Author
There was a problem hiding this comment. It will keep the warning.
Contributor
There was a problem hiding this comment. also will need a cast to unsigned for the input to to_string
Contributor
Author
There was a problem hiding this comment. Yes, converting as unsigned looks cleaner: #153391 |
||||||||||
| // TODO: Could optimize this directly to save the compiler some work when | ||||||||||
|
|
||||||||||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 forisUIntN(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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect to just emit the value with either cast / literal macro or type suffix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
-9223372036854775808in 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)), butINT64_MINis the most succint one. This macro is fromstdint.hwhich is already used in the emitted file foruint32_tetc.INT64_MIN - 1equalsINT64_MAXwhich is a signed integer.