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

[Joseph Huber via Phabricator via llvm-commits]

jhuber6 marked 17 inline comments as done.
jhuber6 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);
 
----------------
jdoerfert wrote:
> Consider passing the set as reference, unsure how much work copying it is
Passing it as a reference would require some external lifetime right? Copy elision should apply in this case, but I'm not sure.


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


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:9802
+    if (getUnion({false, getAssumptions(*AssociatedFunction)}))
+      Changed = ChangeStatus::CHANGED;
+
----------------
jdoerfert wrote:
> This is invariant, so it should happen in the initialize.
The set at some points will still be in the universal state, we only want to add this attribute once it has some known values.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111054