[PATCH] D93305: [AMDGPU][GlobalISel] Transform (fadd (fmul x, y), z) -> (fma x, y, z)

[Mirko Brkusanin via Phabricator via llvm-commits]

mbrkusanin added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4335-4347
+bool CombinerHelper::applyCombineFAddFMulToFMadOrFMA(
+    MachineInstr &MI,
+    std::tuple<Register, Register, Register, unsigned> &MatchInfo) {
+  Register Src1, Src2, Src3;
+  unsigned PreferredFusedOpcode;
+  std::tie(Src1, Src2, Src3, PreferredFusedOpcode) = MatchInfo;
+
----------------
We could instead use applyBuildFn which uses a lambda as MatchInfo that details what instructions to build. I noticed this is used for some new combines. It might look more readable then this long tuple. Especially for other fma combines.

If we do this for other fma combines as well then we could merge multiple matches into one (like the ones that start with fadd D93305, D97937, D97938, D98047) and avoid running a lot of common checks again for each combine (like the ones in canCombineFMadOrFMA).

@foad, @arsenm What do you think of this idea. Would it be more preferable? 
Main reason why all these matching opportunities are not part of the same match function is because we would need the large number of bools in tuple/struct to figure out what exactly to build in the apply function. A problem that can be avoided with this BuildFn lambda mechanism.





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

https://reviews.llvm.org/D93305