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

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 8 inline comments as done.
spatel added a comment.

In D64258#1577635 <https://reviews.llvm.org/D64258#1577635>, @jdoerfert wrote:

> Mostly comments to improve this. Two required changes.


Thanks!

> Maybe we could mention that this logic should, or better will, move into the Attributor framework somewhere?

Yes - I'll add a FIXME to the top of this file, so we know the whole thing should go away.



================
Comment at: llvm/lib/Transforms/IPO/InferFunctionAttrs.cpp:44
+    if (!match(&I, m_Load(m_Value(PtrOp))))
+      continue;
+
----------------
jdoerfert wrote:
> "Style": Is this the only use of the "match" function? If so, why not do the (in the middle-end) more familiar pattern of `dyn_cast` and `getPointerOperand`?
Yes - that's the only use of match. Since we're going to support stores now, it will go away. 
Note: 'match' is the more familiar pattern to me because that's used throughout instcombine.


================
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)
----------------
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?


================
Comment at: llvm/test/Transforms/InferFunctionAttrs/dereferenceable.ll:143
 
-; Could round weird bitwidths down?
+; TODO: Could round weird bitwidths down?
 
----------------
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.


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

https://reviews.llvm.org/D64258