RISCVAsmParser: Don't treat operands with relocation specifier as parse-time constants #133377
Conversation
This file contains 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
Created using spr 1.3.5-bogner
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-mc Author: Fangrui Song (MaskRay) ChangesAn immediate operand is encoded as an Evaluating RISCVMCExpr during parsing is unnecessary. For example, This evaluation introduces unnecessary complexity. For instance, parser This PR eliminates the parse-time evaluation of I will update the remaining MC/RISCV tests if this PR looks good. Patch is 32.01 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/133377.diff 8 Files Affected:
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 3c225fb38cb19..a6882fb86ecc7 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -524,15 +524,8 @@ struct RISCVOperand final : public MCParsedAsmOperand {
bool isGPRAsFPR32() const { return isGPRF32() && Reg.IsGPRAsFPR; }
bool isGPRPairAsFPR64() const { return isGPRPair() && Reg.IsGPRAsFPR; }
- static bool evaluateConstantImm(const MCExpr *Expr, int64_t &Imm,
- RISCVMCExpr::Specifier &VK) {
- if (auto *RE = dyn_cast<RISCVMCExpr>(Expr)) {
- VK = RE->getSpecifier();
- return RE->evaluateAsConstant(Imm);
- }
-
+ static bool evaluateConstantImm(const MCExpr *Expr, int64_t &Imm) {
if (auto CE = dyn_cast<MCConstantExpr>(Expr)) {
- VK = RISCVMCExpr::VK_None;
Imm = CE->getValue();
return true;
}
@@ -547,7 +540,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
bool IsValid;
if (!IsConstantImm)
IsValid = RISCVAsmParser::classifySymbolRef(getImm(), VK);
@@ -562,7 +555,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
int64_t Imm;
RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
// Must be of 'immediate' type but not a constant.
- if (!isImm() || evaluateConstantImm(getImm(), Imm, VK))
+ if (!isImm() || evaluateConstantImm(getImm(), Imm))
return false;
return RISCVAsmParser::classifySymbolRef(getImm(), VK) &&
VK == RISCVMCExpr::VK_None;
@@ -572,7 +565,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
int64_t Imm;
RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
// Must be of 'immediate' type but not a constant.
- if (!isImm() || evaluateConstantImm(getImm(), Imm, VK))
+ if (!isImm() || evaluateConstantImm(getImm(), Imm))
return false;
return RISCVAsmParser::classifySymbolRef(getImm(), VK) &&
(VK == RISCVMCExpr::VK_CALL || VK == RISCVMCExpr::VK_CALL_PLT);
@@ -582,7 +575,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
int64_t Imm;
RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
// Must be of 'immediate' type but not a constant.
- if (!isImm() || evaluateConstantImm(getImm(), Imm, VK))
+ if (!isImm() || evaluateConstantImm(getImm(), Imm))
return false;
return RISCVAsmParser::classifySymbolRef(getImm(), VK) &&
VK == RISCVMCExpr::VK_CALL;
@@ -592,7 +585,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
int64_t Imm;
RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
// Must be of 'immediate' type but not a constant.
- if (!isImm() || evaluateConstantImm(getImm(), Imm, VK))
+ if (!isImm() || evaluateConstantImm(getImm(), Imm))
return false;
return RISCVAsmParser::classifySymbolRef(getImm(), VK) &&
VK == RISCVMCExpr::VK_TPREL_ADD;
@@ -602,7 +595,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
int64_t Imm;
RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
// Must be of 'immediate' type but not a constant.
- if (!isImm() || evaluateConstantImm(getImm(), Imm, VK))
+ if (!isImm() || evaluateConstantImm(getImm(), Imm))
return false;
return RISCVAsmParser::classifySymbolRef(getImm(), VK) &&
VK == RISCVMCExpr::VK_TLSDESC_CALL;
@@ -612,11 +605,10 @@ struct RISCVOperand final : public MCParsedAsmOperand {
bool isVTypeImm(unsigned N) const {
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isUIntN(N, Imm) && VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isUIntN(N, Imm);
}
// If the last operand of the vsetvli/vsetvli instruction is a constant
@@ -659,7 +651,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
if (VK == RISCVMCExpr::VK_LO || VK == RISCVMCExpr::VK_PCREL_LO ||
VK == RISCVMCExpr::VK_TLSDESC_LOAD_LO ||
VK == RISCVMCExpr::VK_TLSDESC_ADD_LO)
@@ -676,12 +668,11 @@ struct RISCVOperand final : public MCParsedAsmOperand {
bool isImmXLenLI_Restricted() const {
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
// 'la imm' supports constant immediates only.
- return IsConstantImm && (VK == RISCVMCExpr::VK_None) &&
+ return IsConstantImm &&
(isRV64Imm() || (isInt<32>(Imm) || isUInt<32>(Imm)));
}
@@ -690,7 +681,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- if (!evaluateConstantImm(getImm(), Imm, VK) || VK != RISCVMCExpr::VK_None)
+ if (!evaluateConstantImm(getImm(), Imm) || VK != RISCVMCExpr::VK_None)
return false;
return (isRV64Imm() && isUInt<6>(Imm)) || isUInt<5>(Imm);
}
@@ -700,7 +691,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- if (!evaluateConstantImm(getImm(), Imm, VK) || VK != RISCVMCExpr::VK_None)
+ if (!evaluateConstantImm(getImm(), Imm) || VK != RISCVMCExpr::VK_None)
return false;
if (Imm == 0)
return false;
@@ -712,18 +703,17 @@ struct RISCVOperand final : public MCParsedAsmOperand {
RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- if (!evaluateConstantImm(getImm(), Imm, VK) || VK != RISCVMCExpr::VK_None)
+ if (!evaluateConstantImm(getImm(), Imm) || VK != RISCVMCExpr::VK_None)
return false;
return (isRV64Imm() && isUInt<5>(Imm)) || isUInt<4>(Imm);
}
template <unsigned N> bool IsUImm() const {
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isUInt<N>(Imm) && VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isUInt<N>(Imm);
}
bool isUImm1() const { return IsUImm<1>(); }
@@ -746,144 +736,116 @@ struct RISCVOperand final : public MCParsedAsmOperand {
if (!isImm())
return false;
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isUInt<5>(Imm) && (Imm != 0) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isUInt<5>(Imm) && (Imm != 0);
}
bool isUImm5GT3() const {
if (!isImm())
return false;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
int64_t Imm;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isUInt<5>(Imm) && (Imm > 3) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isUInt<5>(Imm) && (Imm > 3);
}
bool isUImm5Plus1() const {
if (!isImm())
return false;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
int64_t Imm;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && ((isUInt<5>(Imm) && (Imm != 0)) || (Imm == 32)) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && ((isUInt<5>(Imm) && (Imm != 0)) || (Imm == 32));
}
bool isUImm5GE6Plus1() const {
if (!isImm())
return false;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
int64_t Imm;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && ((isUInt<5>(Imm) && (Imm >= 6)) || (Imm == 32)) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && ((isUInt<5>(Imm) && (Imm >= 6)) || (Imm == 32));
}
bool isUImm5Slist() const {
if (!isImm())
return false;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
int64_t Imm;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
return IsConstantImm &&
((Imm == 0) || (Imm == 1) || (Imm == 2) || (Imm == 4) ||
- (Imm == 8) || (Imm == 16) || (Imm == 15) || (Imm == 31)) &&
- VK == RISCVMCExpr::VK_None;
+ (Imm == 8) || (Imm == 16) || (Imm == 15) || (Imm == 31));
}
bool isUImm8GE32() const {
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isUInt<8>(Imm) && Imm >= 32 &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isUInt<8>(Imm) && Imm >= 32;
}
bool isRnumArg() const {
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && Imm >= INT64_C(0) && Imm <= INT64_C(10) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && Imm >= INT64_C(0) && Imm <= INT64_C(10);
}
bool isRnumArg_0_7() const {
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && Imm >= INT64_C(0) && Imm <= INT64_C(7) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && Imm >= INT64_C(0) && Imm <= INT64_C(7);
}
bool isRnumArg_1_10() const {
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && Imm >= INT64_C(1) && Imm <= INT64_C(10) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && Imm >= INT64_C(1) && Imm <= INT64_C(10);
}
bool isRnumArg_2_14() const {
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
if (!isImm())
return false;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && Imm >= INT64_C(2) && Imm <= INT64_C(14) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && Imm >= INT64_C(2) && Imm <= INT64_C(14);
}
bool isSImm5() const {
if (!isImm())
return false;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
int64_t Imm;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isInt<5>(fixImmediateForRV32(Imm, isRV64Imm())) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isInt<5>(fixImmediateForRV32(Imm, isRV64Imm()));
}
bool isSImm5NonZero() const {
if (!isImm())
return false;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
int64_t Imm;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
return IsConstantImm && Imm != 0 &&
- isInt<5>(fixImmediateForRV32(Imm, isRV64Imm())) &&
- VK == RISCVMCExpr::VK_None;
+ isInt<5>(fixImmediateForRV32(Imm, isRV64Imm()));
}
bool isSImm6() const {
if (!isImm())
return false;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
int64_t Imm;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isInt<6>(fixImmediateForRV32(Imm, isRV64Imm())) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isInt<6>(fixImmediateForRV32(Imm, isRV64Imm()));
}
bool isSImm6NonZero() const {
if (!isImm())
return false;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
int64_t Imm;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
return IsConstantImm && Imm != 0 &&
- isInt<6>(fixImmediateForRV32(Imm, isRV64Imm())) &&
- VK == RISCVMCExpr::VK_None;
+ isInt<6>(fixImmediateForRV32(Imm, isRV64Imm()));
}
bool isCLUIImm() const {
@@ -891,7 +853,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
return false;
int64_t Imm;
RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
return IsConstantImm && (Imm != 0) &&
(isUInt<5>(Imm) || (Imm >= 0xfffe0 && Imm <= 0xfffff)) &&
VK == RISCVMCExpr::VK_None;
@@ -901,70 +863,56 @@ struct RISCVOperand final : public MCParsedAsmOperand {
if (!isImm())
return false;
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isShiftedUInt<1, 1>(Imm) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isShiftedUInt<1, 1>(Imm);
}
bool isUImm5Lsb0() const {
if (!isImm())
return false;
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isShiftedUInt<4, 1>(Imm) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isShiftedUInt<4, 1>(Imm);
}
bool isUImm6Lsb0() const {
if (!isImm())
return false;
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isShiftedUInt<5, 1>(Imm) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isShiftedUInt<5, 1>(Imm);
}
bool isUImm7Lsb00() const {
if (!isImm())
return false;
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isShiftedUInt<5, 2>(Imm) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isShiftedUInt<5, 2>(Imm);
}
bool isUImm7Lsb000() const {
if (!isImm())
return false;
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isShiftedUInt<4, 3>(Imm) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isShiftedUInt<4, 3>(Imm);
}
bool isUImm8Lsb00() const {
if (!isImm())
return false;
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isShiftedUInt<6, 2>(Imm) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isShiftedUInt<6, 2>(Imm);
}
bool isUImm8Lsb000() const {
if (!isImm())
return false;
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isShiftedUInt<5, 3>(Imm) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isShiftedUInt<5, 3>(Imm);
}
bool isSImm9Lsb0() const { return isBareSimmNLsb0<9>(); }
@@ -973,20 +921,16 @@ struct RISCVOperand final : public MCParsedAsmOperand {
if (!isImm())
return false;
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isShiftedUInt<6, 3>(Imm) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isShiftedUInt<6, 3>(Imm);
}
bool isUImm10Lsb00NonZero() const {
if (!isImm())
return false;
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isShiftedUInt<8, 2>(Imm) && (Imm != 0) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isShiftedUInt<8, 2>(Imm) && (Imm != 0);
}
// If this a RV32 and the immediate is a uimm32, sign extend it to 32 bits.
@@ -1000,11 +944,9 @@ struct RISCVOperand final : public MCParsedAsmOperand {
bool isSImm11() const {
if (!isImm())
return false;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
int64_t Imm;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
- return IsConstantImm && isInt<11>(fixImmediateForRV32(Imm, isRV64Imm())) &&
- VK == RISCVMCExpr::VK_None;
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
+ return IsConstantImm && isInt<11>(fixImmediateForRV32(Imm, isRV64Imm()));
}
bool isSImm12() const {
@@ -1013,7 +955,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
bool IsValid;
if (!isImm())
return false;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
if (!IsConstantImm)
IsValid = RISCVAsmParser::classifySymbolRef(getImm(), VK);
else
@@ -1031,12 +973,10 @@ struct RISCVOperand final : public MCParsedAsmOperand {
bool isSImm12Lsb00000() const {
if (!isImm())
return false;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
int64_t Imm;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
return IsConstantImm &&
- isShiftedInt<7, 5>(fixImmediateForRV32(Imm, isRV64Imm())) &&
- VK == RISCVMCExpr::VK_None;
+ isShiftedInt<7, 5>(fixImmediateForRV32(Imm, isRV64Imm()));
}
bool isSImm13Lsb0() const { return isBareSimmNLsb0<13>(); }
@@ -1045,32 +985,26 @@ struct RISCVOperand final : public MCParsedAsmOperand {
if (!isImm())
return false;
int64_t Imm;
- RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None;
- bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK);
+ bool IsConstantImm = evaluateConstantImm(getImm(), Imm);
return IsConstantImm && (Imm != 0) &&
- isShiftedInt<6, 4>(fixImmediateForRV32(Imm, isRV64Imm())) &&
- VK == RISCVMCExpr::VK_None;
+ isShiftedInt<6, 4>(fixImmediateForRV32(Imm, isRV64Imm()));
}
bool isSImm16NonZero() const {
if (!isImm())
return false;
- RISCVMCExpr::Specifier VK = RIS...
[truncated]
|
lenary
approved these changes
Mar 28, 2025
MaskRay
added a commit
to MaskRay/llvm-project
that referenced
this pull request
Mar 28, 2025
…se-time constants An immediate operand is encoded as an `MCExpr`, with `RISCVMCExpr` specifying an operand that includes a relocation specifier. When https://reviews.llvm.org/D23568 added initial fixup and relocation support in 2017, it adapted code from `PPCMCExpr` (for `@l` `@ha`) to evaluate the `RISCVMCExpr` operand. (PPCAsmParser had considerable technical debt, though I’ve recently streamlined it somewhat, e.g. 8560da2) Evaluating RISCVMCExpr during parsing is unnecessary. For example, there's no need to treat `lui a0, %hi(2)` differently from `lui a0, %hi(foo)` when foo has not been defined yet. This evaluation introduces unnecessary complexity. For instance, parser functions require an extra check like `VK == RISCVMCExpr::VK_None`, as seen in these examples: ``` if (!evaluateConstantImm(getImm(), Imm, VK) || VK != RISCVMCExpr::VK_None) return IsConstantImm && isUInt<N>(Imm) && VK == RISCVMCExpr::VK_None; ``` This PR eliminates the parse-time evaluation of `RISCVMCExpr`, aligning it more closely with other targets. I will update the remaining MC/RISCV tests if this PR looks good. Pull Request: llvm#133377
Created using spr 1.3.5-bogner
|
Happy to defer to Sam's review of the patches, but I wanted to give an explicit LGTM on the direction and to say thank you for taking the time to look at how we clean up our MC code. Writing this evaluation logic was a useful exercise at the time, but I agree it's unnecessary complexity at this point. |
lenary
approved these changes
Mar 28, 2025
| @@ -650,7 +643,7 @@ struct RISCVOperand final : public MCParsedAsmOperand { | |||
| RISCVMCExpr::Specifier VK = RISCVMCExpr::VK_None; | |||
| if (!isImm()) | |||
| return false; | |||
| bool IsConstantImm = evaluateConstantImm(getImm(), Imm, VK); | |||
| bool IsConstantImm = evaluateConstantImm(getImm(), Imm); | |||
| if (VK == RISCVMCExpr::VK_LO || VK == RISCVMCExpr::VK_PCREL_LO || | |||
There was a problem hiding this comment.
Nit: This if is always false, and I think VK is always none by the time you get to (new) line 654.
Thanks for the LGTM and the kind words! I have recently spent quite some time on AsmParser across various targets and realized what we could simplify... |
Created using spr 1.3.5-bogner
Created using spr 1.3.5-bogner
|
I'm happy with the code but LLD seems pretty unhappy. Is the problem in the LLD tests one that GNU LD will also have (doesn't like HI/LO relocations against local symbols) |
Looks like LLD should not give an error for the test ELF/riscv-relax-hi20-lo12-pie.s . I'll fix it separately. In addition, this exposes some assembler differences: // R_RISCV_HI20 after this patch
abs = 0x100000
lui x0, %hi(abs)
// R_RISCV_HI20, w/o or w/ this patch; GNU assembler doesn't emit a relocation
lui x0, %hi(abs)
abs = 0x100000
// gas suppresses the relocation even if abs is weak.PowerPC suppresses R_PPC64_LO16 relocation at MCAssembler::evaluateFixup time. RISC-V can't due to relaxation. I prefer keeping things simple and avoiding the addition of assembler code to suppress the relocation. |
|
What's one more relocation between friends :) |
MaskRay
deleted the
users/MaskRay/spr/riscvasmparser-dont-treat-operands-with-relocation-specifier-as-parse-time-constants
branch
llvm-sync bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Mar 29, 2025
…fier as parse-time constants An immediate operand is encoded as an `MCExpr`, with `RISCVMCExpr` specifying an operand that includes a relocation specifier. When https://reviews.llvm.org/D23568 added initial fixup and relocation support in 2017, it adapted code from `PPCMCExpr` (for `@l` `@ha`) to evaluate the `RISCVMCExpr` operand. (PPCAsmParser had considerable technical debt, though I’ve recently streamlined it somewhat, e.g. 8560da2) Evaluating RISCVMCExpr during parsing is unnecessary. For example, there's no need to treat `lui a0, %hi(2)` differently from `lui a0, %hi(foo)` when foo has not been defined yet. This evaluation introduces unnecessary complexity. For instance, parser functions require an extra check like `VK == RISCVMCExpr::VK_None`, as seen in these examples: ``` if (!evaluateConstantImm(getImm(), Imm, VK) || VK != RISCVMCExpr::VK_None) return IsConstantImm && isUInt<N>(Imm) && VK == RISCVMCExpr::VK_None; ``` This PR eliminates the parse-time evaluation of `RISCVMCExpr`, aligning it more closely with other targets. --- `abs = 0x12345; lui t3, %hi(abs)` now generates R_RISCV_HI20/R_RISCV_RELAX with linker relaxation. (Tested by test/MC/RISCV/linker-relaxation.s) (Notably, since commit ba2de8f in lld/ELF, the linker can handle R_RISCV_HI relocations with a symbol index of 0 in -pie mode.) Pull Request: llvm/llvm-project#133377
topperc
added a commit
to topperc/llvm-project
that referenced
this pull request
Mar 30, 2025
Make isSImm12 look more like isUImm20LUI. Move variables closer to their use. Fold some function calls into if statements.
topperc
added a commit
that referenced
this pull request
Mar 31, 2025
SchrodingerZhu
pushed a commit
to SchrodingerZhu/llvm-project
that referenced
this pull request
Mar 31, 2025
Make isSImm12 look more like isUImm20LUI. Move variables closer to their use. Fold some function calls into if statements.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
An immediate operand is encoded as an
MCExpr, withRISCVMCExprspecifying an operand that includes a relocation specifier. When
https://reviews.llvm.org/D23568 added initial fixup and relocation
support in 2017, it adapted code from
PPCMCExpr(for@l@ha) toevaluate the
RISCVMCExproperand. (PPCAsmParser had considerabletechnical debt, though I’ve recently streamlined it somewhat, e.g.
8560da2)
Evaluating RISCVMCExpr during parsing is unnecessary. For example,
there's no need to treat
lui a0, %hi(2)differently fromlui a0, %hi(foo)when foo has not been defined yet.This evaluation introduces unnecessary complexity. For instance, parser
functions require an extra check like
VK == RISCVMCExpr::VK_None, asseen in these examples:
This PR eliminates the parse-time evaluation of
RISCVMCExpr, aligningit more closely with other targets.
abs = 0x12345; lui t3, %hi(abs)now generatesR_RISCV_HI20/R_RISCV_RELAX with linker relaxation.
(Tested by test/MC/RISCV/linker-relaxation.s)
(Notably, since commit ba2de8f in
lld/ELF, the linker can handle R_RISCV_HI relocations with a symbol
index of 0 in -pie mode.)