[PATCH] D143883: [InstCombine] canonicalize urem as cmp+select

[Allen zhong via Phabricator via llvm-commits]

Allen marked 3 inline comments as done.
Allen added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1789
 
+  // If X / Y == 0, then (X + 1) % Y => (X + 1 == Y) ? 0 : X + 1 .
+  if (match(Op0, m_Add(m_Value(X), m_One()))) {
----------------
spatel wrote:
> This comment doesn't match the code.
> Should it be:
>   // For "(X + 1) % Op1" and if (X u< Op1) => (X + 1) == Op1 ? 0 : X + 1
Thanks, apply it.


================
Comment at: llvm/test/Transforms/InstCombine/urem-via-cmp-select.ll:77
+
+declare void @llvm.assume(i1 noundef)
----------------
spatel wrote:
> nikic wrote:
> > Missing tests where add constant is not 1.
> > 
> > It would be good to also have a test that is not based on assume, where `%x < %n` is known through some other way instead.
> Still no tests with no "assume" call. There should be a way to show the transform using math or bitwise logic ops and/or using a dominating condition?
Add a case **urem_with_dominating_condition**, but now it is not supported, need a following patch to fix


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

https://reviews.llvm.org/D143883