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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I like this version a lot better. Selectively looking into the context is the right choice (I think). I added some comments, when the tests are updated I'll give it a last look over but this looks almost ready to me.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:701
+/// Base class is required to have `followUse` method.
+/// `followUse` returns true if the value should be tracked transitively.
+template <typename AAType, typename Base,
----------------
Please describe the type of `followUse` in more detail here. (what arguments, what do they mean, that the update logic should go in there, what not to do, etc.)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:738
+      }
+    }
+    return BeforeState == S ? ChangeStatus::UNCHANGED : ChangeStatus::CHANGED;
----------------
I would have though we need to advance the explorer iterator somewhere? 


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2444
+    }
+
+    return false;
----------------
This code looks like the other `followUse`. Can we have a helper like `getKnownNonNullAndDerefBytesForUse` which we call from both? (Maybe that also is reusable/combinable with the existing logic we have)


================
Comment at: llvm/test/Transforms/FunctionAttrs/dereferenceable.ll:189
+  ret void
+}
----------------
I think we lack a return value only test, see below.
```
define i32* @f7_3() {
; ATTRIBUTOR: define nonnull dereferenceable(4) i32* @f7_3()
  %ptr = tail call i32* @unkown_ptr()
  store i32 10, i32* %ptr, align 16
  ret i32* %ptr
}
```


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

https://reviews.llvm.org/D65402