[Passes] Move AggressiveInstCombine after InstCombine

[anton-afanasyev]

Summary

Swap AIC and IC neighbouring in pipeline. This looks more natural and even
almost has no effect for now (three slightly touched tests of test-suite). Also
this could be the first step towards merging AIC (or its part) to -O2 pipeline.

After several changes in AIC (like D108091, D108201, D107766, D109515, D109236)
there've been observed several regressions (like PR52078, PR52253, PR52289)
that were fixed in different passes (see D111330, D112721) by extending their
functionality, but these regressions were exposed since changed AIC prevents IC
from making some of early optimizations.

This is common problem and it should be fixed by just moving AIC after IC
which looks more logically by itself: make aggressive instruction combining
only after failed ordinary one.

Fixes PR52289

[lebedev.ri]

Isn't it just a symptom that said passes need to be enhanced?
They will still not handle the 'bad' patterns even if AIC no longer produces them beforehand.

[spatel]

In D113179#3108451, @lebedev.ri wrote:

Isn't it just a symptom that said passes need to be enhanced?
They will still not handle the 'bad' patterns even if AIC no longer produces them beforehand.

I agree (and I put an instcombine fold candidate in PR52289), but I also think this is a good change.
AIC isn't generally expecting non-canonical IR, so it could be missing transforms if it encounters weird IR (for example, a commutative binop with constant as operand 0).
Is there anything in the commit logs or patch reviews that explains why AIC was put before instcombine in the pipeline?

[anton-afanasyev]

In D113179#3108451, @lebedev.ri wrote:

Isn't it just a symptom that said passes need to be enhanced?
They will still not handle the 'bad' patterns even if AIC no longer produces them beforehand.

That's definitely a symptom, sure. I've waited some time to gather unveiled issues and going to wait more till all of them seem reported and fixed. But I still believe this change should be ever committed.

Is there anything in the commit logs or patch reviews that explains why AIC was put before instcombine in the pipeline?

