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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2827
+  bool PropagatedNonNull = false;
+  for (Value *V : NonNullParams) {
+    auto *CallerArg = dyn_cast<Argument>(V);
----------------
spatel wrote:
> sanjoy wrote:
> > I don't think this is necessary.  You've proved that once you enter the caller, you are going to pass the said argument as a `nonnull` parameter to `CS`.  This means that the incoming value is `nonnull` too, irrespective of where all it is used.
> > 
> > That is, instead of `NonNullParams.push_back(V)`, you should be able to do:
> > 
> > ```
> > if (auto *A = dyn_cast<Argument>(V))
> >   A->addAttr(NonNull);
> > ```
> > 
> We have to prove that the first use of the arg is a nonnull use, and so the 'dominates' check is needed. Let me know if I'm not understanding the suggestion. 
> 
> The first test case was supposed to demonstrate this, but it doesn't fulfill that intent after the addition of the isGuaranteedToTransferExecutionToSuccessor() check. I can add a test case like below to show this possibility.
> 
>   declare i8 @use1readonly(i8* %x) readonly ; readonly guarantees that execution continues to successor
>   declare void @use1nonnull(i8* nonnull %x);
> 
>   define i8 @parent7(i8* %a) {
>     %ret = call i8 @use1readonly(i8* %a) ; %a could be null in here
>     call void @use1nonnull(i8* %a) ; guaranteed nonnull use, but can't extend nonnull to parent
>     ret i8 %ret
>   }
> 
On 2nd thought, my reply makes no sense. :)
You're right - we're now guaranteeing that the call with nonnull must execute, so the arg must be nonnull for the parent.
I think we still need the extra test case to show this because the first test's call doesn't guarantee execution to successor.


https://reviews.llvm.org/D27855