[PATCH] D61780: [ARM][ParallelDSP] Change the search for smlads

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

A first pass, see inlined nits.



================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:100
 
-  struct Reduction {
-    PHINode         *Phi;             // The Phi-node from where we start
-                                      // pattern matching.
-    Instruction     *AccIntAdd;       // The accumulating integer add statement,
-                                      // i.e, the reduction statement.
-    OpChainList     MACCandidates;    // The MAC candidates associated with
-                                      // this reduction statement.
-    PMACPairList    PMACPairs;
-    Reduction (PHINode *P, Instruction *Acc) : Phi(P), AccIntAdd(Acc) { };
+  class Reduction {
+    Instruction     *Root = nullptr;
----------------
if we use doxygen style comments for the methods of this class, should we provide a general description here too?


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:104
+    OpChainList     Muls;
+    PMACPairList        MulPairs;
+    SmallPtrSet<Instruction*, 4> Adds;
----------------
nit: indentation


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:112
+
+    /// Record an Add instruction that is apart of the this reduction.
+    void InsertAdd(Instruction *I) { Adds.insert(I); }
----------------
nit: apart?


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:115
+
+    /// Record a BinOpChain, rooted at a Mul instruction, that is apart of this
+    /// reduction.
----------------
same here?


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:123
+    /// already been added.
+    bool InsertAcc(Value *I) {
+      if (Acc)
----------------
nit: perhaps this can also be a setter method, as that's the only thing it does essentially.

I am now curious about the false case. I.e., perhaps also briefly comment on what is supported (1 or more accumulators in 1 loop), and what happens in the false case.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:141
+    /// Return the add instruction which is the root of the reduction.
+    Instruction *getRoot() { return Root; }
+
----------------
another nit: the getter and setter methods are not capitalised, while the other methods are. Not sure they should be though....perhaps they are an exception.


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:461
+// To use SMLAD:
+// 1) we first need to find integer add reduction PHIs,
+// 2) then from the PHI, look for this pattern:
----------------
Comment out of date? Don't think we look at PHIs anymore, but just at any add?


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:491
+bool ARMParallelDSP::MatchSMLAD(Loop *L) {
+
+  // Search recursively back through the operands to find a tree of values that
----------------
nit: newline


================
Comment at: lib/Target/ARM/ARMParallelDSP.cpp:499
+
+    // If we find a non-instruction, it could be used as the initial
+    // accumulator value.
----------------
Perhaps good to spend a few words here on which non-instructions that might be?


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

https://reviews.llvm.org/D61780