[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 ↗

(On Diff #194093)

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 ↗

(On Diff #194101)

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 ↗

(On Diff #194101)

oops. fixed.

[lebedev.ri]

LG

llvm/test/Transforms/InstCombine/div.ll

809 ↗

(On Diff #194107)

test name is @test_exact_nonsw_noexact

[lebedev.ri]

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

1696

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

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

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

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

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

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

[shchenz]

llvm/test/Transforms/InstCombine/div.ll

809 ↗

(On Diff #194107)

fixed.

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

1696

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

llvm/trunk/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/trunk/test/Transforms/InstCombine/div.ll

;
   %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_nsw_exact(i32 %x) {
 ; CHECK-LABEL: @test_exact_nsw_exact(
-; CHECK-NEXT:    [[NEG:%.*]] = sdiv i32 [[X:%.*]], -3
+; CHECK-NEXT:    [[NEG:%.*]] = sdiv exact i32 [[X:%.*]], -3
 ; CHECK-NEXT:    ret i32 [[NEG]]
 ;
   %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:    [[NEG:%.*]] = sdiv <2 x i64> [[X:%.*]], <i64 -3, i64 -4>
+; CHECK-NEXT:    [[NEG:%.*]] = sdiv exact <2 x i64> [[X:%.*]], <i64 -3, i64 -4>
 ; CHECK-NEXT:    ret <2 x i64> [[NEG]]
 ;
   %div = sdiv exact <2 x i64> %x, <i64 3, i64 4>
   %neg = sub nsw <2 x i64> zeroinitializer, %div
   ret <2 x i64> %neg
 }
 
 define i32 @test_exact_nonsw_exact(i32 %x) {
 ; CHECK-LABEL: @test_exact_nonsw_exact(
-; CHECK-NEXT:    [[NEG:%.*]] = sdiv i32 [[X:%.*]], -3
+; CHECK-NEXT:    [[NEG:%.*]] = sdiv exact i32 [[X:%.*]], -3
 ; CHECK-NEXT:    ret i32 [[NEG]]
 ;
   %div = sdiv exact i32 %x, 3
   %neg = sub i32 0, %div
   ret i32 %neg
 }
 
 define i32 @test_exact_nsw_noexact(i32 %x) {
 ; CHECK-LABEL: @test_exact_nsw_noexact(
 ; CHECK-NEXT:    [[NEG:%.*]] = sdiv i32 [[X:%.*]], -3
 ; CHECK-NEXT:    ret i32 [[NEG]]
 ;
   %div = sdiv i32 %x, 3
   %neg = sub nsw i32 0, %div
   ret i32 %neg
 }
 
 define i32 @test_exact_nonsw_noexact(i32 %x) {
 ; CHECK-LABEL: @test_exact_nonsw_noexact(
 ; CHECK-NEXT:    [[NEG:%.*]] = sdiv i32 [[X:%.*]], -3
 ; CHECK-NEXT:    ret i32 [[NEG]]
 ;
   %div = sdiv i32 %x, 3
-  %neg = sub nsw i32 0, %div
+  %neg = sub i32 0, %div
   ret i32 %neg
 }
 
 define i32 @test_exact_div_nonconst(i32 %x, i32 %y) {
 ; CHECK-LABEL: @test_exact_div_nonconst(
 ; CHECK-NEXT:    [[DIV:%.*]] = sdiv exact i32 [[X:%.*]], [[Y:%.*]]
 ; CHECK-NEXT:    [[NEG:%.*]] = sub nsw i32 0, [[DIV]]
 ; CHECK-NEXT:    ret i32 [[NEG]]