[Attributor][NFC] Connect AAPotentialValues with AAValueSimplify

[okura]

Summary

This patch enables AAValueSimplify to use information from AAPotentialValues

[jdoerfert]

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

4493

I'm not sure we need/want this failAll flag. Can't we just return true, given that ::None is the right simplified value in that case? With the current setup we will always call AAPotentialValues, even if the range analysis would determine a constant result eventually.

[okura]

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

4493

Can't we just return true, given that ::None is the right simplified value in that case?

I thought llvm::None is a weaker result than concrete ConstantInt values, but that was my mistake. I'll fix this.

[okura]

• fix the places that were pointed out

[jdoerfert]

LGTM, some minor nits to consider.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

4488

Nit: Even better if we add a dependence only if we use the result, in either case, OPTIONAL is the right kind :)

4502

You don't need an else after return, that should bring all the indention to a single level here.

4508

early exits:

if (ask...)
  return true;
if (ask...)
  return true;
return false;

[okura]

• fix nits
• modify tests
• move changes in PotentialValuesState from D85632

[okura]

I'm sorry for forgetting to include changes in tests. There are changes in the number of iterations only.
And I moved PotentialValuesState's change to this because the bug in there causes a wrong value simplification.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp

static bool checkAndUpdate(Attributor &A, const AbstractAttribute &QueryingAA,
     LLVM_DEBUG(dbgs() << "[ValueSimplify] " << QueryingValue
                       << " is assumed to be "
                       << QueryingValueSimplifiedUnwrapped << "\n");
 
     AccumulatedSimplifiedValue = QueryingValueSimplified;
     return true;
   }
 
-  bool askSimplifiedValueForAAValueConstantRange(Attributor &A) {
+  /// Returns a candidate is found or not
+  /// \p failAll is true when nullptr is returned by all other AAs
+  /// If at least one AA returns llvm::None, a candidate may be found in later
+  /// iteration.
+  template <typename AAType>
+  bool askSimplifiedValueFor(Attributor &A, bool &failAll) {
     if (!getAssociatedValue().getType()->isIntegerTy())
       return false;
 
-    const auto &ValueConstantRangeAA =
-        A.getAAFor<AAValueConstantRange>(*this, getIRPosition());
+    const auto &AA =
+        A.getAAFor<AAType>(*this, getIRPosition(), /* TrackDependence */ true,
+                           DepClassTy::OPTIONAL);
 
-    Optional<ConstantInt *> COpt =
-        ValueConstantRangeAA.getAssumedConstantInt(A);
-    if (COpt.hasValue()) {
-      if (auto *C = COpt.getValue())
-        SimplifiedAssociatedValue = C;
-      else
-        return false;
+    Optional<ConstantInt *> COpt = AA.getAssumedConstantInt(A);
+
+    if (!COpt.hasValue()) {
+      failAll = false;
     } else {
-      SimplifiedAssociatedValue = llvm::None;
-    }
+      if (auto *C = COpt.getValue()) {
+        SimplifiedAssociatedValue = C;
+        return true;
+      } else {
+        return false;
+      }
+    }
+
+  bool askSimplifiedValueForOtherAAs(Attributor &A) {
+    bool failAll = true;
+    SimplifiedAssociatedValue = llvm::None;
+    if (askSimplifiedValueFor<AAValueConstantRange>(A, failAll))
+      return true;
+    if (askSimplifiedValueFor<AAPotentialValues>(A, failAll))
       return true;
+    return !failAll;
   }
 
   /// See AbstractAttribute::manifest(...).
   ChangeStatus manifest(Attributor &A) override {
     ChangeStatus Changed = ChangeStatus::UNCHANGED;
 
     if (SimplifiedAssociatedValue.hasValue() &&
         !SimplifiedAssociatedValue.getValue())
auto PredForCallSite = [&](AbstractCallSite ACS) {
           if (C->isThreadDependent())
             return false;
       return checkAndUpdate(A, *this, ArgOp, SimplifiedAssociatedValue);
     };
 
     bool AllCallSitesKnown;
     if (!A.checkForAllCallSites(PredForCallSite, *this, true,
                                 AllCallSitesKnown))
-      if (!askSimplifiedValueForAAValueConstantRange(A))
+      if (!askSimplifiedValueForOtherAAs(A))
         return indicatePessimisticFixpoint();
 
     // If a candicate was found in this update, return CHANGED.
     return HasValueBefore == SimplifiedAssociatedValue.hasValue()
                ? ChangeStatus::UNCHANGED
                : ChangeStatus ::CHANGED;
   }
 
struct AAValueSimplifyReturned : AAValueSimplifyImpl {
   ChangeStatus updateImpl(Attributor &A) override {
     bool HasValueBefore = SimplifiedAssociatedValue.hasValue();
 
     auto PredForReturned = [&](Value &V) {
       return checkAndUpdate(A, *this, V, SimplifiedAssociatedValue);
     };
 
     if (!A.checkForAllReturnedValues(PredForReturned, *this))
-      if (!askSimplifiedValueForAAValueConstantRange(A))
+      if (!askSimplifiedValueForOtherAAs(A))
         return indicatePessimisticFixpoint();
 
     // If a candicate was found in this update, return CHANGED.
     return HasValueBefore == SimplifiedAssociatedValue.hasValue()
                ? ChangeStatus::UNCHANGED
                : ChangeStatus ::CHANGED;
   }
 
auto VisitValueCB = [&](Value &V, const Instruction *CtxI, bool &,
       }
       return checkAndUpdate(A, *this, V, SimplifiedAssociatedValue);
     };
 
     bool Dummy = false;
     if (!genericValueTraversal<AAValueSimplify, bool>(
             A, getIRPosition(), *this, Dummy, VisitValueCB, getCtxI(),
             /* UseValueSimplify */ false))
-      if (!askSimplifiedValueForAAValueConstantRange(A))
+      if (!askSimplifiedValueForOtherAAs(A))
         return indicatePessimisticFixpoint();
 
     // If a candicate was found in this update, return CHANGED.
 
     return HasValueBefore == SimplifiedAssociatedValue.hasValue()
                ? ChangeStatus::UNCHANGED
                : ChangeStatus ::CHANGED;
   }