[PATCH] D150142: [InstCombine] Add simplifications for div/rem with `i1` operands; PR62607

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked 3 inline comments as done.
goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:1097
   // We don't need to preserve faults!
   if (match(Op1, m_Zero()))
     return PoisonValue::get(Ty);
----------------
floatshadow wrote:
> Can we remove this? I think this might also be covered by `KnownBits::isZero()` check now.
We can, although my feeling is it makes sense to have a fastpath for the common case (knownbits can be expensive).


================
Comment at: llvm/test/Transforms/InstCombine/div-i1.ll:16
+true:
+  %y_true = and i1 %y, 0
+  br label %done
----------------
nikic wrote:
> Is there a more robust way to test this that doesn't rely on worklist order bugs? Something like an assume to zero in an instsimplify test? Or does that also get simplified early?
> Is there a more robust way to test this that doesn't rely on worklist order bugs? Something like an assume to zero in an instsimplify test? Or does that also get simplified early?

It gets simplified early (and is handled correctly now/before). This was the simplest reproduction I could create.


================
Comment at: llvm/test/Transforms/InstCombine/pr62607.ll:86
+  br label %bb_6
+}
----------------
nikic wrote:
> Is this already a minimal reduction per llvm-reduce?
> Is this already a minimal reduction per llvm-reduce?

No, but updated (so yes in v3). Neat tool :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150142