[PATCH] D80175: [PowerPC][MachineCombiner] reassociate fma to expose more ILP

[Jinsong Ji via Phabricator via llvm-commits]

jsji accepted this revision as: jsji.
jsji added a comment.
This revision is now accepted and ready to land.

LGTM with some nits.



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:354
+    } else if (Instr.getOpcode() !=
+               FMAOpIdxInfo[getFMAOpIdxInfo(Root.getOpcode())][1])
+      return false;
----------------
Can we define or use enum for index instead of hardcode 1,2,3,4? So that it will be easier to read and maintain.
eg: 
```
#define InfoArrayIdxFMAInst 0
#define InfoArrayIdxFAddInst 1
#define InfoArrayIdxFMULInst 2
#define InfoArrayIdxAddOpIdx 3 
#define InfoArrayIdxMULOpIdx 4 
```


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:395
+  // Prev must be a valid FMA like instruction.
+  if (!IsReassociable(*Prev, AddOpIdx, false, false))
+    return false;
----------------
Do we need to reset `AddOpIdx` befoer calling `IsReassociable` again? Or else the value is not `-1` anymore, we won't be able to catch issues in following `assert`..


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:322
+  /// chain ending in \p Root. All potential patterns are output in the \p
+  /// Pattern array.
+  bool getFMAPatterns(MachineInstr &Root,
----------------
`Pattern`? -> `P`? Or replace `P` to `Pattern` in following line.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.h:336
+  /// On PowerPC, we try to reassociate FMA chain which will increase 
+  /// instruction size. Set extension resource length limit to 1 for edge case.
+  int getExtendResourceLenLimit() const override { return 1; }
----------------
Can we make this comment more clearer? eg: Why we need to set it to 1? for what edge case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80175