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

[Sanjoy Das via Phabricator via llvm-commits]

sanjoy added a comment.

Few random unsolicited comments inline.



================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2784
+/// may disappear with the call.
+static Instruction *extendArgAttrsToCaller(CallSite &CS, DominatorTree &DT) {
+  // Make sure that this callsite is part of a function.
----------------
We pass `CallSite` by value.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2791
+  if (!F)
+    return nullptr;
+
----------------
When can these be false (i.e. when can we have a call site not in a basic block or inside a function)?


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:2801
+  // Verify that all instructions before the call will execute sequentially.
+  for (Instruction &I : F->getEntryBlock()) {
+    if (&I == CS.getInstruction())
----------------
Maybe use range for here?

```
for (Instruction &I : make_range(BB->begin(), CS.getInstruction()->getIterator()))
```

at which point you can just `std::all_of` over the above.


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



https://reviews.llvm.org/D27855