[PATCH] D129735: [RISCV] Add new pass to transform undef to pseudo for vector values.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp:149
+    MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI) {
+  MachineRegisterInfo *MRI = &(MBB.getParent()->getRegInfo());
+  MachineInstr &MI = *MBBI;
----------------
I kind of think we should keep these instructions all the way to `RISCVAsmPrinter::emitInstruction`. Setting the operands to "undef" says it is ok to change the operands after this pass runs, but its not.  RISCVExpandPseudo runs late enough there is probably no pass that will change them. Keeping the pseudo all the way to `RISCVAsmPrinter::emitInstruction` removes any possibility.


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:87
+RISCVInitUndef::getVRLargestSuperClass(const TargetRegisterClass *RC) const {
+  const TargetRegisterClass *Super = RC;
+  TargetRegisterClass::sc_iterator I = RC->getSuperClasses();
----------------
Can this be

```
if (RISCV::VRM8RegClass.hasSubClassEq(RC))
  return &RISCV::VRM8RegClass;
if (RISCV::VRM4RegClass.hasSubClassEq(RC))
  return &RISCV::VRM4RegClass;
if (RISCV::VRM2RegClass.hasSubClassEq(RC))
  return &RISCV::VRM2RegClass;
if (RISCV::VRRegClass.hasSubClassEq(RC))
  return &RISCV::VRRegClass;
return RC;
```


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:116
+  case RISCV::VRRegClassID:
+    Opcode = RISCV::PseudoRVVInitUndefM1;
+    break;
----------------
`return RISCV::PseudoRVVInitUndefM1;`


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:151
+    bool TiedToDef = false;
+    for (MachineOperand &UseMO : UserMI->operands()) {
+      if (!UseMO.isReg())
----------------
Do we need to scan all operands? Can we check if MO is a tied use and find the operand it is tied to check the early clobber?


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:187
+static bool isEarlyClobberMI(MachineInstr &MI) {
+  for (MachineOperand &UseMO : MI.operands()) {
+    if (!UseMO.isReg())
----------------
Use `defs()`?


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:187
+static bool isEarlyClobberMI(MachineInstr &MI) {
+  for (MachineOperand &UseMO : MI.operands()) {
+    if (!UseMO.isReg())
----------------
craig.topper wrote:
> Use `defs()`?
Replace the place with llvm::any_of?


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:200
+
+  for (MachineOperand &UseMO : MI.operands()) {
+    if (!UseMO.isReg())
----------------
Can we scan `uses()` instead of `operands()`?


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:205
+      continue;
+    if (UseMO.isEarlyClobber())
+      continue;
----------------
I think we wouldn't need this if we only checked `uses`?


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:228
+    TRI->getCoveringSubRegIndexes(*MRI, TargetRegClass,
+                                  Info.UsedLanes & ~Info.DefinedLanes,
+                                  SubRegIndexNeedInsert);
----------------
Can we put `Info.UsedLanes & ~Info.DefinedLanes` into a variable? We use that expression twice


================
Comment at: llvm/lib/Target/RISCV/RISCVRVVInitUndef.cpp:231
+
+    Register LastestReg = Reg;
+
----------------
`Lastest` isn't a word


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129735