[PATCH] D103247: [SLP]Allow to reorder nodes with >2 scalar values.

[Simon Pilgrim via Phabricator via llvm-commits]

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

LGTM with a few minors



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7540
+        BoUpSLP::OrdersType NewOrder(Order->begin(), Order->end());
+        fixupOrderingIndices(NewOrder);
         // TODO: reorder tree nodes without tree rebuilding.
----------------
All the calls to fixupOrderingIndices create a 'NewOrder' immediately beforehand - maybe just perform the copy inside fixupOrderingIndices and return it? Its up to you - I have no strong preference.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:7970
   // extract scalars into scalar registers, so NeedExtraction is set true.
-  return tryToVectorizeList(BuildVectorOpds, R, /*AllowReorder=*/false);
 }
----------------
tbh I'd prefer to see the AllowReorder arg retained (and made compulsory in tryToVectorizeList) to make it easier to grok.


================
Comment at: llvm/test/Transforms/SLPVectorizer/X86/alternate-calls-inseltpoison.ll:7
+; RUN: opt < %s -mtriple=x86_64-unknown -mcpu=knl -basic-aa -slp-vectorizer -instcombine -S | FileCheck %s --check-prefixes=AVX512
+; RUN: opt < %s -mtriple=x86_64-unknown -mcpu=skx -basic-aa -slp-vectorizer -instcombine -S | FileCheck %s --check-prefixes=AVX512
 
----------------
I may be wrong but I think all the AVX modes can use the same AVX prefix?


================
Comment at: llvm/test/Transforms/SLPVectorizer/X86/alternate-calls.ll:7
+; RUN: opt < %s -mtriple=x86_64-unknown -mcpu=knl -basic-aa -slp-vectorizer -instcombine -S | FileCheck %s --check-prefixes=AVX512
+; RUN: opt < %s -mtriple=x86_64-unknown -mcpu=skx -basic-aa -slp-vectorizer -instcombine -S | FileCheck %s --check-prefixes=AVX512
 
----------------
I may be wrong but I think all the AVX modes can use the same AVX prefix?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103247