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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1586-1593
   /// Match a binop + shuffle pyramid that represents a horizontal reduction
   /// over the elements of a vector starting from the EXTRACT_VECTOR_ELT node /p
   /// Extract. The reduction must use one of the opcodes listed in /p
   /// CandidateBinOps and on success /p BinOp will contain the matching opcode.
   /// Returns the vector that is being reduced on, or SDValue() if a reduction
-  /// was not matched.
+  /// was not matched. If /p AllowPartials is set then in the case of a
+  /// reduction pattern that only matches the first few stages, the extracted
----------------
lebedev.ri wrote:
> Aren't slashes wrong here, should it be `\p` ?
(in the original comment too, but that is not specific to the patch)


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:9055-9056
+  SDValue PrevOp;
   for (unsigned i = 0; i < Stages; ++i) {
+    unsigned MaskEnd = (1 << i);
+
----------------
lebedev.ri wrote:
> This might warrant a comment/assertion that we do know that i is always less than 31.
I indeed meant `31`, **not** `32`.
Because if `i=31`, while there's no overflow, you get `(unsigned(1)<<31)`. which is `INT_MIN`,
and then in 
```
for (int Index = 0; Index < (int)MaskEnd; ++Index)
```
you'll get at least a signed overflow, and maybe a deadlock.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D65047