[PATCH] D64876: [Attributor] Deduce "dereferenceable" attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Initial comments.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:379
+// Helper function that returns argument index of value.
+// If the value is not an argument, this returns 0.
+static int getArgNo(Value &V) {
----------------
`-1 instead`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1125
     Argument *Arg = cast<Argument>(getAssociatedValue());
-    if (Arg->hasNonNullAttr())
+    if (Arg->hasNonNullAttr() || Arg->hasAttribute(Attribute::Dereferenceable))
       indicateOptimisticFixpoint();
----------------
Do we make sure `dereferenceable(0)` is invalid IR? If not, we should. (The logic you use above relies on it and other places might be as well.)


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1148
+        CS.paramHasAttr(ArgNo, Attribute::Dereferenceable) ||
         CS.paramHasAttr(ArgNo, getAttrKind()))
       indicateOptimisticFixpoint();
----------------
This can be commited separately with the other nonnull changes above. Please change the order in this conditional to first check the attributes and then call `isKnownNonZero`, or better, remove the attribute checks and make sure `isKnownNonZero` performs them.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1226
+    DEREF_GLOBAL = 1 << 1,
+  };
+
----------------
comments describing the members missing


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1229
+  /// See AbstractState::isValidState()
+  bool isValidState() const override { return DerefBytesState.isValidState(); }
+
----------------
While it practically doesn't make a difference (I think) I would have suspected both states to be asked here.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1379
+uint64_t AADereferenceableImpl::computeAssumedDerefenceableBytes(Attributor &A,
+                                                                 Value &V) {
+  // First, we try to get information about V from Attributor.
----------------
Add a TODO here wrt. tacking the globally flag as well, or just do it.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1382
+  auto *DerefAA = A.getAAFor<AADereferenceable>(*this, V);
+  if (DerefAA)
+    return DerefAA->getAssumedDereferenceableBytes();
----------------
Here and below it might make sense to move the declaration into the conditional:

` if (auto *DerefAA = ... )`


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1397
+        BaseDerefAA->getAssumedDereferenceableBytes(), Offset.getSExtValue(),
+        BaseDerefAA->isAssumedNonNull());
+
----------------
If `Offset` is not `0`, `Base` cannot be `null` (due to the `inbounds` properties). I thought about relying on the attribute to know this but that is actually not always possible. Instead, I think you can here say something like:
`Offset != 0 || BaseDerefAA->isAssumedNonNull()`

Furthermore, we could strip non-inbounds offsets as well, I think. Thought, we then have to be more careful with the difference calculation, e.g., only non-negative `Offset` values would then be allowed.

What do you think?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1410
+        DL.getTypeStoreSize(Base->getType()->getPointerElementType()),
+        Offset.getSExtValue(), true);
+
----------------
`true` should be `!NullPointerIsDefined(&getAnchorScope(), V.getType()->getPointerAddressSpace())` to allow `nullptr` as valid pointer.

Shouldn't one of the two checks above subsume the other? 


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1467
+        A.getAAFor<AADereferenceable>(*this, *CS.getInstruction(), ArgNo);
+
+    // Check that DereferenceableAA is AADereferenceableCallSiteArgument.
----------------
Why not call `computeAssumedDerefenceableBytes` on the `CS.getArgOperand(ArgNo)` value?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1804
+        // Call site argument attribute "dereferenceable".
+        registerAA(*new AADereferenceableCallSiteArgument(CS, i, InfoCache), i);
       }
----------------
Whitelist check missing here and above (and in existing code).


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

https://reviews.llvm.org/D64876