[PATCH] D93750: [RISCV] Frame handling for RISC-V V extension.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:620
+
+    unsigned FactorRegister = 0;
+    int ShiftAmount = &Bin - &BinSizes[0];
----------------
unsigned -> Register.

Use RISCV::NoRegister instead of 0


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.cpp:621
+    unsigned FactorRegister = 0;
+    int ShiftAmount = &Bin - &BinSizes[0];
+    // The maximum LMUL is 8.
----------------
BinSizes.data() might be a little better than "&BinSizes[0]"


================
Comment at: llvm/lib/Target/RISCV/RISCVMCInstLower.cpp:204
+    OutMI.setOpcode(RISCV::CSRRS);
+    OutMI.addOperand(MCOperand::createImm(RISCVCSR::VLENB));
+    OutMI.addOperand(MCOperand::createReg(RISCV::X0));
----------------
Can we use RISCVSysReg::lookupSysRegByName("VLENB")->Encoding here instead of adding a new enum that repeats the addresses?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:173
+  bool NeedsIndirectAddressing = false;
+  const RISCVVPseudosTable::PseudoInfo *RVV =
+      RISCVVPseudosTable::getPseudoInfo(MI.getOpcode());
----------------
Are we just using RVV as a boolean and not a pointer? Should we do something like?

```
bool IsRVV = RISCVVPseudosTable::getPseudoInfo(MI.getOpcode()) != nullptr;
```


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp:192
+    Register RVVBaseReg = MRI.createVirtualRegister(&RISCV::GPRRegClass);
+    auto &Subtarget = MF.getSubtarget<RISCVSubtarget>();
+    unsigned LoadOpcode = Subtarget.is64Bit() ? RISCV::LD : RISCV::LW;
----------------
Add 'const' as the pre-merge check suggests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93750/new/

https://reviews.llvm.org/D93750