[PATCH] D18751: [MachineCombiner] Support for floating-point FMA on ARM64

[Gerolf Hoflehner via llvm-commits]

Gerolf added a comment.




================
Comment at: include/llvm/CodeGen/MachineCombinerPattern.h:42
@@ +41,3 @@
+  MULSUBXI_OP1,
+  // Floating Point
+  FMULADDS_OP1,
----------------
jmolloy wrote:
> For the future: the pattern list is starting to grow quite large. I wonder if in the future we should consider moving the MachineCombinerPatterns to be table-generated?
Yes, I agree. And more generally we need to decide which pattern should be handled by DAGCombiner/Global Isel and what should be moved into the MachineCombiner. 

================
Comment at: include/llvm/CodeGen/SelectionDAGTargetInfo.h:141
@@ -140,1 +140,3 @@
   }
+  // Return true when it could be more effiicent to generated FMUL and ADD
+  // instead on FMA (similar for FMS, FMLA etc.)
----------------
v_klochkov wrote:
> There are several misprints here. 'effiicent'->'efficient', 'generated'->'generate',
> 'instead on'->instead of'.
> 
> Regarding the name of the function. Shouldn't it start with a lower case letter?
> Also, the name of the function would be a bit more informative if it was
>  fuseMulAddInMachineCombiner()
> or
>  generateFMAsInMachineCombiner()
> 
> The word 'Evaluate' initially made me think that the code did the replacement of FMAs with MUL/ADD. (Yes, in many cases such un-fusing may be very efficient).
Agree & done, except for the lower case/upper case issue. I picked the spelling consistent with other names in the corresponding header files. I will change all names following the current naming convention in a follow up commit.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2626
@@ -2605,3 +2625,3 @@
 
-static bool canCombineWithMUL(MachineBasicBlock &MBB, MachineOperand &MO,
-                              unsigned MulOpc, unsigned ZeroReg) {
+static bool canCombine(MachineBasicBlock &MBB, MachineOperand &MO,
+                       unsigned CombineOpc, unsigned ZeroReg = 0,
----------------
v_klochkov wrote:
> I think it is a good practice to add a descriptive comment telling what functions do, 
> including the description of the function parameters and returns. Well, at least for the newly added functions.
Done.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2638
@@ -2625,3 +2637,3 @@
   // Must only used by the user we combine with.
-  if (!MRI.hasOneNonDBGUse(MI->getOperand(0).getReg()))
+  if (!MRI.hasOneNonDBGUse(MI->getOperand(0).getReg())) {
     return false;
----------------
v_klochkov wrote:
> One statement following an if-statement does not need braces { }.
Done.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2800
@@ +2799,3 @@
+static bool
+getFPFusedMultiplyPatterns(MachineInstr &Root,
+                           SmallVectorImpl<MachineCombinerPattern> &Patterns) {
----------------
v_klochkov wrote:
> In my opinion, the function name is a bit confusing. 
> The comment says about MADD operations (i.e. MUL and ADD), but the function name has only the word 'Multiply'.
> The current name makes me think that function is doing some sort of fusion of few MULs into a bigger operation, e.g. a*b*c*d--> some_operation(a,b,c,d).
Changed to getFMAPatterns()

================
Comment at: test/CodeGen/AArch64/arm64-fma-combines.ll:121
@@ +120,3 @@
+  %e6 = insertelement <4 x float> %e5, float %add, i32 1
+  %add3 = fadd fast  <4 x float> %mul2, <float 3.000000e+00, float -3.000000e+00, float 5.000000e+00, float 7.000000e+00> 
+  %mulx = fmul fast <4 x float> %add2, %e2
----------------
flyingforyou wrote:
> Please, remove trailing whitespace.
done.


http://reviews.llvm.org/D18751