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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:2524
+  /// intersections with a "universal" set that contains all elements.
+  struct UniversalSet {
+    /// Creates a universal set with no concrete elements or an empty set.
----------------
Unsure if I'd call this "UniversalSet" as it might not be a universal set ;)


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:2588
+  /// See AbstractState::isValidState()
+  bool isValidState() const override { return !Assumed.isUniversal(); }
+
----------------
Why would it not be valid if assumed is not universal anymore?
One way is to define `Assumed.empty()` as invalid.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:2624
+  bool getIntersection(const UniversalSet &RHS) {
+    Changed = Assumed.getIntersection(RHS);
+    return Changed;
----------------
You should not remove any known assumptions, so remove known from RHS first.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:2642
+
+  bool Changed;
+
----------------
We should not cache "changed" anywhere.


================
Comment at: llvm/lib/IR/Assumptions.cpp:109
+  return true;
+}
+
----------------
The two functions above are equal, make it a local template function and call it from the two exposed ones.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9734
+          *this, IRPosition::function(*ACS.getInstruction()->getFunction()),
+          DepClassTy::REQUIRED);
+      Changed |= getIntersection(AssumptionAA.getAssumed());
----------------
You can/should query the call site AA here. Through inlining and such the call site might have more assumptions than the caller.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9750
+
+    return Changed ? ChangeStatus::CHANGED : ChangeStatus::UNCHANGED;
+  }
----------------
The way this is set up it might signal change even if things did not change. That is bad as it will spin until the iteration limit is reached. Either consider known in the intersect or keep track of the number of assumptions you had before and have now.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9762
+            IRP, A, getAssumptions(cast<CallBase>(IRP.getAssociatedValue())),
+            false) {}
+
----------------
Technically we can add the caller assumptions and the callee assumptions to the known set here if we want. Not needed right now though.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9764
+
+  void initialize(Attributor &A) override { getUnion(getKnown()); }
+
----------------
No need to union something into assumed which is universal at this point.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9779
+    // Add in the assumptions derived for the parent function.
+    getUnion(AssumptionAA->getKnown());
+
----------------
You can add the Assumed here. We are in a fixpoint state but the AA above might not have been informed of that yet.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9793
+    auto &AssumptionAA =
+        A.getAAFor<AAAssumptionInfo>(*this, FnPos, DepClassTy::REQUIRED);
+    return AssumptionAA.hasChanged() ? ChangeStatus::CHANGED
----------------
We should not look at the callee but caller. The callee is known and asked for "hasAssumption" queries anyway. The caller is what might disappear while the call site sticks around. So propagation direction is "caller -> call site -> callee". 


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9795
+    return AssumptionAA.hasChanged() ? ChangeStatus::CHANGED
+                                     : indicatePessimisticFixpoint();
+  }
----------------
This doesn't actually update the state of this AA at all. You want to add the caller assumptions to the call site and indicate a change if that caused the call site to change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111054