[PATCH] D135264: [MachineCombiner][RISCV] Enable MachineCombiner for RISCV

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1149
+  MachineInstrBuilder MIB1(MF, &NewMI1);
+  MIB1.add(FRM);
+  MachineInstrBuilder MIB2(MF, &NewMI2);
----------------
Probably want to make sure that the FrmOpIdx is the same as the current number of operands. If it's not then adding to the end is wrong.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1153
+  if (FRM.getImm() == RISCVFPRndMode::DYN) {
+    const MachineOperand &FRMDep = OldMI1.getOperand(FrmOpIdx + 1);
+    MIB1.add(FRMDep);
----------------
I'm not sure I like this assumption that FRM physical register is the next operand. Can we just create a FRM implicit use operand instead of copying it from the old?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1210
+
+  if (Options.UnsafeFPMath && getFPPatterns(Root, Patterns))
+    return true;
----------------
Why not use fast math flags?


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:271
   TargetPassConfig::addMachineSSAOptimization();
+  if (TM->getOptLevel() == CodeGenOpt::Aggressive && EnableMachineCombiner)
+    addPass(&MachineCombinerID);
----------------
Doesn't every other target put this in `addILPOpts`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135264