[RISCV] Use named sub-operands to simplify encoding/decoding for CoreV Reg-Reg instructions. #133181
Conversation
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
…V Reg-Reg instructions. We can name the sub-operands using a DAG in the 'ins'. This allows those names to be matched to the encoding fields. This removes the need for a custom encoder/decoder that treats the 2 sub-operands as a single 10-bit value. While doing this, I noticed the base and offset names in the MIOperandInfo were swapped relative to how the operands are parsed and printed. Assuming that I've correclty understood the parsing/print format as "offset(base)".
topperc
requested review from
ChunyuLiao,
asb,
lenary,
melonedo,
mshockwave and
wangpc-pp
|
@llvm/pr-subscribers-backend-risc-v Author: Craig Topper (topperc) ChangesWe can name the sub-operands using a DAG in the 'ins'. This allows those names to be matched to the encoding fields. This removes the need for a custom encoder/decoder that treats the 2 sub-operands as a single 10-bit value. While doing this, I noticed the base and offset names in the MIOperandInfo were swapped relative to how the operands are parsed and printed. Assuming that I've correclty understood the parsing/print format as "offset(base)". Full diff: https://github.com/llvm/llvm-project/pull/133181.diff 3 Files Affected:
diff --git a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
index 93cbf662bfa32..46b01417a1ea1 100644
--- a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
+++ b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
@@ -507,9 +507,6 @@ static DecodeStatus decodeXTHeadMemPair(MCInst &Inst, uint32_t Insn,
static DecodeStatus decodeZcmpRlist(MCInst &Inst, uint32_t Imm,
uint64_t Address, const void *Decoder);
-static DecodeStatus decodeRegReg(MCInst &Inst, uint32_t Insn, uint64_t Address,
- const MCDisassembler *Decoder);
-
static DecodeStatus decodeZcmpSpimm(MCInst &Inst, uint32_t Imm,
uint64_t Address, const void *Decoder);
@@ -621,15 +618,6 @@ static DecodeStatus decodeZcmpRlist(MCInst &Inst, uint32_t Imm,
return MCDisassembler::Success;
}
-static DecodeStatus decodeRegReg(MCInst &Inst, uint32_t Insn, uint64_t Address,
- const MCDisassembler *Decoder) {
- uint32_t Rs1 = fieldFromInstruction(Insn, 0, 5);
- uint32_t Rs2 = fieldFromInstruction(Insn, 5, 5);
- DecodeGPRRegisterClass(Inst, Rs1, Address, Decoder);
- DecodeGPRRegisterClass(Inst, Rs2, Address, Decoder);
- return MCDisassembler::Success;
-}
-
static DecodeStatus decodeZcmpSpimm(MCInst &Inst, uint32_t Imm,
uint64_t Address, const void *Decoder) {
Inst.addOperand(MCOperand::createImm(Imm));
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
index e589e6171d010..77d33f2e06871 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
@@ -103,10 +103,6 @@ class RISCVMCCodeEmitter : public MCCodeEmitter {
unsigned getRlistOpValue(const MCInst &MI, unsigned OpNo,
SmallVectorImpl<MCFixup> &Fixups,
const MCSubtargetInfo &STI) const;
-
- unsigned getRegReg(const MCInst &MI, unsigned OpNo,
- SmallVectorImpl<MCFixup> &Fixups,
- const MCSubtargetInfo &STI) const;
};
} // end anonymous namespace
@@ -621,17 +617,4 @@ unsigned RISCVMCCodeEmitter::getRlistOpValue(const MCInst &MI, unsigned OpNo,
return Imm;
}
-unsigned RISCVMCCodeEmitter::getRegReg(const MCInst &MI, unsigned OpNo,
- SmallVectorImpl<MCFixup> &Fixups,
- const MCSubtargetInfo &STI) const {
- const MCOperand &MO = MI.getOperand(OpNo);
- const MCOperand &MO1 = MI.getOperand(OpNo + 1);
- assert(MO.isReg() && MO1.isReg() && "Expected registers.");
-
- unsigned Op = Ctx.getRegisterInfo()->getEncodingValue(MO.getReg());
- unsigned Op1 = Ctx.getRegisterInfo()->getEncodingValue(MO1.getReg());
-
- return Op | Op1 << 5;
-}
-
#include "RISCVGenMCCodeEmitter.inc"
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td b/llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
index e18a61ad79278..9c59da3ee94b6 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
@@ -24,10 +24,8 @@ def CVrrAsmOperand : AsmOperandClass {
def CVrr : Operand<i32>,
ComplexPattern<i32, 2, "SelectAddrRegReg",[]> {
let ParserMatchClass = CVrrAsmOperand;
- let EncoderMethod = "getRegReg";
- let DecoderMethod = "decodeRegReg";
let PrintMethod = "printRegReg";
- let MIOperandInfo = (ops GPR:$base, GPR:$offset);
+ let MIOperandInfo = (ops GPR:$offset, GPR:$base);
}
def cv_tuimm2 : TImmLeaf<XLenVT, [{return isUInt<2>(Imm);}]>;
@@ -288,17 +286,9 @@ class CVLoad_rr_inc<bits<7> funct7, bits<3> funct3, string opcodestr>
}
class CVLoad_rr<bits<7> funct7, bits<3> funct3, string opcodestr>
- : RVInstR<funct7, funct3, OPC_CUSTOM_1, (outs GPR:$rd), (ins CVrr:$cvrr),
- opcodestr, "$rd, $cvrr"> {
- bits<5> rd;
- bits<10> cvrr;
-
- let Inst{31-25} = funct7;
- let Inst{24-20} = cvrr{4-0};
- let Inst{19-15} = cvrr{9-5};
- let Inst{14-12} = funct3;
- let Inst{11-7} = rd;
-}
+ : RVInstR<funct7, funct3, OPC_CUSTOM_1, (outs GPR:$rd),
+ (ins (CVrr $rs2, $rs1):$cvrr),
+ opcodestr, "$rd, $cvrr">;
} // hasSideEffects = 0, mayLoad = 1, mayStore = 0
let hasSideEffects = 0, mayLoad = 0, mayStore = 1 in {
@@ -327,16 +317,17 @@ class CVStore_rr_inc<bits<3> funct3, bits<7> funct7, string opcodestr>
class CVStore_rr<bits<3> funct3, bits<7> funct7, string opcodestr>
- : RVInst<(outs), (ins GPR:$rs2, CVrr:$cvrr), opcodestr, "$rs2, $cvrr", [],
- InstFormatOther> {
+ : RVInst<(outs), (ins GPR:$rs2, (CVrr $rs3, $rs1):$cvrr), opcodestr,
+ "$rs2, $cvrr", [], InstFormatOther> {
+ bits<5> rs1;
bits<5> rs2;
- bits<10> cvrr;
+ bits<5> rs3;
let Inst{31-25} = funct7;
let Inst{24-20} = rs2;
- let Inst{19-15} = cvrr{9-5};
+ let Inst{19-15} = rs1;
let Inst{14-12} = funct3;
- let Inst{11-7} = cvrr{4-0};
+ let Inst{11-7} = rs3;
let Inst{6-0} = OPC_CUSTOM_1.Value;
}
} // hasSideEffects = 0, mayLoad = 0, mayStore = 1
|
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/14993 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/198/builds/3197 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
4 participants
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.
We can name the sub-operands using a DAG in the 'ins'. This allows those names to be matched to the encoding fields. This removes the need for a custom encoder/decoder that treats the 2 sub-operands as a single 10-bit value.
While doing this, I noticed the base and offset names in the MIOperandInfo were swapped relative to how the operands are parsed and printed. Assuming that I've correclty understood the parsing/print format as "offset(base)".