[PATCH] D39726: [X86] Attempt to match multiple binary reduction ops at once. NFCI

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb accepted this revision.
andreadb added a comment.
This revision is now accepted and ready to land.

LGTM.

I made a couple of (minor) comments below.



================
Comment at: lib/Target/X86/X86ISelLowering.cpp:30119-30126
+    // Match against one of the candidate binary ops.
+    if (i == 0) {
+      if (llvm::none_of(CandidateBinOps, [Op](ISD::NodeType BinOp) {
+            return Op.getOpcode() == BinOp;
+          }))
+        return SDValue();
+      CandidateBinOp = Op.getOpcode();
----------------
If you want, you can move this check outside of the loop. The value of CandidateBinOp is only set by the first iteration of the loop, based on node `Op`.
If you decide to move the check before the loop, then the check for `Op.getOpcode() != CandidateBinOp` can be moved at around line 30140 (i.e. after the block of code that sets the Op for the next stage).


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:30138-30141
     // The first operand of the shuffle should be the same as the other operand
     // of the add.
     if (!Shuffle || (Shuffle->getOperand(0) != Op))
       return SDValue();
----------------
That comment seems to imply that we always try to match an 'add' reduction. I suggest to change the comment so that it says 'binary operation' instead of 'add'.


Repository:
  rL LLVM

https://reviews.llvm.org/D39726