[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:1210
+
+  if (Options.UnsafeFPMath && getFPPatterns(Root, Patterns))
+    return true;
----------------
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?


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:271
   TargetPassConfig::addMachineSSAOptimization();
+  if (TM->getOptLevel() == CodeGenOpt::Aggressive && EnableMachineCombiner)
+    addPass(&MachineCombinerID);
----------------
craig.topper wrote:
> Doesn't every other target put this in `addILPOpts`?
Thanks for the question, it's definitely something we should discuss. You are right, `addILPOpts` is the common insertion point for this pass. However, my experiments show that it's more profitable to run machine combiner after machine LICM and machine sinking which are inserted right after ILPOpts customization point. I also need to say that my local machine combiner version has many more patterns to combine, e.g. it can reassociate integer additions.
One example I remember is that on Coremark machine sinking gathers a chain of additions in the loop:
```
bb.291.for.body3.us.us.i66:
; predecessors: %bb.289, %bb.290 
  successors: %bb.63(0x04000000), %bb.62(0x7c000000); %bb.63(3.12%), %bb.62(96.88%)

  %76:gpr = PHI %1287:gpr, %bb.289, %1285:gpr, %bb.290 
  %1288:gpr = PHI %1210:gpr, %bb.289, %1286:gpr, %bb.290 
  %1212:gpr = ADD %1211:gpr, %72:gpr 
  %1223:gpr = ADD %1222:gpr, %1212:gpr
  %1234:gpr = ADD %1233:gpr, %1223:gpr
  %1245:gpr = ADD %1244:gpr, %1234:gpr
  %1256:gpr = ADD %1255:gpr, %1245:gpr
  %1267:gpr = ADD %1266:gpr, %1256:gpr
  %1278:gpr = ADD %1277:gpr, %1267:gpr
  %77:gpr = ADD killed %1288:gpr, killed %1278:gpr
  %78:gpr = nuw ADDI %71:gpr, 8
  BNE %3114:gpr, %78:gpr, %bb.62
  PseudoBR %bb.63
```
Reassociation allows using multiple ALUs for this code.

At the same time I wasn't able to find any examples against inserting machine combiner a little later than other targets do. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135264