[PATCH] D38566: [SimplifyCFG] don't sink common insts too soon (PR34603)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Passes/PassBuilder.cpp:464
+  // GVN has run, so allow more aggressive sinking and select formation.
+  FPM.addPass(SimplifyCFGPass(SimplifyCFGOptions().sinkCommonInsts(true)));
   FPM.addPass(InstCombinePass());
----------------
hfinkel wrote:
> I don't think that we want this here. This is still during canonicalization. You already have this right after SLP vectorization (we might actually want this right before SLP vectorization - do we SLP vectorize things we'll turn into selects otherwise?). Is it bad if you don't have this here for some test case?
There are no regression test diffs if I remove the sinking from that invocation (here and the equivalent in the old pass manager). 

But there is no SimplifyCFG ahead of the vectorizers in buildModuleOptimizationPipeline() (around line 685). Should I just add a SimplifyCFG with sinkCommonInsts ahead of the loop passes? I agree that we have the potential to miss vectorizations if we have not converted into select form before we get to the vectorization passes.

I tried to manufacture an -O2 PhaseOrdering test that would show the potential regression for SLP, but no luck yet.



https://reviews.llvm.org/D38566