[Attributor] Move pass after InstCombine to futher eliminate null pointer checks

[xbolva00]

Summary

PR44149

[xbolva00]

But I am bit worried that we are in a trap.

If I remember well, @spatel's motivating case needs attributor to run before instcombine. So we need atleast two instances of Attributor before/after IC to catch this and spatel's case?

[jdoerfert]

Why is inscombine the pass that adds nonnull to strchr?
This is not a good solution, I'll try to come up with the SCC version of the Attributor soon to add it to the inliner loop

[xbolva00]

In D70737#1760839, @jdoerfert wrote:

Why is inscombine the pass that adds nonnull to strchr?
This is not a good solution, I'll try to come up with the SCC version of the Attributor soon to add it to the inliner loop

You were fine with this approach..... https://reviews.llvm.org/D53342.

[jdoerfert]

In D70737#1760841, @xbolva00 wrote:

In D70737#1760839, @jdoerfert wrote:

Why is inscombine the pass that adds nonnull to strchr?
This is not a good solution, I'll try to come up with the SCC version of the Attributor soon to add it to the inliner loop

You were fine with this approach..... https://reviews.llvm.org/D53342.

I did not make the connection from SimplifyLibCalls to Instcombine. I think annotating library calls should live in SimplifyLibCalls.

Let me try to come up with the Attributor SCC pass so we can run it early on the entire module and during the inliner loop on an SCC at a time. That would solve the problem. OK? (We should keep the test though)

[jdoerfert]

I need to write the CG update part and some tests but D70767 should solve this properly.

[xbolva00]

Great!

[lebedev.ri]

From the disscussion, i didn't get the impression that there was a consensus on the move?

[xbolva00]

Sorry, commited accidentally! reverted

llvm/lib/Transforms/IPO/PassManagerBuilder.cpp

void PassManagerBuilder::populateModulePassManager(
   addExtensionsToPM(EP_ModuleOptimizerEarly, MPM);
 
   if (OptLevel > 2)
     MPM.add(createCallSiteSplittingPass());
 
   MPM.add(createIPSCCPPass());          // IP SCCP
   MPM.add(createCalledValuePropagationPass());
 
-  // Infer attributes on declarations, call sites, arguments, etc.
-  MPM.add(createAttributorLegacyPass());
-
   MPM.add(createGlobalOptimizerPass()); // Optimize out global vars
   // Promote any localized global vars.
   MPM.add(createPromoteMemoryToRegisterPass());
 
   MPM.add(createDeadArgEliminationPass()); // Dead argument elimination
 
   addInstructionCombiningPass(MPM); // Clean up after IPCP & DAE
+  // Infer attributes on declarations, call sites, arguments, etc.
+  MPM.add(createAttributorLegacyPass());
   addExtensionsToPM(EP_Peephole, MPM);
   MPM.add(createCFGSimplificationPass()); // Clean up after IPCP & DAE
 
   // For SamplePGO in ThinLTO compile phase, we do not want to do indirect
   // call promotion as it will change the CFG too much to make the 2nd
   // profile annotation in backend more difficult.
   // PGO instrumentation is added during the compile phase for ThinLTO, do
   // not run it a second time

llvm/test/Transforms/PhaseOrdering/null-check-elim.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -O3 -attributor-disable=false -S < %s                    | FileCheck %s --check-prefixes=ANY,OLDPM
+; RUN: opt -passes='default<O3>' -attributor-disable=false -S < %s  | FileCheck %s --check-prefixes=ANY,NEWPM
+
+@p = external global [2 x i8*], align 16
+
+define void @test(i8* %arg, i32 %arg1) {
+; OLDPM-LABEL: @test(
+; OLDPM-NEXT:  bb5:
+; OLDPM-NEXT:    [[TMP:%.*]] = tail call i8* @strchr(i8* nofree nonnull dereferenceable(1) [[ARG:%.*]], i32 [[ARG1:%.*]]) #1
+; OLDPM-NEXT:    store i8* [[TMP]], i8** getelementptr inbounds ([2 x i8*], [2 x i8*]* @p, i64 0, i64 0), align 16
+; OLDPM-NEXT:    [[TMP4:%.*]] = tail call i8* @foo(i8* nonnull [[ARG]])
+; OLDPM-NEXT:    store i8* [[TMP4]], i8** getelementptr inbounds ([2 x i8*], [2 x i8*]* @p, i64 0, i64 1), align 8
+; OLDPM-NEXT:    ret void
+;
+; NEWPM-LABEL: @test(
+; NEWPM-NEXT:  bb:
+; NEWPM-NEXT:    [[TMP:%.*]] = tail call i8* @strchr(i8* nonnull dereferenceable(1) [[ARG:%.*]], i32 [[ARG1:%.*]])
+; NEWPM-NEXT:    store i8* [[TMP]], i8** getelementptr inbounds ([2 x i8*], [2 x i8*]* @p, i64 0, i64 0), align 16
+; NEWPM-NEXT:    [[TMP2:%.*]] = icmp eq i8* [[ARG]], null
+; NEWPM-NEXT:    br i1 [[TMP2]], label [[BB5:%.*]], label [[BB3:%.*]]
+; NEWPM:       bb3:
+; NEWPM-NEXT:    [[TMP4:%.*]] = tail call i8* @foo(i8* nonnull [[ARG]])
+; NEWPM-NEXT:    br label [[BB5]]
+; NEWPM:       bb5:
+; NEWPM-NEXT:    [[TMP6:%.*]] = phi i8* [ [[TMP4]], [[BB3]] ], [ null, [[BB:%.*]] ]
+; NEWPM-NEXT:    store i8* [[TMP6]], i8** getelementptr inbounds ([2 x i8*], [2 x i8*]* @p, i64 0, i64 1), align 8
+; NEWPM-NEXT:    ret void
+;
+bb:
+  %tmp = tail call i8* @strchr(i8* %arg, i32 %arg1)
+  store i8* %tmp, i8** getelementptr inbounds ([2 x i8*], [2 x i8*]* @p, i64 0, i64 0), align 16
+  %tmp2 = icmp eq i8* %arg, null
+  br i1 %tmp2, label %bb5, label %bb3
+
+bb3:                                              ; preds = %bb
+  %tmp4 = tail call i8* @foo(i8* %arg)
+  br label %bb5
+
+bb5:                                              ; preds = %bb3, %bb
+  %tmp6 = phi i8* [ %tmp4, %bb3 ], [ null, %bb ]
+  store i8* %tmp6, i8** getelementptr inbounds ([2 x i8*], [2 x i8*]* @p, i64 0, i64 1), align 8
+  ret void
+}
+
+declare i8* @strchr(i8*, i32)
+declare i8* @foo(i8*)