I've passed through the long story of its review (D38313) and haven't seen special attention to this point. Initially AIC was implemented as a part of InstCombine, running _before_ its iterations of pattern-matching, but it was splitted out to separate pass in a process of review (https://reviews.llvm.org/D38313#906667).

[spatel]

In D113179#3108672, @anton-afanasyev wrote:

Is there anything in the commit logs or patch reviews that explains why AIC was put before instcombine in the pipeline?

I've passed through the long story of its review (D38313) and haven't seen special attention to this point. Initially AIC was implemented as a part of InstCombine, running _before_ its iterations of pattern-matching, but it was splitted out to separate pass in a process of review (https://reviews.llvm.org/D38313#906667).

Thanks for digging that up. I don't see any reason that it was placed that way either, so LGTM. Please wait to see if @lebedev.ri agrees though.

We should update/file bugs for the regressions that were uncovered by the previous AIC change, so they are independent of -O3/AIC and we don't lose those. Do you know if there others -- apart from the ones listed in the summary: PR52078 (fixed), PR52253 (in progress), PR52289 (updated/forked) ?

[anton-afanasyev]

We should update/file bugs for the regressions that were uncovered by the previous AIC change, so they are independent of -O3/AIC and we don't lose those. Do you know if there others -- apart from the ones listed in the summary: PR52078 (fixed), PR52253 (in progress), PR52289 (updated/forked) ?

That's full list for now.

[anton-afanasyev]

Thanks for digging that up. I don't see any reason that it was placed that way either, so LGTM. Please wait to see if @lebedev.ri agrees though.

Ping @lebedev.ri .

[RKSimon]

any more comments? @lebedev.ri @spatel

[lebedev.ri]

In D113179#3146480, @RKSimon wrote:

any more comments? @lebedev.ri @spatel

There was some older differential that proposed doing this,
IIRC there was disscussion there, but i'm failing to find it now.
Bu general recollection is that the order is intentional
to avoid pattern disturbance by instcombine before AIC has a chance.

[anton-afanasyev]

Bu general recollection is that the order is intentional
to avoid pattern disturbance by instcombine before AIC has a chance.

Hmm, it would be great to see an example. I've seen the reverse cases for now: disturbance by AIC before IC.

[anton-afanasyev]

Btw, the only impact seen by this patch is almost zero, the test suite shows three size-reduced tests and no regressions:

$ ~/llvm/test-suite/utils/compare.py -a --filter-short -m size..text result_base.json vs result_exp.json
 ...
 test-suite...abench/jpeg/jpeg-6a/cjpeg.test   167826.00  167810.00  -0.0%
 test-suite...nsumer-jpeg/consumer-jpeg.test   167186.00  167170.00  -0.0%
 test-suite...lications/sqlite3/sqlite3.test   496674.00  496546.00  -0.0%

[spatel]

In D113179#3146480, @RKSimon wrote:

any more comments? @lebedev.ri @spatel

I'll make my earlier acceptance official here in Phab.
I can't find any earlier reviews/comments to justify the ordering, there are no regression tests to support it, and we have evidence that this (new/proposed) ordering helps at least one case (and we still have a bug tracking the root cause).

[anton-afanasyev]

In D113179#3147047, @spatel wrote:

In D113179#3146480, @RKSimon wrote:

any more comments? @lebedev.ri @spatel

I'll make my earlier acceptance official here in Phab.
I can't find any earlier reviews/comments to justify the ordering, there are no regression tests to support it, and we have evidence that this (new/proposed) ordering helps at least one case (and we still have a bug tracking the root cause).

@lebedev.ri Need your final approval :)

[anton-afanasyev]

Update tests

clang/test/CodeGen/thinlto-distributed-newpm.ll

 ; CHECK-O: Running pass: PostOrderFunctionAttrsPass on (main)
 ; CHECK-O3: Running pass: ArgumentPromotionPass on (main)
 ; CHECK-O: Running pass: SROAPass on main
 ; CHECK-O: Running pass: EarlyCSEPass on main
 ; CHECK-O: Running pass: SpeculativeExecutionPass on main
 ; CHECK-O: Running pass: JumpThreadingPass on main
 ; CHECK-O: Running pass: CorrelatedValuePropagationPass on main
 ; CHECK-O: Running pass: SimplifyCFGPass on main
-; CHECK-O3: Running pass: AggressiveInstCombinePass on main
 ; CHECK-O: Running pass: InstCombinePass on main
+; CHECK-O3: Running pass: AggressiveInstCombinePass on main
 ; CHECK-O: Running pass: LibCallsShrinkWrapPass on main
 ; CHECK-O: Running pass: TailCallElimPass on main
 ; CHECK-O: Running pass: SimplifyCFGPass on main
 ; CHECK-O: Running pass: ReassociatePass on main
 ; CHECK-O: Running pass: RequireAnalysisPass<{{.*}}OptimizationRemarkEmitterAnalysis
 ; CHECK-O: Running pass: LoopSimplifyPass on main
 ; CHECK-O: Running pass: LCSSAPass on main
 ; CHECK-O: Running pass: SimplifyCFGPass on main

llvm/lib/Passes/PassBuilderPipelines.cpp

PassBuilder::buildFunctionSimplificationPipeline(OptimizationLevel Level,
   // Speculative execution if the target has divergent branches; otherwise nop.
   FPM.addPass(SpeculativeExecutionPass(/* OnlyIfDivergentTarget =*/true));
 
   // Optimize based on known information about branches, and cleanup afterward.
   FPM.addPass(JumpThreadingPass());
   FPM.addPass(CorrelatedValuePropagationPass());
 
   FPM.addPass(SimplifyCFGPass());
+  FPM.addPass(InstCombinePass());
   if (Level == OptimizationLevel::O3)
     FPM.addPass(AggressiveInstCombinePass());
-  FPM.addPass(InstCombinePass());
 
   if (!Level.isOptimizingForSize())
     FPM.addPass(LibCallsShrinkWrapPass());
 
   invokePeepholeEPCallbacks(FPM, Level);
 
   // For PGO use pipeline, try to optimize memory intrinsics such as memcpy
   // using the size value profile. Don't perform this when optimizing for size.
PassBuilder::buildLTODefaultPipeline(OptimizationLevel Level,
   // Remove unused arguments from functions.
   MPM.addPass(DeadArgumentEliminationPass());
 
   // Reduce the code after globalopt and ipsccp.  Both can open up significant
   // simplification opportunities, and both can propagate functions through
   // function pointers.  When this happens, we often have to resolve varargs
   // calls, etc, so let instcombine do this.
   FunctionPassManager PeepholeFPM;
+  PeepholeFPM.addPass(InstCombinePass());
   if (Level == OptimizationLevel::O3)
     PeepholeFPM.addPass(AggressiveInstCombinePass());
-  PeepholeFPM.addPass(InstCombinePass());
   invokePeepholeEPCallbacks(PeepholeFPM, Level);
 
   MPM.addPass(createModuleToFunctionPassAdaptor(std::move(PeepholeFPM),
                                                 PTO.EagerlyInvalidateAnalyses));
 
   // Note: historically, the PruneEH pass was run first to deduce nounwind and
   // generally clean up exception handling overhead. It isn't clear this is
   // valuable as the inliner doesn't currently care whether it is inlining an

llvm/test/Other/new-pm-defaults.ll

 ; CHECK-O-NEXT: Running pass: EarlyCSEPass
 ; CHECK-O-NEXT: Running analysis: MemorySSAAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: SpeculativeExecutionPass
 ; CHECK-O23SZ-NEXT: Running pass: JumpThreadingPass
 ; CHECK-O23SZ-NEXT: Running analysis: LazyValueAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: CorrelatedValuePropagationPass
 ; CHECK-O23SZ-NEXT: Invalidating analysis: LazyValueAnalysis
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O3-NEXT: AggressiveInstCombinePass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
+; CHECK-O3-NEXT: AggressiveInstCombinePass
 ; CHECK-O1-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O2-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O3-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-EP-PEEPHOLE-NEXT: Running pass: NoOpFunctionPass
 ; CHECK-O23SZ-NEXT: Running pass: TailCallElimPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Running pass: ReassociatePass
 ; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}OptimizationRemarkEmitterAnalysis

llvm/test/Other/new-pm-lto-defaults.ll

 ; CHECK-O-NEXT: Running analysis: CallGraphAnalysis
 ; CHECK-O-NEXT: Running pass: GlobalSplitPass
 ; CHECK-O-NEXT: Running pass: WholeProgramDevirtPass
 ; CHECK-O1-NEXT: Running pass: LowerTypeTestsPass
 ; CHECK-O23SZ-NEXT: Running pass: GlobalOptPass
 ; CHECK-O23SZ-NEXT: Running pass: PromotePass
 ; CHECK-O23SZ-NEXT: Running pass: ConstantMergePass
 ; CHECK-O23SZ-NEXT: Running pass: DeadArgumentEliminationPass
-; CHECK-O3-NEXT: Running pass: AggressiveInstCombinePass
 ; CHECK-O23SZ-NEXT: Running pass: InstCombinePass
+; CHECK-O3-NEXT: Running pass: AggressiveInstCombinePass
 ; CHECK-EP-Peephole-NEXT: Running pass: NoOpFunctionPass
 ; CHECK-O23SZ-NEXT: Running pass: ModuleInlinerWrapperPass
 ; CHECK-O23SZ-NEXT: Running analysis: InlineAdvisorAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: InlinerPass
 ; CHECK-O23SZ-NEXT: Running pass: InlinerPass
 ; CHECK-O23SZ-NEXT: Invalidating analysis: InlineAdvisorAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: GlobalOptPass
 ; CHECK-O23SZ-NEXT: Running pass: GlobalDCEPass

llvm/test/Other/new-pm-thinlto-defaults.ll

 ; CHECK-O-NEXT: Running pass: EarlyCSEPass
 ; CHECK-O-NEXT: Running analysis: MemorySSAAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: SpeculativeExecutionPass
 ; CHECK-O23SZ-NEXT: Running pass: JumpThreadingPass
 ; CHECK-O23SZ-NEXT: Running analysis: LazyValueAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: CorrelatedValuePropagationPass
 ; CHECK-O23SZ-NEXT: Invalidating analysis: LazyValueAnalysis
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O3-NEXT: Running pass: AggressiveInstCombinePass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
+; CHECK-O3-NEXT: Running pass: AggressiveInstCombinePass
 ; CHECK-O1-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O2-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O3-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O23SZ-NEXT: Running pass: TailCallElimPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Running pass: ReassociatePass
 ; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}OptimizationRemarkEmitterAnalysis
 ; CHECK-O-NEXT: Running pass: LoopSimplifyPass

llvm/test/Other/new-pm-thinlto-postlink-pgo-defaults.ll

 ; CHECK-O-NEXT: Running pass: EarlyCSEPass
 ; CHECK-O-NEXT: Running analysis: MemorySSAAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: SpeculativeExecutionPass
 ; CHECK-O23SZ-NEXT: Running pass: JumpThreadingPass
 ; CHECK-O23SZ-NEXT: Running analysis: LazyValueAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: CorrelatedValuePropagationPass
 ; CHECK-O23SZ-NEXT: Invalidating analysis: LazyValueAnalysis
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O3-NEXT: Running pass: AggressiveInstCombinePass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
+; CHECK-O3-NEXT: Running pass: AggressiveInstCombinePass
 ; CHECK-O1-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O2-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O3-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O23SZ-NEXT: Running pass: TailCallElimPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Running pass: ReassociatePass
 ; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}OptimizationRemarkEmitterAnalysis
 ; CHECK-O-NEXT: Running pass: LoopSimplifyPass

