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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

One minor and one not so minor comment inlined. That is the last problem I found and once fixed this is fine :)

In D65402#1689907 <https://reviews.llvm.org/D65402#1689907>, @xbolva00 wrote:

> Nice, +1
>
> Is this the last remaining work before you turn the attributor on?


No, tuning is still needed, right now we focused on applicability and aggressive optimizations. I fixed all bugs I found in the TS and SPEC2006 the other day but I'll have to do that again with more benchmarks soon. This patch is important to fix various issues, one of which is the dereferenceability problem we have (D61652 <https://reviews.llvm.org/D61652>). I'll work on tuning and measuring the Attributor over the next two weeks, present results at the Dev-Meeting, and hopefully I'm able to propose to turn it on by then.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1546
+  const DataLayout &DL = A.getInfoCache().getDL();
+  if (const Value *Base = getBasePointerOfPointerOperand(I, Offset, DL)) {
+    if (Base == &AssociatedValue) {
----------------
uenoku wrote:
> jdoerfert wrote:
> > Reading this line it was at first not clear to me that `I` has to be an access for this to return a base pointer.
> What does it mean?
That means we might want to add a comment here or rename `getBasePointerOfPointerOperand` to something like `getBasePointerOfAccessPointerOperand`.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1554
+          DL.getTypeStoreSize(
+              getPointerOperand(I)->getType()->getPointerElementType());
+
----------------
Don't we have to check if offset is negative? Do we have a test?

something like `A[-2] = 0;` should not cause deref bytes. E.g., we might need to use 64 bit and signed version of deref bytes. but non-null can be set as soon as we know there is an access and null is not a valid pointer (probably need tests for these as well).

I thought we have a helper somewhere to deal with this offset and access size logic already?


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

https://reviews.llvm.org/D65402