[PATCH] D101423: [InstCombine] Fold overflow bit of [u|s]mul.with.overflow in a poison-safe way

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.
This revision is now accepted and ready to land.

LGTM - see inline for some small improvements.



================
Comment at: llvm/include/llvm/Analysis/OverflowInstAnalysis.h:33
+///   %Op1 = xor i1 %NotOp1, true
+///   %ret = select i1 %Op0, i1 true, i1 %Op1 / %ret = or i1 %Op0, %Op1
+///
----------------
This is confusing if we do not have the context of the callers that we can see in this patch.
The comment should say something like: "Match one of the patterns up to the select/logic op. Callers are expected to align that with the operands of the select/logic op user."


================
Comment at: llvm/include/llvm/Analysis/OverflowInstAnalysis.h:35
+///
+/// IsAnd is set to true if the Op0 and Op1 is used as the first pattern.
+/// If Op0 and Op1 is one of the patterns above, return true and fill Y's use.
----------------
"is used" -> "are used"


================
Comment at: llvm/include/llvm/Analysis/OverflowInstAnalysis.h:36
+/// IsAnd is set to true if the Op0 and Op1 is used as the first pattern.
+/// If Op0 and Op1 is one of the patterns above, return true and fill Y's use.
+
----------------
"is one" -> "match one"


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:5024
+        (match(ValAssumedPoison, m_ExtractValue(m_Specific(II))) ||
+         llvm::find(II->arg_operands(), ValAssumedPoison) != II->arg_end()))
       return true;
----------------
Use "is_contained" ?
Is it possible to separate this change (with an independent test)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101423