-
Notifications
You must be signed in to change notification settings - Fork 11.6k
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
[RISCV][NFC] Add generateMCInstSeq in RISCVMatInt #84462
[RISCV][NFC] Add generateMCInstSeq in RISCVMatInt #84462
Conversation
@llvm/pr-subscribers-backend-risc-v Author: Sacha Coppey (Zeavee) ChangesThis allows to avoid duplicating the code handling the instructions outputted by This will be used in #77337. Full diff: https://github.com/llvm/llvm-project/pull/84462.diff 3 Files Affected:
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index d83979a873f2a3..78a6dd210bbcb6 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -3081,34 +3081,11 @@ void RISCVAsmParser::emitToStreamer(MCStreamer &S, const MCInst &Inst) {
void RISCVAsmParser::emitLoadImm(MCRegister DestReg, int64_t Value,
MCStreamer &Out) {
- RISCVMatInt::InstSeq Seq = RISCVMatInt::generateInstSeq(Value, getSTI());
-
- MCRegister SrcReg = RISCV::X0;
- for (const RISCVMatInt::Inst &Inst : Seq) {
- switch (Inst.getOpndKind()) {
- case RISCVMatInt::Imm:
- emitToStreamer(Out,
- MCInstBuilder(Inst.getOpcode()).addReg(DestReg).addImm(Inst.getImm()));
- break;
- case RISCVMatInt::RegX0:
- emitToStreamer(
- Out, MCInstBuilder(Inst.getOpcode()).addReg(DestReg).addReg(SrcReg).addReg(
- RISCV::X0));
- break;
- case RISCVMatInt::RegReg:
- emitToStreamer(
- Out, MCInstBuilder(Inst.getOpcode()).addReg(DestReg).addReg(SrcReg).addReg(
- SrcReg));
- break;
- case RISCVMatInt::RegImm:
- emitToStreamer(
- Out, MCInstBuilder(Inst.getOpcode()).addReg(DestReg).addReg(SrcReg).addImm(
- Inst.getImm()));
- break;
- }
+ SmallVector<MCInst, 8> Seq =
+ RISCVMatInt::generateMCInstSeq(Value, getSTI(), DestReg);
- // Only the first instruction has X0 as its source.
- SrcReg = DestReg;
+ for (MCInst &Inst : Seq) {
+ emitToStreamer(Out, Inst);
}
}
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp
index 4358a5b878e631..8ebdcd577fab66 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp
@@ -9,6 +9,7 @@
#include "RISCVMatInt.h"
#include "MCTargetDesc/RISCVMCTargetDesc.h"
#include "llvm/ADT/APInt.h"
+#include "llvm/MC/MCInstBuilder.h"
#include "llvm/Support/MathExtras.h"
using namespace llvm;
@@ -436,6 +437,46 @@ InstSeq generateInstSeq(int64_t Val, const MCSubtargetInfo &STI) {
return Res;
}
+SmallVector<MCInst, 8>
+generateMCInstSeq(int64_t Val, const MCSubtargetInfo &STI, MCRegister DestReg) {
+ RISCVMatInt::InstSeq Seq = RISCVMatInt::generateInstSeq(Val, STI);
+
+ SmallVector<MCInst, 8> Instructions;
+
+ MCRegister SrcReg = RISCV::X0;
+ for (RISCVMatInt::Inst &Inst : Seq) {
+ switch (Inst.getOpndKind()) {
+ case RISCVMatInt::Imm:
+ Instructions.push_back(MCInstBuilder(Inst.getOpcode())
+ .addReg(DestReg)
+ .addImm(Inst.getImm()));
+ break;
+ case RISCVMatInt::RegX0:
+ Instructions.push_back(MCInstBuilder(Inst.getOpcode())
+ .addReg(DestReg)
+ .addReg(SrcReg)
+ .addReg(RISCV::X0));
+ break;
+ case RISCVMatInt::RegReg:
+ Instructions.push_back(MCInstBuilder(Inst.getOpcode())
+ .addReg(DestReg)
+ .addReg(SrcReg)
+ .addReg(SrcReg));
+ break;
+ case RISCVMatInt::RegImm:
+ Instructions.push_back(MCInstBuilder(Inst.getOpcode())
+ .addReg(DestReg)
+ .addReg(SrcReg)
+ .addImm(Inst.getImm()));
+ break;
+ }
+
+ // Only the first instruction has X0 as its source.
+ SrcReg = DestReg;
+ }
+ return Instructions;
+}
+
InstSeq generateTwoRegInstSeq(int64_t Val, const MCSubtargetInfo &STI,
unsigned &ShiftAmt, unsigned &AddOpc) {
int64_t LoVal = SignExtend64<32>(Val);
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.h
index 780f685463f300..70b1b9e492093f 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.h
@@ -10,6 +10,8 @@
#define LLVM_LIB_TARGET_RISCV_MCTARGETDESC_MATINT_H
#include "llvm/ADT/SmallVector.h"
+#include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCRegisterInfo.h"
#include "llvm/MC/MCSubtargetInfo.h"
#include <cstdint>
@@ -48,6 +50,10 @@ using InstSeq = SmallVector<Inst, 8>;
// instruction selection.
InstSeq generateInstSeq(int64_t Val, const MCSubtargetInfo &STI);
+// Helper to generate the generateInstSeq instruction sequence using MCInsts
+SmallVector<MCInst, 8>
+generateMCInstSeq(int64_t Val, const MCSubtargetInfo &STI, MCRegister DestReg);
+
// Helper to generate an instruction sequence that can materialize the given
// immediate value into a register using an additional temporary register. This
// handles cases where the constant can be generated by (ADD (SLLI X, C), X) or
|
Thanks, if this is just a refactoring can you also include NFC somewhere in the PR title? |
@@ -10,6 +10,8 @@ | |||
#define LLVM_LIB_TARGET_RISCV_MCTARGETDESC_MATINT_H | |||
|
|||
#include "llvm/ADT/SmallVector.h" | |||
#include "llvm/MC/MCInst.h" | |||
#include "llvm/MC/MCRegisterInfo.h" |
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.
Do we need these includes?
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.
Yes, they are needed for the SmallVectorImpl<MCInst>
and the MCRegister
.
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.
We don't need MCInst.h I think.
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.
Oh yes, you're right, sorry.
Good catch!
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.
It's its needed for MCRegister
, shouldn't it be MCRegister.h?
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. Thanks!
Hi, I fixed the imports by using I sadly do not have write access. Now that this pull request is approved, can someone please commit it on my behalf? Thank you! |
This allows to avoid duplicating the code handling the instructions outputted by `generateInstSeq` when emitting `MCInst`s.
This allows to avoid duplicating the code handling the instructions outputted by
generateInstSeq
when emittingMCInst
s.This will be used in #77337.