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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Initial comments.



================
Comment at: llvm/include/llvm/Transforms/Utils/AssumeBundleBuilder.h:45
 
+IntrinsicInst *buildAssumeFromKnowledge(ArrayRef<RetainedKnowledge> Knowledge,
+                                        Instruction *InsertPoint,
----------------
Please add some documentation here.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:4230
+    if (match(Next, m_Intrinsic<Intrinsic::assume>(m_Specific(IIOperand))))
+      return RemoveConditionFromAssume(Next);
 
----------------
I don't get this. Doesn't it replace the operand of an assume with false if there is one with the same condition following? That seems wrong, maybe "true" is what you wanted?


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:4253
 
+    CmpInst::Predicate Pred;
+    if (EnableKnowledgeRetention &&
----------------
Comments on what is happening here and below please.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp:4265
+                   ConstantInt::getFalse(II->getContext()));
+      return RemoveConditionFromAssume(II);
+    }
----------------
Isn't this call the same as the 5 lines before?


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:291
+  return Builder.build();
+}
+
----------------
It is odd that this takes an insertion point but apparently does not insert the instruction. Can we change either to confuse me less?


================
Comment at: llvm/test/Transforms/InstCombine/assume.ll:253
 ;
 ; SAME-LABEL: @bar4(
 ; SAME-NEXT:  entry:
----------------
Why do we have SAME checking the function twice? Is this a bug in the update script?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82703