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

[Sanjay Patel via Phabricator via llvm-commits]

spatel 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()))) {
----------------
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


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1794
+    if (Val && match(Val, m_One())) {
+      Value *FrozenX = Builder.CreateFreeze(Op0, Op0->getName() + ".frozen");
+      Value *Cmp = Builder.CreateICmpEQ(FrozenX, Op1);
----------------
This is called FrozenX, but it is freezing Op0. Should it freeze X directly as the name suggests?


================
Comment at: llvm/test/Transforms/InstCombine/urem-via-cmp-select.ll:77
+
+declare void @llvm.assume(i1 noundef)
----------------
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?


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

https://reviews.llvm.org/D143883