llvm/test/Other/new-pm-thinlto-postlink-samplepgo-defaults.ll

 ; CHECK-O-NEXT: Running pass: EarlyCSEPass
 ; CHECK-O-NEXT: Running analysis: MemorySSAAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: SpeculativeExecutionPass
 ; CHECK-O23SZ-NEXT: Running pass: JumpThreadingPass
 ; CHECK-O23SZ-NEXT: Running analysis: LazyValueAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: CorrelatedValuePropagationPass
 ; CHECK-O23SZ-NEXT: Invalidating analysis: LazyValueAnalysis
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O3-NEXT: Running pass: AggressiveInstCombinePass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
+; CHECK-O3-NEXT: Running pass: AggressiveInstCombinePass
 ; CHECK-O1-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O2-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O3-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O23SZ-NEXT: Running pass: TailCallElimPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Running pass: ReassociatePass
 ; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}OptimizationRemarkEmitterAnalysis
 ; CHECK-O-NEXT: Running pass: LoopSimplifyPass

llvm/test/Other/new-pm-thinlto-prelink-pgo-defaults.ll

 ; CHECK-O-NEXT: Running pass: EarlyCSEPass
 ; CHECK-O-NEXT: Running analysis: MemorySSAAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: SpeculativeExecutionPass
 ; CHECK-O23SZ-NEXT: Running pass: JumpThreadingPass
 ; CHECK-O23SZ-NEXT: Running analysis: LazyValueAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: CorrelatedValuePropagationPass
 ; CHECK-O23SZ-NEXT: Invalidating analysis: LazyValueAnalysis
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O3-NEXT: Running pass: AggressiveInstCombinePass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
 ; CHECK-O-NEXT: Running analysis: BlockFrequencyAnalysis on foo
 ; These next two can appear in any order since they are accessed as parameters
 ; on the same call to BlockFrequencyInfo::calculate.
 ; CHECK-O-DAG: Running analysis: LoopAnalysis on foo
 ; CHECK-O-DAG: Running analysis: BranchProbabilityAnalysis on foo
 ; CHECK-O-DAG: Running analysis: PostDominatorTreeAnalysis on foo
