[PATCH] D64258: [InferFuncAttributes] extend 'dereferenceable' attribute based on loads

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Fwiw, I don't need all the proposed improvements. Getting this in with one or more FIXMEs is fine with me.



================
Comment at: llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp:51
+    Value *Base = GetPointerBaseWithConstantOffset(PtrOp, ByteOffset, DL);
+    auto *Arg = dyn_cast_or_null<Argument>(Base);
+    if (!Arg || ByteOffset < 0)
----------------
spatel wrote:
> jdoerfert wrote:
> > I don't think you can get a `nullptr` back.
> Ah, I misunderstood the API - I thought if there's no constant offset, the base should be returned as nullptr. 
> 
> Independent of this patch: add a line to the documentation comment to make the behavior clear as part of the cleanup in D64468?
> Independent of this patch: add a line to the documentation comment to make the behavior clear as part of the cleanup in D64468?

Done, take a look if that is what you wanted. (The wrappers are "not well" documented but only the base function is.)


================
Comment at: llvm/test/Transforms/InferFunctionAttrs/dereferenceable.ll:143
 
-; Could round weird bitwidths down?
+; TODO: Could round weird bitwidths down?
 
----------------
spatel wrote:
> jdoerfert wrote:
> > Yes ;)
> I've left this as a TODO for now because it's highly unusual to see non-power-of-2 bitwidths.
> We're going to rewrite the code fairly soon, and we'll have a code comment and this test in place to remind us that we can make the logic more flexible.
Fine with me.


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

https://reviews.llvm.org/D64258