[PATCH] D65047: [DAGCombine] matchBinOpReduction - add partial reduction matching

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Seems ok.



================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:9056
   for (unsigned i = 0; i < Stages; ++i) {
+    assert(i < 32 && "Too many reduction stages");
+    unsigned MaskEnd = (1 << i);
----------------
xbolva00 wrote:
> assert ‘Stages’ , not ‘i’
Now the assertion is obviously-tautological, and it lost it's meaning - it does not convey the intent ('*this* code will break otherwise'), only some hardcoded limit.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:9086-9087
 
   BinOp = (ISD::NodeType)CandidateBinOp;
   return Op;
 }
----------------
Would it be better to always return via `PartialReduction()`, and special-case the case with non-partial reduction in `PartialReduction()` itself?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65047