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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/IR/Assumptions.h:60
+/// Return the set of all assumptions for the call \p CB.
+DenseSet<StringRef> getAssumptions(const CallBase &CB);
 
----------------
Consider passing the set as reference, unsure how much work copying it is


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:4617
+
+  using AssumptionSet = std::pair<bool, DenseSet<StringRef>>;
+
----------------
I don't like the pair approach for long lived data. Make it a struct with properly named members. Costs the same but is much easier to read.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:4622
+  /// Returns true if the assumption set contains the assumption \p Assumption.
+  virtual bool hasAttribute(const StringRef Assumption) const = 0;
+
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:15
+#include "llvm/ADT/SetOperations.h"
+#include "llvm/IR/Assumptions.h"
 #include "llvm/Transforms/IPO/Attributor.h"
----------------
Style: move down to the block of includes


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9703
+    // assumptions. This is only quantified once we know the call graph entry.
+    Assumptions = AssumptionSet({true, DenseSet<StringRef>()});
+  }
----------------
This should be in the constructor. Initialize can and should already look at the underlying value.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9756
+    Function *AssociatedFunction =
+        this->getIRPosition().getAssociatedFunction();
+
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9759
+    // Add assumptions to the function associated with this AA.
+    LLVMContext &Ctx = AssociatedFunction->getParent()->getContext();
+    if (!Assumptions.second.empty())
----------------



================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9764
+                               llvm::join(Assumptions.second.begin(),
+                                          Assumptions.second.end(), ",")));
+
----------------
Does this overwrite an existing assumptions attribute? Do we have a test for that?


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9766
+
+    return ChangeStatus::UNCHANGED;
+  }
----------------
Adding assumptions is a change. Maybe return if it's empty with unchanged and otherwise add + CHANGED.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9769-9772
+  /// See AbstractAttribute::initialize(...).
+  void initialize(Attributor &A) override {
+    AAAssumptionInfoImpl::initialize(A);
+  }
----------------
Not needed. If you just redirect to the base class, remove it.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9779
+    const Function *AssociatedFunction =
+        this->getIRPosition().getAssociatedFunction();
+
----------------
No this, also elsewhere.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9785
+          DepClassTy::REQUIRED);
+      return !getIntersection(AssumptionAA.getAssumptionSet());
+    };
----------------
getIntersection returns if a change was made. CallSitePred should return false only if the traversal of call sites failed and we should give up. I think the return value of the CB here should be: 
`NotUniversal && AssumptionSet.empty()` or something like that.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9791
+    if (!A.checkForAllCallSites(CallSitePred, *this, true, AllCallSitesKnown))
+      Changed = ChangeStatus::CHANGED;
+
----------------
This is not how checkForAllXXXX work. If they return false it means no all XXXX have been visited and you cannot rely on the information gathered.
Basically `if (!checkForAllCXXXX(...)) return indicatePessimisticFixpoint(); ` is the way to go almost always.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9798
+      return indicateOptimisticFixpoint();
+    }
+
----------------
This won't be necessary because checkForAllCallSites returns false if you asked for *all* and not all call be visited.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9802
+    if (getUnion({false, getAssumptions(*AssociatedFunction)}))
+      Changed = ChangeStatus::CHANGED;
+
----------------
This is invariant, so it should happen in the initialize.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9841
+    return ChangeStatus::UNCHANGED;
+  }
+
----------------
Same comments as in the other manifest.


================
Comment at: llvm/test/Transforms/Attributor/assumes_info.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --function-signature --check-attributes --check-globals
+; RUN: opt -passes=attributor -attributor-manifest-internal -attributor-max-iterations-verify -attributor-max-iterations=2 -S < %s | FileCheck %s
+declare void @call()
----------------
Use the same run lines as other tests (4 total)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111054