[PATCH] D60396: [InstCombine] sdiv exact flag fixup

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp:1696
     if (match(Op1, m_SDiv(m_Value(X), m_Constant(C))) && match(Op0, m_Zero()) &&
-        C->isNotMinSignedValue() && !C->isOneValue())
-      return BinaryOperator::CreateSDiv(X, ConstantExpr::getNeg(C));
+        C->isNotMinSignedValue() && !C->isOneValue()) {
+      auto *BO = BinaryOperator::CreateSDiv(X, ConstantExpr::getNeg(C));
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > spatel wrote:
> > > lebedev.ri wrote:
> > > > Uhm.
> > > > So, this is not related to this patch specifically, but i think `isOneValue()` check is incorrect,
> > > > and //may// be causing miscompiles: https://godbolt.org/z/CmAslh <- i don't think `@test_exact_vec` should be folded?
> > > > I think you want to add a `isNotOneValue()`.
> > > > Also, is this fold valid for `undef` elements?
> > > > 
> > > > CC @spatel 
> > > Yes, if this patch has not been reverted yet, it probably should be.
> > > 
> > > It's not correct for arbitrary vector constants. We can either limit this to splat constants (m_APInt) or do the more involved element-by-element check.
> > > 
> > > If the divisor has an undef element, we have immediate UB, so anything goes. But we might want to do something better? I haven't looked at this patch closely yet.
> > Note that *this* patch only fixed propagation of `exact`.
> > I'm wondering if the entire fold is broken for vectors w/`undef` elts.
> ... and it won't be possible to revert the original patch, since it is here for 9+ years.
> So i think this fold should be fixed instead.
Ah, sorry - didn't see it clearly the 1st time. Yes, I agree the bug was already there, so we just need to fix it as a follow-up.

Do you or @shchenz want to write that patch?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D60396