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

[Vyacheslav Klochkov via llvm-commits]

v_klochkov added a comment.

Hello,

I did just a very quick scan through the change-set, 
and added few minor comments related to styling.

Not ready to comment anything about the general idea of the changes.

Thank you,
Slava


================
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.)
----------------
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).

================
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,
----------------
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.

================
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;
----------------
One statement following an if-statement does not need braces { }.

================
Comment at: lib/Target/AArch64/AArch64InstrInfo.cpp:2800
@@ +2799,3 @@
+static bool
+getFPFusedMultiplyPatterns(MachineInstr &Root,
+                           SmallVectorImpl<MachineCombinerPattern> &Patterns) {
----------------
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).


http://reviews.llvm.org/D18751