[PATCH] D82703: [InstCombine] convert assumes to operand bundles

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Analysis/AssumeBundleQueries.h:111
+    return ArgValue < Other.ArgValue;
+  }
   operator bool() const { return AttrKind != Attribute::None; }
----------------
This is not a strict order. Let's use AttrKind and WasOn as well.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:94
 
+extern cl::opt<bool> EnableKnowledgeRetention;
+
----------------
Add a sentence what this controls/means.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:4229
+    auto RemoveConditionFromAssume = [&](Instruction *Assume) -> Instruction * {
+      assert(isa<IntrinsicInst>(Assume));
+      if (isAssumeWithEmptyBundle(*cast<IntrinsicInst>(II)))
----------------
I'd add a message to the assert and a comment before the lambda.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:4299
+          /// Note: this doesn't preserve the offset information but merges
+          /// offset and alignememt.
+          RetainedKnowledge RK{Attribute::Alignment,
----------------
Shouldn't we build a GEP for the offset instead, if the offset is not a multiple of the alignment that is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82703