[PATCH] D79078: [VectorCombine] Leave reduction operation to SLP

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

I'll defer to @spatel, although i semi-weakly insist that adding such cut-offs is weird.
OTOH if this pass is taught to form such reductions we would have caught this regression for free,
because after D79799 <https://reviews.llvm.org/D79799> we would have ended up in an endless combine loop here.

In D79078#2010024 <https://reviews.llvm.org/D79078#2010024>, @spatel wrote:

> I agree that we need a phase ordering test - see llvm/test/Transforms/PhaseOrdering/X86/addsub.ll as an example test file that describes some expected handshake between vector-combine and other passes. Also, please commit the new test with complete baseline CHECK lines (use utils/update_test_checks.py), and then update the file to show the diffs in this review. Let me know if that's not clear.


Almost there - it should be next to the example test, not where it is now.

In D79078#2010024 <https://reviews.llvm.org/D79078#2010024>, @spatel wrote:

> I also sympathize with trying to solve this here rather than SLP. One of the reasons vector-combine exists is because SLP became too hard to reason about. In hindsight, we should have created a separate pass for reductions - those are not traditional SLP concerns. Just my opinion. :)


I'm not sure what you have in mind here?
That *this* pass should also form such reductions?
Or that we should not disturb them after SLP formed them?
Or something else?

In D79078#2031347 <https://reviews.llvm.org/D79078#2031347>, @junparser wrote:

> hi @lebedev.ri , sorry for the late response.
>  I have checked the reduction tree build in SLPVectorizer, for now it only support same operation. It seems we can revert the transformation when match reduction operation in matchAssociativeReduction. However, I don't think it is a good idea.


I'm having trouble parsing this response.
Are you saying that SLP can not be taught about this,
that it should not be taught about this or ???.



================
Comment at: llvm/test/Transforms/VectorCombine/X86/extract-binop.ll:4-5
 ; RUN: opt < %s -vector-combine -S -mtriple=x86_64-- -mattr=AVX2 | FileCheck %s --check-prefixes=CHECK,AVX
+; RUN: opt < %s -O3 -S -mtriple=x86_64-- -mattr=avx | FileCheck %s --check-prefixes=PHASE
+; RUN: opt -passes='default<O3>' -S < %s  | FileCheck %s --check-prefixes=PHASE
 
----------------
Like @spatel has already said, please see `llvm/test/Transforms/PhaseOrdering/X86/addsub.ll`
The passordering test should be similar to that one, in that directory.


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

https://reviews.llvm.org/D79078