+; CHECK-O3-NEXT: Running pass: AggressiveInstCombinePass
 ; CHECK-O1-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O2-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O3-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O2-NEXT: Running pass: PGOMemOPSizeOpt
 ; CHECK-O3-NEXT: Running pass: PGOMemOPSizeOpt
 ; CHECK-O23SZ-NEXT: Running pass: TailCallElimPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Running pass: ReassociatePass

llvm/test/Other/new-pm-thinlto-prelink-samplepgo-defaults.ll

 ; CHECK-O-NEXT: Running pass: EarlyCSEPass
 ; CHECK-O-NEXT: Running analysis: MemorySSAAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: SpeculativeExecutionPass
 ; CHECK-O23SZ-NEXT: Running pass: JumpThreadingPass
 ; CHECK-O23SZ-NEXT: Running analysis: LazyValueAnalysis
 ; CHECK-O23SZ-NEXT: Running pass: CorrelatedValuePropagationPass
 ; CHECK-O23SZ-NEXT: Invalidating analysis: LazyValueAnalysis
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O3-NEXT: Running pass: AggressiveInstCombinePass
 ; CHECK-O-NEXT: Running pass: InstCombinePass
+; CHECK-O3-NEXT: Running pass: AggressiveInstCombinePass
 ; CHECK-O1-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O2-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O3-NEXT: Running pass: LibCallsShrinkWrapPass
 ; CHECK-O23SZ-NEXT: Running pass: TailCallElimPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
 ; CHECK-O-NEXT: Running pass: ReassociatePass
 ; CHECK-O-NEXT: Running pass: RequireAnalysisPass<{{.*}}OptimizationRemarkEmitterAnalysis
 ; CHECK-O-NEXT: Running pass: LoopSimplifyPass

