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

[Anton Sidorenko via Phabricator via llvm-commits]

asi-sc added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1204
+  if (!Root.getFlag(MachineInstr::MIFlag::FmReassoc) ||
+      !MI->getFlag(MachineInstr::MIFlag::FmReassoc))
+    return false;
----------------
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.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1210
+
+  if (Options.UnsafeFPMath && getFPPatterns(Root, Patterns))
+    return true;
----------------
craig.topper wrote:
> asi-sc wrote:
> > craig.topper wrote:
> > > Why not use fast math flags?
> > I changed current implementation to use fast math flags, but I have a question related to that.
> > Clang distributes fast math flags to instructions if `-ffast-math` is passed, whereas llc does not. So, theoretically we can have `UnsafeFPMath` option enabled and don't have fast math flags on instructions. What do you think, should we combine instructions in this case?
> Here's the patch where X86 removed the UnsafeFPMath check https://reviews.llvm.org/D74851
Thanks for the link. Then I think we also should use only instructions flags.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135264