[PATCH] D111054: [Attributor] Introduce AAAssumptionInfo to propagate assumptions

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2499
+  getOrCreateAAFor<AAAssumptionInfo>(FPos);
+
   // Every function might be applicable for Heap-To-Stack conversion.
----------------
We need to initialize this for call sites too, at least if they are not direct call sites.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9683
+
+  const AssumptionSet getAssumptionSet() const override { return Assumptions; }
+
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9686
+  bool hasAssumption(const StringRef Assumption) const override {
+    return Assumptions.getAssumptions().contains(Assumption) && isValidState();
+  }
----------------
I would check the cheap part first, so isvalidstate.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9706
+/// direct sucessors. This is conceptually equivalent to computing the dominator
+/// set of the call graph except we merge attributes instead of nodes.
+struct AAAssumptionInfoFunction final : AAAssumptionInfoImpl {
----------------
This makes it sound like a push-scheme while the updatImpl uses a pull-scheme.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9743
+      Assumptions = AssumptionSet(CurrentAssumptions);
+      return indicateOptimisticFixpoint();
+    }
----------------
If the state has "known" and "assumed" assumptions (see also below) you can always fallback to the known ones if you indicate a pessimistic fixpoint. You basically reimplemented parts of the inidicatePessimistic/OptimisticFixpoint stuff here but only partially as part of the state. In line 9742 you do `Assumed = Known`, which is the definition of indicatePessimisticFixpoint in the integer and boolean state. The special case (line 9737/8) is not necessary at all as `Known == Assume == empty` in that case anyway.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9755
+  /// The assumptions already associated with this function.
+  const DenseSet<StringRef> CurrentAssumptions;
+};
----------------
This is basically the "known assumptions" set while `Assumptions` is the "assumed assumptions" set. Maybe use those terms to match the rest of the AAs. We might even need to provide two APIs to query a known and an assumed assumption.
It's also not clear why this is only in the Function AA and not the Impl as call sites can have assumptions too which are not part of the caller or callee.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9788
+    return !Assumptions.empty() ? ChangeStatus::CHANGED
+                                : ChangeStatus::UNCHANGED;
+  }
----------------
I doubt `assumptions.empty()` means something changed. If you return changed and nothing changed you just force a timeout after the max iteration count.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111054