[PATCH] D79971: [X86] Rewrite how X86PartialReduction find candidates to consider optimizing.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

I think this is ok, but it's hard to tell what might go wrong in the matching. Do we have enough negative tests in place for this analysis?



================
Comment at: llvm/lib/Target/X86/X86PartialReduction.cpp:123
     if (auto *BO = dyn_cast<BinaryOperator>(Op)) {
-      if (BO->getParent() == BB &&
+      if (BO->getParent() == BO->getParent() &&
           IsFreeTruncation(BO->getOperand(0)) &&
----------------
Either we still need a parent block check here or this always true comparison should be removed.


================
Comment at: llvm/lib/Target/X86/X86PartialReduction.cpp:297
 
-bool X86PartialReduction::trySADPattern(BinaryOperator *BO) {
-  if (!ST->hasSSE2())
-    return false;
+// Walk backwards from the ExtractElementInst and determine if its the end of
+// a horizontal reduction. Return the input to the reduction if find one.
----------------
its -> it is


================
Comment at: llvm/lib/Target/X86/X86PartialReduction.cpp:298
+// Walk backwards from the ExtractElementInst and determine if its the end of
+// a horizontal reduction. Return the input to the reduction if find one.
+static Value *matchAddReduction(const ExtractElementInst &EE) {
----------------
if find -> if we find?


================
Comment at: llvm/lib/Target/X86/X86PartialReduction.cpp:342
+
+    // Verify the shuffle has the expected (at this stage of the pyramdid) mask.
+    unsigned MaskEnd = 1 << i;
----------------
typo - pyramid


================
Comment at: llvm/lib/Target/X86/X86PartialReduction.cpp:354
+// use BinaryOperators with the same opcode. If we get back then we know we've
+// found a loop and its safe to step through this Add to find more leaves.
+static bool isReachableFromPHI(PHINode *Phi, BinaryOperator *BO) {
----------------
its -> it is


================
Comment at: llvm/lib/Target/X86/X86PartialReduction.cpp:406
+
+        // If there is additional use, make sure its an unvisited phi that gets
+        // us back to this node.
----------------
its -> it is


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

https://reviews.llvm.org/D79971