-
Notifications
You must be signed in to change notification settings - Fork 15.5k
[RISCV][NFC] Simplify Imm range checks #170497
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@llvm/pr-subscribers-backend-risc-v Author: Piotr Fusik (pfusik) ChangesFull diff: https://github.com/llvm/llvm-project/pull/170497.diff 8 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVGISel.td b/llvm/lib/Target/RISCV/RISCVGISel.td
index eba35ef0a746d..67d2cacd5cdb9 100644
--- a/llvm/lib/Target/RISCV/RISCVGISel.td
+++ b/llvm/lib/Target/RISCV/RISCVGISel.td
@@ -17,14 +17,14 @@ include "RISCV.td"
include "RISCVCombine.td"
def simm12Plus1 : ImmLeaf<XLenVT, [{
- return (isInt<12>(Imm) && Imm != -2048) || Imm == 2048;}]>;
+ return Imm >= -2047 && Imm <= 2048;}]>;
def simm12Plus1i32 : ImmLeaf<i32, [{
- return (isInt<12>(Imm) && Imm != -2048) || Imm == 2048;}]>;
+ return Imm >= -2047 && Imm <= 2048;}]>;
// FIXME: This doesn't check that the G_CONSTANT we're deriving the immediate
// from is only used once
def simm12Minus1Nonzero : ImmLeaf<XLenVT, [{
- return (Imm >= -2049 && Imm < 0) || (Imm > 0 && Imm <= 2046);}]>;
+ return Imm >= -2049 && Imm <= 2046 && Imm != 0;}]>;
def simm12Minus1NonzeroNonNeg1 : ImmLeaf<XLenVT, [{
return (Imm >= -2049 && Imm < -1) || (Imm > 0 && Imm <= 2046);}]>;
diff --git a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
index 7cf6f203fda89..88431ed1c7ad1 100644
--- a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
@@ -4315,14 +4315,14 @@ bool RISCVDAGToDAGISel::selectVSplatSimm5(SDValue N, SDValue &SplatVal) {
bool RISCVDAGToDAGISel::selectVSplatSimm5Plus1(SDValue N, SDValue &SplatVal) {
return selectVSplatImmHelper(
N, SplatVal, *CurDAG, *Subtarget,
- [](int64_t Imm) { return (isInt<5>(Imm) && Imm != -16) || Imm == 16; },
+ [](int64_t Imm) { return Imm >= -15 && Imm <= 16; },
/*Decrement=*/true);
}
bool RISCVDAGToDAGISel::selectVSplatSimm5Plus1NoDec(SDValue N, SDValue &SplatVal) {
return selectVSplatImmHelper(
N, SplatVal, *CurDAG, *Subtarget,
- [](int64_t Imm) { return (isInt<5>(Imm) && Imm != -16) || Imm == 16; },
+ [](int64_t Imm) { return Imm >= -15 && Imm <= 16; },
/*Decrement=*/false);
}
@@ -4331,7 +4331,7 @@ bool RISCVDAGToDAGISel::selectVSplatSimm5Plus1NonZero(SDValue N,
return selectVSplatImmHelper(
N, SplatVal, *CurDAG, *Subtarget,
[](int64_t Imm) {
- return Imm != 0 && ((isInt<5>(Imm) && Imm != -16) || Imm == 16);
+ return Imm != 0 && Imm >= -15 && Imm <= 16;
},
/*Decrement=*/true);
}
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 89ec4a2a4a3e1..037786c32ebbe 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -2898,13 +2898,13 @@ bool RISCVInstrInfo::verifyInstruction(const MachineInstr &MI,
Ok = isShiftedUInt<4, 1>(Imm);
break;
case RISCVOp::OPERAND_UIMM5_NONZERO:
- Ok = isUInt<5>(Imm) && (Imm != 0);
+ Ok = Imm >= 1 && Imm <= 31;
break;
case RISCVOp::OPERAND_UIMM5_GT3:
- Ok = isUInt<5>(Imm) && (Imm > 3);
+ Ok = Imm >= 4 && Imm <= 31;
break;
case RISCVOp::OPERAND_UIMM5_PLUS1:
- Ok = (isUInt<5>(Imm) && (Imm != 0)) || (Imm == 32);
+ Ok = Imm >= 1 && Imm <= 32;
break;
case RISCVOp::OPERAND_UIMM6_LSB0:
Ok = isShiftedUInt<5, 1>(Imm);
@@ -2937,7 +2937,7 @@ bool RISCVInstrInfo::verifyInstruction(const MachineInstr &MI,
Ok = isShiftedUInt<8, 2>(Imm) && (Imm != 0);
break;
case RISCVOp::OPERAND_UIMM16_NONZERO:
- Ok = isUInt<16>(Imm) && (Imm != 0);
+ Ok = Imm >= 1 && Imm <= 65535;
break;
case RISCVOp::OPERAND_THREE:
Ok = Imm == 3;
@@ -2946,7 +2946,7 @@ bool RISCVInstrInfo::verifyInstruction(const MachineInstr &MI,
Ok = Imm == 4;
break;
case RISCVOp::OPERAND_IMM5_ZIBI:
- Ok = (isUInt<5>(Imm) && Imm != 0) || Imm == -1;
+ Ok = (Imm >= 1 && Imm <= 31) || Imm == -1;
break;
// clang-format off
CASE_OPERAND_SIMM(5)
@@ -2957,7 +2957,7 @@ bool RISCVInstrInfo::verifyInstruction(const MachineInstr &MI,
CASE_OPERAND_SIMM(26)
// clang-format on
case RISCVOp::OPERAND_SIMM5_PLUS1:
- Ok = (isInt<5>(Imm) && Imm != -16) || Imm == 16;
+ Ok = Imm >= -15 && Imm <= 16;
break;
case RISCVOp::OPERAND_SIMM5_NONZERO:
Ok = isInt<5>(Imm) && (Imm != 0);
@@ -2991,7 +2991,7 @@ bool RISCVInstrInfo::verifyInstruction(const MachineInstr &MI,
Ok = Ok && Imm != 0;
break;
case RISCVOp::OPERAND_CLUI_IMM:
- Ok = (isUInt<5>(Imm) && Imm != 0) ||
+ Ok = (Imm >= 1 && Imm <= 31) ||
(Imm >= 0xfffe0 && Imm <= 0xfffff);
break;
case RISCVOp::OPERAND_RVKRNUM:
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.td b/llvm/lib/Target/RISCV/RISCVInstrInfo.td
index 84b962b2a8607..99dda07b582f9 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -507,11 +507,11 @@ def ixlenimm_li_restricted : Operand<XLenVT> {
// A 12-bit signed immediate plus one where the imm range will be -2047~2048.
def simm12_plus1 : ImmLeaf<XLenVT,
- [{return (isInt<12>(Imm) && Imm != -2048) || Imm == 2048;}]>;
+ [{return Imm >= -2047 && Imm <= 2048;}]>;
// A 6-bit constant greater than 32.
def uimm6gt32 : ImmLeaf<XLenVT, [{
- return isUInt<6>(Imm) && Imm > 32;
+ return Imm >= 33 && Imm <= 63;
}]>;
// Addressing modes.
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoC.td b/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
index 24e7a0ee5a79f..f30d2a772bdd4 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
@@ -17,9 +17,7 @@ def UImmLog2XLenNonZeroAsmOperand : AsmOperandClass {
}
def uimmlog2xlennonzero : RISCVOp, ImmLeaf<XLenVT, [{
- if (Subtarget->is64Bit())
- return isUInt<6>(Imm) && (Imm != 0);
- return isUInt<5>(Imm) && (Imm != 0);
+ return Imm >= 1 && Imm <= (Subtarget->is64Bit() ? 63 : 31);
}]> {
let ParserMatchClass = UImmLog2XLenNonZeroAsmOperand;
let DecoderMethod = "decodeUImmLog2XLenNonZeroOperand";
@@ -28,9 +26,11 @@ def uimmlog2xlennonzero : RISCVOp, ImmLeaf<XLenVT, [{
int64_t Imm;
if (!MCOp.evaluateAsConstantImm(Imm))
return false;
+ if (Imm <= 0)
+ return false;
if (STI.getTargetTriple().isArch64Bit())
- return isUInt<6>(Imm) && (Imm != 0);
- return isUInt<5>(Imm) && (Imm != 0);
+ return Imm <= 63;
+ return Imm <= 31;
}];
}
@@ -70,9 +70,8 @@ def CLUIImmAsmOperand : AsmOperandClass {
// bit 17. Therefore, this 6-bit immediate can represent values in the ranges
// [1, 31] and [0xfffe0, 0xfffff].
def c_lui_imm : RISCVOp,
- ImmLeaf<XLenVT, [{return (Imm != 0) &&
- (isUInt<5>(Imm) ||
- (Imm >= 0xfffe0 && Imm <= 0xfffff));}]> {
+ ImmLeaf<XLenVT, [{return (Imm >= 1 && Imm <= 31) ||
+ (Imm >= 0xfffe0 && Imm <= 0xfffff);}]> {
let ParserMatchClass = CLUIImmAsmOperand;
let EncoderMethod = "getImmOpValue";
let DecoderMethod = "decodeCLUIImmOperand";
@@ -81,8 +80,8 @@ def c_lui_imm : RISCVOp,
int64_t Imm;
if (!MCOp.evaluateAsConstantImm(Imm))
return false;
- return (Imm != 0) && (isUInt<5>(Imm) ||
- (Imm >= 0xfffe0 && Imm <= 0xfffff));
+ return (Imm >= 1 && Imm <= 31) ||
+ (Imm >= 0xfffe0 && Imm <= 0xfffff);
}];
}
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoV.td b/llvm/lib/Target/RISCV/RISCVInstrInfoV.td
index f46455a9acedf..594a75a4746d4 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoV.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoV.td
@@ -79,19 +79,19 @@ def simm5 : RISCVSImmLeafOp<5> {
}
def simm5_plus1 : RISCVOp, ImmLeaf<XLenVT,
- [{return (isInt<5>(Imm) && Imm != -16) || Imm == 16;}]> {
+ [{return Imm >= -15 && Imm <= 16;}]> {
let ParserMatchClass = SImmAsmOperand<5, "Plus1">;
let OperandType = "OPERAND_SIMM5_PLUS1";
let MCOperandPredicate = [{
int64_t Imm;
if (MCOp.evaluateAsConstantImm(Imm))
- return (isInt<5>(Imm) && Imm != -16) || Imm == 16;
+ return Imm >= -15 && Imm <= 16;
return MCOp.isBareSymbolRef();
}];
}
def simm5_plus1_nonzero : ImmLeaf<XLenVT,
- [{return Imm != 0 && ((isInt<5>(Imm) && Imm != -16) || Imm == 16);}]>;
+ [{return Imm != 0 && Imm >= -15 && Imm <= 16;}]>;
//===----------------------------------------------------------------------===//
// Scheduling definitions.
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td b/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
index 13ceead2d28b4..3508224c60d7d 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
@@ -32,7 +32,7 @@ def qc_insb : RVSDNode<"QC_INSB", SDTypeProfile<1, 4, [SDTCisSameAs<0, 1>,
def qc_e_li : RVSDNode<"QC_E_LI", SDTIntUnaryOp>;
def uimm5nonzero : RISCVOp<XLenVT>,
- ImmLeaf<XLenVT, [{return (Imm != 0) && isUInt<5>(Imm);}]> {
+ ImmLeaf<XLenVT, [{return Imm >= 1 && Imm <= 31;}]> {
let ParserMatchClass = UImmAsmOperand<5, "NonZero">;
let DecoderMethod = "decodeUImmNonZeroOperand<5>";
let OperandType = "OPERAND_UIMM5_NONZERO";
@@ -40,20 +40,20 @@ def uimm5nonzero : RISCVOp<XLenVT>,
int64_t Imm;
if (!MCOp.evaluateAsConstantImm(Imm))
return false;
- return (Imm != 0) && isUInt<5>(Imm);;
+ return Imm >= 1 && Imm <= 31;
}];
}
-def tuimm5nonzero : TImmLeaf<XLenVT, [{return (Imm != 0) && isUInt<5>(Imm);}]>;
+def tuimm5nonzero : TImmLeaf<XLenVT, [{return Imm >= 1 && Imm <= 31;}]>;
def uimm5gt3 : RISCVOp<XLenVT>, ImmLeaf<XLenVT,
- [{return (Imm > 3) && isUInt<5>(Imm);}]> {
+ [{return Imm >= 4 && Imm <= 31;}]> {
let ParserMatchClass = UImmAsmOperand<5, "GT3">;
let DecoderMethod = "decodeUImmOperandGE<5, 4>";
let OperandType = "OPERAND_UIMM5_GT3";
}
-def tuimm5gt3 : TImmLeaf<XLenVT, [{return (Imm > 3) && isUInt<5>(Imm);}]>;
+def tuimm5gt3 : TImmLeaf<XLenVT, [{return Imm >= 4 && Imm <= 31;}]>;
def UImm5Plus1AsmOperand : AsmOperandClass {
let Name = "UImm5Plus1";
@@ -62,7 +62,7 @@ def UImm5Plus1AsmOperand : AsmOperandClass {
}
def uimm5_plus1 : RISCVOp, ImmLeaf<XLenVT,
- [{return (isUInt<5>(Imm) && (Imm != 0)) || (Imm == 32);}]> {
+ [{return Imm >= 1 && Imm <= 32;}]> {
let ParserMatchClass = UImm5Plus1AsmOperand;
let EncoderMethod = "getImmOpValueMinus1";
let DecoderMethod = "decodeUImmPlus1Operand<5>";
@@ -71,12 +71,12 @@ def uimm5_plus1 : RISCVOp, ImmLeaf<XLenVT,
int64_t Imm;
if (!MCOp.evaluateAsConstantImm(Imm))
return false;
- return (isUInt<5>(Imm) && (Imm != 0)) || (Imm == 32);
+ return Imm >= 1 && Imm <= 32;
}];
}
def uimm5ge6_plus1 : RISCVOp<XLenVT>, ImmLeaf<XLenVT,
- [{return (Imm >= 6) && (isUInt<5>(Imm) || (Imm == 32));}]> {
+ [{return Imm >= 6 && Imm <= 32;}]> {
let ParserMatchClass = UImmAsmOperand<5, "GE6Plus1">;
let EncoderMethod = "getImmOpValueMinus1";
let DecoderMethod = "decodeUImmPlus1OperandGE<5,6>";
@@ -85,7 +85,7 @@ def uimm5ge6_plus1 : RISCVOp<XLenVT>, ImmLeaf<XLenVT,
int64_t Imm;
if (!MCOp.evaluateAsConstantImm(Imm))
return false;
- return (Imm >= 6) && (isUInt<5>(Imm) || (Imm == 32));
+ return Imm >= 6 && Imm <= 32;
}];
}
@@ -129,7 +129,7 @@ def uimm14lsb00 : RISCVOp,
}
def uimm16nonzero : RISCVOp<XLenVT>,
- ImmLeaf<XLenVT, [{return (Imm != 0) && isUInt<16>(Imm);}]> {
+ ImmLeaf<XLenVT, [{return Imm >= 1 && Imm <= 65535;}]> {
let ParserMatchClass = UImmAsmOperand<16, "NonZero">;
let DecoderMethod = "decodeUImmNonZeroOperand<16>";
let OperandType = "OPERAND_UIMM16_NONZERO";
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZibi.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZibi.td
index 412bb08b00929..5caae543bf6ba 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoZibi.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZibi.td
@@ -12,7 +12,7 @@
// A 5-bit unsigned immediate representing 1-31 and -1. 00000 represents -1.
def imm5_zibi : RISCVOp<XLenVT>, ImmLeaf<XLenVT, [{
- return (Imm != 0 && isUInt<5>(Imm)) || Imm == -1;
+ return Imm != 0 && Imm >= -1 && Imm <= 31;
}]> {
let ParserMatchClass = ImmAsmOperand<"", 5, "Zibi">;
let EncoderMethod = "getImmOpValueZibi";
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| break; | ||
| case RISCVOp::OPERAND_UIMM5_NONZERO: | ||
| Ok = isUInt<5>(Imm) && (Imm != 0); | ||
| Ok = Imm >= 1 && Imm <= 31; |
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.
The original code was a direct translation of the name. So that seems a little clearer to me.
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.
Checking the lower bound once is easier to read for me.
But that's not a strong preference. How about I revert the isXXX && oneCond cases to leave only the plus1/minus1 changed?
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.
personally I also think that using isUInt instead of "magic" constants is easier to read.
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.
How about I revert the isXXX && oneCond cases to leave only the plus1/minus1 changed?
That works for me.
topperc
left a comment
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.
LGTM
| break; | ||
| case RISCVOp::OPERAND_UIMM5_PLUS1: | ||
| Ok = (isUInt<5>(Imm) && (Imm != 0)) || (Imm == 32); | ||
| Ok = Imm >= 1 && Imm <= 32; |
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.
Not sure if that is any better but we couldn't we also do isUInt<5>(Imm - 1) to avoid using these constants?
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.
Either way this is an improvement so LGTM.
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.
Not sure if that is any better but we couldn't we also do isUInt<5>(Imm - 1) to avoid using these constants?
You have to do Imm != INT64_MIN && isUInt<5>(Imm - 1) or isUInt<5>((uint64_t)Imm - 1) to avoid UB from signed integer wrapping on the Imm - 1.
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.
Ah yes I guess you could cast to unsigned but that negates the clarity benefit.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/34324 Here is the relevant piece of the build log for the reference |
No description provided.