[PATCH] D27855: [InstCombine] try to extend nonnull-ness of arguments from a callsite back to its parent function

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2799
+  Instruction *Call = CS.getInstruction();
+  auto CallDominatesOtherUsesOfValue = [&Call, &DT](Value *V) {
+    for (User *U : V->users())
----------------
mkuper wrote:
> spatel wrote:
> > efriedma wrote:
> > > I don't think this check is sufficient; consider something like this:
> > > 
> > > ```
> > > void g(int* NONNULL notnull_ptr);
> > > void f(int *ptr, bool ptr_is_nonnull) {
> > >   if (ptr_is_nonnull) g(ptr);
> > > }
> > > ```
> > > 
> > > I think your check marks the "ptr" argument to f() as nonnull.  This deduction gets propagated out to callers of f(), which then assume the pointer isn't null.
> > Nice catch. Yes, that would be wrong. We can only do this if the callsite is in the entry block?
> I'd say "if the callsite post-dominates the entry block", except that LLVM's notion of post-dominance isn't really strong enough to be useful, IIRC.
You need to prove that it would be undefined behavior if the argument were in fact null, i.e. the if the function is called, the callsite will be executed.

You can break it down like this: the callsite needs to dominate every exit from the function, and every instruction which might execute before the callsite in question needs to be isGuaranteedToTransferExecutionToSuccessor() .  The first is true for every instruction in the entry block, but the second isn't.


https://reviews.llvm.org/D27855