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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks! Looks pretty good, I just have a few comments and questions.



================
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
----------------
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.


================
Comment at: lld/test/ELF/lto/slp-vectorize-pm.ll:11
+; CHECK: Starting llvm::Module pass manager run
+; CHECK: Finished llvm::Module pass manager run
+
----------------
I think the above testing was cloned from another existing lld test - can it be removed from this one since not related to vectorization?


================
Comment at: llvm/include/llvm/LTO/Config.h:130
 
+  PipelineTuningOptions PTO;
+
----------------
add doxygen comment


================
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
----------------
Does clang run this at -Os?


================
Comment at: llvm/test/tools/gold/X86/slp-vectorize-pm.ll:12
+
+; CHECK: Starting llvm::Module pass manager run
+
----------------
Similar comment to new lld test - I think the above lines are from another test and can be removed from here?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D72386