[PATCH] D103860: [Attributor] Use AAValueSimplify to simplify returned values

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:1031
+        A, IRPosition::value(*Ret.getReturnValue()), *this, Ret, ReturnValueCB,
+        &I);
   };
----------------
kuter wrote:
> `genericValueTraversal` does not look at callbase returned values.
> So the `AAReturnedValues` no longer contains transitively returned values. Wouldn't this affect some attributes ?
This is a good thing, I think. From the commit message:

```AAReturnedValues is now much easier and the collected returned
values/instructions are now from the associated function only, making it
much more sane. We also do not have the brittle logic anymore that looks
for unresolved calls. Instead, we use AAValueSimplify to handle
recursion.```

If you use value simplification you still look through the call sites into callers, even better than before. If you use AAReturnedValues you will be restricted to the values in that function, I think this is better overall.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:4393-4407
+    if (Simplify) {
+      auto &ValueSimplifyAA = A.getAAFor<AAValueSimplify>(
         QueryingAA,
         IRPosition::value(QueryingValue, QueryingAA.getCallBaseContext()),
         DepClassTy::REQUIRED);
 
+      AccumulatedSimplifiedValue = AA::combineOptionalValuesInAAValueLatice(
----------------
kuter wrote:
> I think the code can be made more readable here.
Will do :)


================
Comment at: llvm/test/Transforms/Attributor/nocapture-2.ll:222
 ; CHECK-NEXT:    [[TMP1:%.*]] = bitcast i8* [[CALL]] to double*
-; CHECK-NEXT:    [[CALL1:%.*]] = call dereferenceable_or_null(8) i64* @scc_B(double* noalias nofree readnone dereferenceable_or_null(8) "no-capture-maybe-returned" [[TMP1]]) #[[ATTR2]]
+; CHECK-NEXT:    [[CALL1:%.*]] = call dereferenceable_or_null(4) i64* @scc_B(double* noalias nofree readnone dereferenceable_or_null(8) "no-capture-maybe-returned" [[TMP1]]) #[[ATTR2]]
 ; CHECK-NEXT:    [[TMP2:%.*]] = bitcast i64* [[CALL1]] to i32*
----------------
kuter wrote:
> We are getting less dereferenceablity. Isn't this weird ? 
Some of the attributes are dependent on known information and therefore iteration order dependent. We cannot really do much about that without introducing extra cost. That means, sometimes we regress because we reach a fixpoint before some other information is marked as known.

With this patch we propagate the 4 into more places but not the 8, which is worth a FIXME but not necessarily something I'd address here.


================
Comment at: llvm/test/Transforms/Attributor/nocapture-2.ll:270
 ; CHECK-NEXT:    [[TMP2:%.*]] = bitcast i64* [[CALL1]] to i16*
-; CHECK-NEXT:    [[CALL2:%.*]] = call i8* @scc_C(i16* noalias nofree readnone dereferenceable_or_null(8) "no-capture-maybe-returned" [[TMP2]]) #[[ATTR2]]
+; CHECK-NEXT:    [[CALL2:%.*]] = call dereferenceable_or_null(4) i8* @scc_C(i16* noalias nofree readnone dereferenceable_or_null(4) "no-capture-maybe-returned" [[TMP2]]) #[[ATTR2]]
 ; CHECK-NEXT:    br label [[COND_END:%.*]]
----------------
^^^


================
Comment at: llvm/test/Transforms/Attributor/nocapture-2.ll:324
 ; CHECK-NEXT:    [[TMP2:%.*]] = bitcast i8* [[COND]] to i32*
-; CHECK-NEXT:    [[CALL3:%.*]] = call float* @scc_A(i32* noalias nofree readnone dereferenceable_or_null(4) "no-capture-maybe-returned" [[TMP2]]) #[[ATTR2]]
+; CHECK-NEXT:    [[CALL3:%.*]] = call dereferenceable_or_null(4) float* @scc_A(i32* noalias nofree readnone dereferenceable_or_null(4) "no-capture-maybe-returned" [[TMP2]]) #[[ATTR2]]
 ; CHECK-NEXT:    [[TMP3:%.*]] = bitcast float* [[CALL3]] to i8*
----------------
^^^


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103860