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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.
This revision is now accepted and ready to land.

This looks ok to me, but maybe wait for one more review.



================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:9017
   unsigned Stages = Log2_32(Op.getValueType().getVectorNumElements());
+  assert(Stages < 31 && "Too many reduction stages");
 
----------------
I'm now regretting commenting about it in the first place.
We don't have a problem with `Stages==31` yet.
The loop would iterate from `0` to `30`, `1 << 30` is not UB,
and won't set signbit so the `for (int Index = 0; Index < (int)MaskEnd` loop is good too.
If you want to assert for `Stages`, then it should stay as `Stages < 32`, but that is trivially-tautological.
Or move the assertion back into the loop and assert `1 < 31`, which is what we are really checking.
Or just drop it, this review shows it to be more confusing and controversial than worthwhile.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65047