llvm/test/Transforms/PhaseOrdering/X86/pr52289.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -O3 -S < %s | FileCheck %s
 
 target datalayout = "n8:16:32:64"
 
 define i32 @main(i32 %a) {
 ; CHECK-LABEL: @main(
 ; CHECK-NEXT:  if.end:
-; CHECK-NEXT:    [[TMP0:%.*]] = trunc i32 [[A:%.*]] to i8
-; CHECK-NEXT:    [[TMP1:%.*]] = add i8 [[TMP0]], 1
-; CHECK-NEXT:    [[CONV:%.*]] = and i8 [[TMP1]], 8
-; CHECK-NEXT:    [[CMP_I_NOT:%.*]] = icmp eq i8 [[CONV]], 0
-; CHECK-NEXT:    [[SHL_I:%.*]] = select i1 [[CMP_I_NOT]], i8 7, i8 0
-; CHECK-NEXT:    [[COND_I:%.*]] = shl i8 [[CONV]], [[SHL_I]]
-; CHECK-NEXT:    [[CONV1:%.*]] = sext i8 [[COND_I]] to i32
-; CHECK-NEXT:    [[SEXT:%.*]] = mul i32 [[CONV1]], 1355350016
-; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp ne i32 [[SEXT]], 65536
-; CHECK-NEXT:    tail call void @llvm.assume(i1 [[TOBOOL]])
 ; CHECK-NEXT:    ret i32 0
 ;
   %inc = add nsw i32 %a, 1
   %and = and i32 %inc, 8
   %conv = trunc i32 %and to i8
   %call = call fastcc i8 @a(i8 %conv, i32 7)
   %conv1 = sext i8 %call to i64
   %call2 = call fastcc i64 @d(i64 %conv1)