[PATCH] D78014: [AssumeBundles] Prevent generation of some redundant assumes

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I think this is fine, if it is not too costly to run these checks. I am not sure if it is worth it at the end of the day given that we will still need a pass to merge and remove assumes as these opportunities become available. I mean, after running the Attributor the information might be tied to arguments which we makes the assume obsolete. After inlining we might have multiple assumes with overlapping information we want to merge. Etc.



================
Comment at: llvm/lib/Analysis/AssumeBundleQueries.cpp:180
+          return RK;
   }
   return RetainedKnowledge::none();
----------------
Nit: `if (!Bundle) continue` to reduce indention or `if (... Bundle = ...)`


================
Comment at: llvm/lib/Analysis/AssumeBundleQueries.cpp:190
                                 return isValidAssumeForContext(I, CtxI, DT);
                               });
 }
----------------
[Is this C++14?]


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:77
   Instruction *InsertBeforeInstruction = nullptr;
-
+  AssumptionCache* AC;
+  DominatorTree* DT = nullptr;
----------------
It should be nullptr or better initialized by the constructor. Same for the other members. There should be a constructor to initialize all of this and nullptrs otherwise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78014