[PATCH] D65402: [Attributor][MustExec] Deduce dereferenceable and nonnull attribute using MustBeExecutedContextExplorer

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Hopefully last round of comments.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:739
+        if (std::find(Explorer.begin(CtxI), Explorer.end(), UserI) !=
+            Explorer.end())
+          if (Base::followUse(A, U, UserI))
----------------
This also only works for the first time the explorer iterator is created, the contains call before was the right way (as long as the explorer works the way it does now).

You can do sth like:
```
for (const Use *U : Uses) {
  const Instruction *UserI = cast<Instruction>(U->getUser());
  auto EIt = Explorer.begin(CtxI), EEnd = Explorer.end(CtxI);
  bool Found = EIt.contains(UserI);
  while (!Found && ++EIt != EEnd)
     Found = EIt.getCurrentInst() == UserI;
  if (Found && Base::followUse(A, U, UserI))
    for (const Use &Us : UserI->uses())
              Uses.insert(&Us);
}

       
```


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1533
+    if (ICS.isCallee(U)) {
+      IsNonNull = true;
+      return 0;
----------------
This is only true, I think, if null is not a valid pointer.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1546
+  const DataLayout &DL = A.getInfoCache().getDL();
+  if (const Value *Base = getBasePointerOfPointerOperand(I, Offset, DL)) {
+    if (Base == &AssociatedValue) {
----------------
Reading this line it was at first not clear to me that `I` has to be an access for this to return a base pointer.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1585
+           (std::to_string(getAssumed()) + "/" + std::to_string(getKnown()));
+    // return getAssumed() ? "nonnull" : "may-null";
   }
----------------
Do we want this change or was it just for debugging? Shouldn't it already say "nonnull [fix]" if it is known?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1616
+    ChangeStatus Change = Base::updateImpl(A);
+
     const DataLayout &DL = A.getDataLayout();
----------------
Change is not used. You could check if it is fixed/known after this update and not do the stuff below in that case.


================
Comment at: llvm/test/Transforms/FunctionAttrs/dereferenceable.ll:197
+  ret i32* %ptr
+}
----------------
Is this derived or not? There is the same string after the `FIXME` and `ATTRIBUTOR`, right?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65402/new/

https://reviews.llvm.org/D65402