Skip to content
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.

Sign up for GitHub

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

[RISCV] Use decodeUImmLog2XLenNonZeroOperand in decodeRVCInstrRdRs1UImm. NFC #133759

Merged
merged 1 commit into from
Apr 1, 2025
Merged

[RISCV] Use decodeUImmLog2XLenNonZeroOperand in decodeRVCInstrRdRs1UImm. NFC #133759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 11 additions & 15 deletions llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/RISCV/RISCVInstrInfoC.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are two things going on here:

  • c.slli is a hint when rd=0 (which I think this patch is refactoring, maybe?)
  • c.slli is 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;                                                                   
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you're right this patch might not be NFC unless the c.slli64 hint was getting decoded first.

Copy link
Collaborator Author

@topperc topperc Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

def C_SLLI64_HINT : RVInst16CI<0b000, 0b10, (outs GPR:$rd_wb), (ins GPR:$rd),
Expand Down