[RISCV] Use decodeUImmLog2XLenNonZeroOperand in decodeRVCInstrRdRs1UImm. NFC #133759
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
…mm. NFC decodeUImmLog2XLenNonZeroOperand already contains the uimm5 check for RV32 so we can reuse it. This makes C_SLLI_HINT code more similar to the tblgen code for C_SLLI.
|
@llvm/pr-subscribers-backend-risc-v Author: Craig Topper (topperc) ChangesdecodeUImmLog2XLenNonZeroOperand already contains the uimm5 check for RV32 so we can reuse it. This makes C_SLLI_HINT code more similar to the tblgen code for C_SLLI. Full diff: https://github.com/llvm/llvm-project/pull/133759.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
index b22a4a7246c23..cda34ac01d7c0 100644
--- a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
+++ b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
@@ -488,9 +488,10 @@ static DecodeStatus decodeRVCInstrRdSImm(MCInst &Inst, uint32_t Insn,
uint64_t Address,
const MCDisassembler *Decoder);
-static DecodeStatus decodeRVCInstrRdRs1UImm(MCInst &Inst, uint32_t Insn,
- uint64_t Address,
- const MCDisassembler *Decoder);
+static DecodeStatus
+decodeRVCInstrRdRs1UImmLog2XLenNonZero(MCInst &Inst, uint32_t Insn,
+ uint64_t Address,
+ const MCDisassembler *Decoder);
static DecodeStatus decodeRVCInstrRdRs2(MCInst &Inst, uint32_t Insn,
uint64_t Address,
@@ -553,21 +554,16 @@ static DecodeStatus decodeRVCInstrRdSImm(MCInst &Inst, uint32_t Insn,
return MCDisassembler::Success;
}
-static DecodeStatus decodeRVCInstrRdRs1UImm(MCInst &Inst, uint32_t Insn,
- uint64_t Address,
- const MCDisassembler *Decoder) {
+static DecodeStatus
+decodeRVCInstrRdRs1UImmLog2XLenNonZero(MCInst &Inst, uint32_t Insn,
+ uint64_t Address,
+ const MCDisassembler *Decoder) {
Inst.addOperand(MCOperand::createReg(RISCV::X0));
Inst.addOperand(Inst.getOperand(0));
- uint32_t UImm6 = fieldFromInstruction(Insn, 12, 1) << 5;
- // On RV32C, uimm[5]=1 is reserved for custom extensions.
- if (UImm6 != 0 && Decoder->getSubtargetInfo().hasFeature(RISCV::Feature32Bit))
- return MCDisassembler::Fail;
- UImm6 |= fieldFromInstruction(Insn, 2, 5);
- [[maybe_unused]] DecodeStatus Result =
- decodeUImmOperand<6>(Inst, UImm6, Address, Decoder);
- assert(Result == MCDisassembler::Success && "Invalid immediate");
- return MCDisassembler::Success;
+ uint32_t UImm6 =
+ fieldFromInstruction(Insn, 12, 1) << 5 | fieldFromInstruction(Insn, 2, 5);
+ return decodeUImmLog2XLenNonZeroOperand(Inst, UImm6, Address, Decoder);
}
static DecodeStatus decodeRVCInstrRdRs2(MCInst &Inst, uint32_t Insn,
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoC.td b/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
index 199d056986dc2..eafd2844a691c 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
@@ -661,7 +661,7 @@ def C_SLLI_HINT : RVInst16CI<0b000, 0b10, (outs GPRX0:$rd_wb),
Sched<[WriteShiftImm, ReadShiftImm]> {
let Constraints = "$rd = $rd_wb";
let Inst{11-7} = 0;
- let DecoderMethod = "decodeRVCInstrRdRs1UImm";
+ let DecoderMethod = "decodeRVCInstrRdRs1UImmLog2XLenNonZero";
}
def C_SLLI64_HINT : RVInst16CI<0b000, 0b10, (outs GPR:$rd_wb), (ins GPR:$rd),
|
lenary
reviewed
Mar 31, 2025
| @@ -661,7 +661,7 @@ def C_SLLI_HINT : RVInst16CI<0b000, 0b10, (outs GPRX0:$rd_wb), | |||
| Sched<[WriteShiftImm, ReadShiftImm]> { | |||
| let Constraints = "$rd = $rd_wb"; | |||
| let Inst{11-7} = 0; | |||
| let DecoderMethod = "decodeRVCInstrRdRs1UImm"; | |||
| let DecoderMethod = "decodeRVCInstrRdRs1UImmLog2XLenNonZero"; | |||
There was a problem hiding this comment.
I think there are two things going on here:
c.slliis a hint when rd=0 (which I think this patch is refactoring, maybe?)c.slliis a hint when shamt=0
For the latter case, do we have a different Inst defintion?
If not, then isn't this wrong, because we should allow shamt=0 when decoding this instruction, which I think is now prevented? I'm not entirely sure, honestly.
There was a problem hiding this comment.
Quick note: these two constraints are given in the other order in my copy of the unprivileged spec:
For RV32C and RV64C, the shift amount must be non-zero; the code points with shamt=0 are HINTs. For all base ISAs, the code points with rd=x0 are HINTs, except those with shamt[5]=1 in RV32C.
There was a problem hiding this comment.
The instruction definition I'm changing is
def C_SLLI_HINT : RVInst16CI<0b000, 0b10, (outs GPRX0:$rd_wb),
(ins GPRX0:$rd, uimmlog2xlennonzero:$imm),
"c.slli", "$rd, $imm">,
Sched<[WriteShiftImm, ReadShiftImm]> {
let Constraints = "$rd = $rd_wb";
let Inst{11-7} = 0;
let DecoderMethod = "decodeRVCInstrRdRs1UImm";
}
which doesn't allow immediate of 0 in the parser due to uimmlog2xlennonzero.
I think the hint with imm==0 is c.slli64
def C_SLLI64_HINT : RVInst16CI<0b000, 0b10, (outs GPR:$rd_wb), (ins GPR:$rd),
"c.slli64", "$rd">,
Sched<[WriteShiftImm, ReadShiftImm]> {
let Constraints = "$rd = $rd_wb";
let imm = 0;
}
There was a problem hiding this comment.
But you're right this patch might not be NFC unless the c.slli64 hint was getting decoded first.
There was a problem hiding this comment.
I've verified that c.slli64 already head priority so the check for 0 shamt added here isn't a functional change. Exhaustive tests added in #133820
lenary
approved these changes
Apr 1, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/169/builds/10004 Here is the relevant piece of the build log for the reference |
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.
decodeUImmLog2XLenNonZeroOperand already contains the uimm5 check for RV32 so we can reuse it. This makes C_SLLI_HINT code more similar to the tblgen code for C_SLLI.