[PATCH] D76149: [AssumeBundles] Use assume bundles in isKnownNonZero

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Analysis/AssumeBundleQueries.h:18
 #include "llvm/IR/Attributes.h"
-#include "llvm/IR/Instructions.h"
+#include "llvm/IR/Instruction.h"
+#include "llvm/IR/PassManager.h"
----------------
Nothing from the header seems to be actually used. Forward declare Instruction?



================
Comment at: llvm/include/llvm/Analysis/AssumeBundleQueries.h:19
+#include "llvm/IR/Instruction.h"
+#include "llvm/IR/PassManager.h"
+#include "llvm/IR/Dominators.h"
----------------
I could not find anything that uses declarations from PassManager.h. Did I miss something?


================
Comment at: llvm/include/llvm/Analysis/AssumeBundleQueries.h:20
+#include "llvm/IR/PassManager.h"
+#include "llvm/IR/Dominators.h"
 #include "llvm/ADT/DenseMap.h"
----------------
Nothing from the header seems to be actually used. Forward declare DominatorTree?


================
Comment at: llvm/include/llvm/Analysis/AssumeBundleQueries.h:104
 /// Represent one information held inside an operand bundle of an llvm.assume.
 /// AttrKind is the property that hold.
 /// WasOn if not null is that Value for which AttrKind holds.
----------------
unrelated nit: property that holds?


================
Comment at: llvm/include/llvm/Analysis/AssumeBundleQueries.h:106
 /// WasOn if not null is that Value for which AttrKind holds.
 /// ArgValue is optionally an argument.
 struct RetainedKnowledge {
----------------
unrelated, but it is not clear from the comment what ArgValue is referring to. Might be good to clarify


================
Comment at: llvm/include/llvm/Analysis/AssumeBundleQueries.h:142
 
+/// return a valid Knowledge associated to the Use U if its Attribute kind is
+/// in AttrKinds
----------------
nit: Return


================
Comment at: llvm/include/llvm/Analysis/AssumeBundleQueries.h:143
+/// return a valid Knowledge associated to the Use U if its Attribute kind is
+/// in AttrKinds
+RetainedKnowledge getKnowledgeFromUse(const Use *U,
----------------
nit: period at end of sentence.


================
Comment at: llvm/include/llvm/Analysis/AssumeBundleQueries.h:147
+
+/// return a valid Knowledge associated to the Value V if its Attribute kind is
+/// in AttrKinds and it matches the Filter.
----------------
nit: Return


================
Comment at: llvm/include/llvm/Analysis/AssumeBundleQueries.h:152
+    AssumptionCache *AC = nullptr,
+    llvm::function_ref<bool(RetainedKnowledge, Instruction *)> Filter =
+        [](RetainedKnowledge, Instruction *) { return true; });
----------------
llvm:: not needed?


================
Comment at: llvm/include/llvm/Analysis/AssumeBundleQueries.h:155
+
+/// return a valid Knowledge associated to the Value V if its Attribute kind is
+/// in AttrKinds and the knowledge is suitable to be used in the context of
----------------
nit: Return


================
Comment at: llvm/lib/Analysis/AssumeBundleQueries.cpp:133
+                          ArrayRef<Attribute::AttrKind> AttrKinds) {
+  auto *Intr = dyn_cast<IntrinsicInst>(U->getUser());
+  if (!match(U->getUser(),
----------------
It would be slightly better to just use cast<> after the check I think.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:19
 #include "llvm/ADT/Statistic.h"
+#include "llvm/Analysis/AssumeBundleQueries.h"
+#include "llvm/Analysis/CallGraph.h"
----------------
all new includes unused?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76149