[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:1164
+  }
+}
+
----------------
Should we intersect the MIFlags too like X86 and PowerPC do? Not sure where AArch64 doesn't.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1204
+  if (!Root.getFlag(MachineInstr::MIFlag::FmReassoc) ||
+      !MI->getFlag(MachineInstr::MIFlag::FmReassoc))
+    return false;
----------------
asi-sc wrote:
> I looked into how quite similar transformations are implemented in InstCombine. Usually it's enough to check that the root instruction allows reassociation. So, we can think of relaxing this condition.
> 
> One more thing to mention is that other targets in addititon to `FmReassoc` checks `FmNsz`. I don't see any need to do this for `fadd` and `fmul` cases. Please, correct me if I'm wrong.
I don't think Nsz is needed, but I'd rather us be consistent with the other 3 targets that do this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135264