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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

In D65402#1694872 <https://reviews.llvm.org/D65402#1694872>, @uenoku wrote:

> Currently, any use is not tracked so nothing about A[1] or A[-2] is deduced.


As long as we have a test that is fine.

> This would be solved once making it track gep instruction. But beforehand, I strongly suggest separating deduction for known/assumption respectively.

What do you mean by the separation part?

I inlined one comment, if you agree with that one and the proposed fix, the rest looks good to me. If not, let me know.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1563
+          (int64_t)DL.getTypeStoreSize(
+              getPointerOperand(I)->getType()->getPointerElementType());
+
----------------
I'm still unsure about this logic, correct me if I'm wrong but we have:

  `Base = Offset + I.getPointer()`
and we know due to the access I that there are `D` dereferenceable bytes with
  `D = DL.getTypeStoreSize(getPointerOperand(I)->getType()->getPointerElementType());`
Now, deref from `Base` should be:
  `max(0, D + Offset)`
which is the same we have `AADereferenceableFloating::updateImpl`, but with an offset in the other direction, I think.


================
Comment at: llvm/test/Transforms/FunctionAttrs/dereferenceable.ll:199
+define i32* @test_for_minus_index(i32* %p) {
+; FIXME: This should be define nonnull dereferenceable(8) i32* @test_for_minus_index(i32* dereferenceable(4) nonnull %p)
+; ATTRIBUTOR: define nonnull dereferenceable(8) i32* @test_for_minus_index(i32* "no-capture-maybe-returned" %p)
----------------
I don't think we can deduce much about %p but only about the return, so I would expect
  `FIXME: This should be define nonnull dereferenceable(8) i32* @test_for_minus_index(i32* nonnull %p)`
or an explanation why %p is deref.



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

https://reviews.llvm.org/D65402