[InstCombine] sdiv exact flag fixup

[shchenz]

Summary

This is a issue found when investigate https://reviews.llvm.org/D60395.
exact flag is missing when opt combine 0 - (X sdiv C) -> (X sdiv -C)

[lebedev.ri]

Looks good, but a question re tests.

----------------------------------------
Optimization: zz
Precondition: ((C != 1) && (C != -128))
  %div = sdiv exact i8 %x, C
  %r = sub i8 0, %div
=>
  %r = sdiv exact i8 %x, -C

Done: 1
Optimization is correct!

llvm/test/Transforms/InstCombine/div.ll

763

Please add more tests.

• Variant with nsw, but without exact
• Variant without nsw, but with exact
• Variant without nsw, and without exact
• ???
• Are there tests with C = 1, C = MinSigned ?

[shchenz]

address comments

[lebedev.ri]

please precommit the tests

llvm/test/Transforms/InstCombine/div.ll

783

tests are faulty, contain nsw despite the name

[shchenz]

address Roman's comments.

[shchenz]

Thanks for your comments Roman @lebedev.ri . Updated.

llvm/test/Transforms/InstCombine/div.ll

783

oops. fixed.

[lebedev.ri]

LG

llvm/test/Transforms/InstCombine/div.ll

781

test name is @test_exact_nonsw_noexact

[lebedev.ri]

llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp

1696 ↗

(On Diff #194116)

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

[spatel]

llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp

1696 ↗

(On Diff #194116)

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.

[lebedev.ri]

llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp

1696 ↗

(On Diff #194116)

Note that *this* patch only fixed propagation of exact.
I'm wondering if the entire fold is broken for vectors w/undef elts.

[lebedev.ri]

llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp

1696 ↗

(On Diff #194116)

... 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.

[spatel]

llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp

1696 ↗

(On Diff #194116)

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?

[lebedev.ri]

llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp

1696 ↗

(On Diff #194116)

Filed https://bugs.llvm.org/show_bug.cgi?id=41425

[shchenz]

llvm/test/Transforms/InstCombine/div.ll

781

fixed.

llvm/trunk/lib/Transforms/InstCombine/InstCombineAddSub.cpp

1696 ↗

(On Diff #194116)

thanks for pointing out this bug. I am afraid I don't have time for this issue now.

llvm/lib/Transforms/InstCombine/InstCombineAddSub.cpp

if (Op1->hasOneUse()) {
 
     // (X - (X & Y))   -->   (X & ~Y)
     if (match(Op1, m_c_And(m_Value(Y), m_Specific(Op0))))
       return BinaryOperator::CreateAnd(Op0,
                                   Builder.CreateNot(Y, Y->getName() + ".not"));
 
     // 0 - (X sdiv C)  -> (X sdiv -C)  provided the negation doesn't overflow.
     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));
+      BO->setIsExact(cast<BinaryOperator>(Op1)->isExact());
+      return BO;
+    }
 
     // 0 - (X << Y)  -> (-X << Y)   when X is freely negatable.
     if (match(Op1, m_Shl(m_Value(X), m_Value(Y))) && match(Op0, m_Zero()))
       if (Value *XNeg = dyn_castNegVal(X))
         return BinaryOperator::CreateShl(XNeg, Y);
 
     // Subtracting -1/0 is the same as adding 1/0:
     // sub [nsw] Op0, sext(bool Y) -> add [nsw] Op0, zext(bool Y)

llvm/test/Transforms/InstCombine/div.ll

 ; CHECK-NEXT:    ret <2 x i8> [[C]]
 ;
   %a = mul nuw <2 x i8> %z, %x
   %b = mul <2 x i8> %z, %y
   %c = udiv <2 x i8> %a, %b
   ret <2 x i8> %c
 }
 
 define i32 @test_exact(i32 %x) {
 ; CHECK-LABEL: @test_exact(
-; CHECK-NEXT:    [[DIV:%.*]] = sdiv i32 [[X:%.*]], -3
+; CHECK-NEXT:    [[DIV:%.*]] = sdiv exact i32 [[X:%.*]], -3
 ; CHECK-NEXT:    ret i32 [[DIV]]
 ;
   %div = sdiv exact i32 %x, 3
   %neg = sub nsw i32 0, %div
   ret i32 %neg
 }
 
 define <2 x i64> @test_exact_vec(<2 x i64> %x) {
 ; CHECK-LABEL: @test_exact_vec(
-; CHECK-NEXT:    [[DIV:%.*]] = sdiv <2 x i64> [[X:%.*]], <i64 -3, i64 -4>
+; CHECK-NEXT:    [[DIV:%.*]] = sdiv exact <2 x i64> [[X:%.*]], <i64 -3, i64 -4>
 ; CHECK-NEXT:    ret <2 x i64> [[DIV]]
 ;
   %div = sdiv exact <2 x i64> %x, <i64 3, i64 4>
   %neg = sub nsw <2 x i64> zeroinitializer, %div
   ret <2 x i64> %neg
 }