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

[Hideto Ueno via Phabricator via llvm-commits]

uenoku added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1563
+          (int64_t)DL.getTypeStoreSize(
+              getPointerOperand(I)->getType()->getPointerElementType());
+
----------------
jdoerfert wrote:
> 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.
I agree.


================
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)
----------------
jdoerfert wrote:
> 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.
> 
It is my mistake. Fixed.


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

https://reviews.llvm.org/D65402