[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 marked 2 inline comments as done.
spatel added a comment.

In https://reviews.llvm.org/D27855#626049, @sanjoy wrote:

> Few random unsolicited comments inline.


Always appreciated!



================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2791
+  if (!F)
+    return nullptr;
+
----------------
sanjoy wrote:
> When can these be false (i.e. when can we have a call site not in a basic block or inside a function)?
Honestly, I don't know. I saw some other code - that I'm not finding now - that scared me into thinking that it's not safe to assume that an instruction is always tied to a block/function. I can make it an assert if we believe it's safe.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2827
+  bool PropagatedNonNull = false;
+  for (Value *V : NonNullParams) {
+    auto *CallerArg = dyn_cast<Argument>(V);
----------------
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
  }



https://reviews.llvm.org/D27855