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

[Hideto Ueno via Phabricator via llvm-commits]

uenoku added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1148
+        CS.paramHasAttr(ArgNo, Attribute::Dereferenceable) ||
         CS.paramHasAttr(ArgNo, getAttrKind()))
       indicateOptimisticFixpoint();
----------------
jdoerfert wrote:
> 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.
> or better, remove the attribute checks and make sure `isKnownNonZero` performs them.
I think `isKnownNonZero` would not check parameter attribute in call site so it is not correct.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1229
+  /// See AbstractState::isValidState()
+  bool isValidState() const override { return DerefBytesState.isValidState(); }
+
----------------
jdoerfert wrote:
> While it practically doesn't make a difference (I think) I would have suspected both states to be asked here.
Even though NonNullGlobalState was not valid, we can deduce `dereferenceable_or_null` so I think this is appropriate.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1397
+        BaseDerefAA->getAssumedDereferenceableBytes(), Offset.getSExtValue(),
+        BaseDerefAA->isAssumedNonNull());
+
----------------
jdoerfert wrote:
> 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?
> 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.

I'll try it later.


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


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

https://reviews.llvm.org/D64876