[PATCH] D72386: [ThinLTO] pass UnrollLoops/VectorizeLoop/VectorizeSLP in CGOpts down to pass builder in ltobackend

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: clang/test/CodeGen/thinlto-slp-vectorize-pm.c:3
+// Test to ensure the SLP flag is passed down to the ThinLTO backend.
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-obj -O2 -vectorize-slp -mllvm -vectorize-slp=false -o %t2.o -x ir %t.o -fthinlto-index=%t.thinlto.bc -fdebug-pass-manager -fexperimental-new-pass-manager 2>&1 | FileCheck %s --check-prefix=O2-SLP
+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-obj -O0 -vectorize-slp -mllvm -vectorize-slp=false -o %t2.o -x ir %t.o -fthinlto-index=%t.thinlto.bc -fdebug-pass-manager -fexperimental-new-pass-manager 2>&1 | FileCheck %s --check-prefix=O0-SLP
----------------
wmi wrote:
> tejohnson wrote:
> > Can you clarify the difference between -vectorize-slp and -mllvm -vectorize-slp=false? Is this related to you comment about not being able to use the internal options to disable it? Worth a comment in that case. Ditto for the vectorize-loops tests below.
> -vectorize-slp is a cc1 option and will be added automatically when O2/O3/Os/Oz is available. Once -vectorize-slp is enabled, "-mllvm -vectorize-slp=false" won't disable it currently. I added "-mllvm -vectorize-slp=false" here in the test to ensure the slp vectorization is executed because the -vectorize-slp cc1 flag is passed down, not because -mllvm -vectorize-slp is enabled.
Why add "-mllvm -vectorize-slp=false" to the invocation lines though? If it is to illustrate that the internal option cannot override the cc1 option, then add comments to that effect.


================
Comment at: llvm/test/Other/new-pm-defaults.ll:244
+; CHECK-O3-NEXT: Running pass: SLPVectorizerPass
+; CHECK-Os-NEXT: Running pass: SLPVectorizerPass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
----------------
wmi wrote:
> tejohnson wrote:
> > Does clang run this at -Os?
> Yes it does. But surprisingly I find clang add "-vectorize-slp" at Oz as well but doesn't not add "-vectorize-loops" at Oz. This is another inconsistency with opt old pass manager, which enable neither slp nor vectorize-loops at Oz. I am not sure how it should behave. 
Hmm, I think probably best to be consistent with the old PM on this for now, and add a comment where you enable these differently. This should presumably be reevaluated, but probably best if the two are in sync for now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D72386