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

[Quentin Colombet via llvm-commits]

qcolombet added a comment.

Hi Gerolf,

I haven’t looked into neither the machine combiner or the aarch64 specific changes, but here are some thoughts on DAG related parts.

Let me know if you want me to look into the other parts as well.

Cheers,
-Quentin


================
Comment at: include/llvm/CodeGen/SelectionDAGTargetInfo.h:144
@@ -141,1 +143,3 @@
+  // This will be evaluated by the machine combiner.
+  virtual bool GenerateFMAsInMachineCombiner() const { return false; }
 };
----------------
Shouldn’t the comment say something like: return true if we want to delegate the generation of fma to the machine combiner?
With the current comment I fell like this is a shift between what the comment said and what the hook is used for:
- From the name of the method, I would expect the decision to generate fma is delegated to the machine combiner.
- From the comment, it seems I need to return true if my target supports FMA and the fact that this combining is done by the machine combiner is almost anecdotic here.

================
Comment at: include/llvm/CodeGen/SelectionDAGTargetInfo.h:144
@@ -141,1 +143,3 @@
+  // This will be evaluated by the machine combiner.
+  virtual bool GenerateFMAsInMachineCombiner() const { return false; }
 };
----------------
qcolombet wrote:
> Shouldn’t the comment say something like: return true if we want to delegate the generation of fma to the machine combiner?
> With the current comment I fell like this is a shift between what the comment said and what the hook is used for:
> - From the name of the method, I would expect the decision to generate fma is delegated to the machine combiner.
> - From the comment, it seems I need to return true if my target supports FMA and the fact that this combining is done by the machine combiner is almost anecdotic here.
Take the optimization level as input of the hook to give more control to the target.

================
Comment at: include/llvm/CodeGen/SelectionDAGTargetInfo.h:144
@@ -141,1 +143,3 @@
+  // This will be evaluated by the machine combiner.
+  virtual bool GenerateFMAsInMachineCombiner() const { return false; }
 };
----------------
qcolombet wrote:
> qcolombet wrote:
> > Shouldn’t the comment say something like: return true if we want to delegate the generation of fma to the machine combiner?
> > With the current comment I fell like this is a shift between what the comment said and what the hook is used for:
> > - From the name of the method, I would expect the decision to generate fma is delegated to the machine combiner.
> > - From the comment, it seems I need to return true if my target supports FMA and the fact that this combining is done by the machine combiner is almost anecdotic here.
> Take the optimization level as input of the hook to give more control to the target.
A remark for the future.

Would it make sense to have this hook take an enum or something of the combines we want to delegate to the machine combiner?
We don’t have to decide now, but I believe the approach could be generalized to other hooks and we do not want one hook per instruction/combine.

================
Comment at: include/llvm/Target/TargetInstrInfo.h:824
@@ +823,3 @@
+  /// \param Pattern - combiner pattern
+  virtual bool isThroughputPattern(MachineCombinerPattern Pattern) const;
+
----------------
Could we have a property on the pattern instead?
What are the other kinds of patterns?

The machine combiner optimize either for throughput or latency, something else?

================
Comment at: lib/CodeGen/MachineCombiner.cpp:415
@@ -409,1 +414,3 @@
 
+      bool substituteAlways = false;
+      if (ML && TII->isThroughputPattern(P)) {
----------------
Capital letter for the first letter per the coding standard.

================
Comment at: lib/CodeGen/MachineCombiner.cpp:418
@@ -410,1 +417,3 @@
+        substituteAlways = true;
+      }
       // Substitute when we optimize for codesize and the new sequence has
----------------
No curly braces per the coding standard.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:89
@@ -87,2 +88,3 @@
     SelectionDAG &DAG;
+    const SelectionDAGTargetInfo &STI;
     const TargetLowering &TLI;
----------------
Since this is already accessible through the DAG and we use that only twice, I am not sure it is worth adding a field for it.
I don’t feel strongly about it though.

================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:7865
@@ +7864,3 @@
+  if (AllowFusion && OptLevel >= CodeGenOpt::Aggressive &&
+      STI.GenerateFMAsInMachineCombiner())
+    return SDValue();
----------------
Query the opt level from the STI hook.
That way the target has a better control on when it wants to move the combine to the machine combiners.


http://reviews.llvm.org/D18751