[llvm] r219634 - InstCombine: Don't miscompile (x lshr C1) udiv C2
Timur Iskhodzhanov
timurrrr at google.com
Wed Oct 15 09:32:47 PDT 2014
Oh, I see, thanks!
2014-10-15 20:31 GMT+04:00 David Majnemer <david.majnemer at gmail.com>:
> On Wed, Oct 15, 2014 at 9:13 AM, Timur Iskhodzhanov <timurrrr at google.com>
> wrote:
>>
>> There was a segfault that was probably fixed by r219639. David?
>
>
> Not that I am aware of. r219639 fixed an independent bug that somebody
> filed against InstCombine.
> r219634 caused clang to need to be updated because the APInt API changed,
> this was r219637.
>
> Reid and I both believe that our internal build was somehow flaky and that
> the failure you observed was a red herring or is otherwise unrelated to
> r219634 .
>
>>
>>
>> 2014-10-15 20:10 GMT+04:00 Bob Wilson <bob.wilson at apple.com>:
>> > Are you talking about a performance regression or a correctness problem?
>> >
>> > On Oct 14, 2014, at 5:48 AM, Timur Iskhodzhanov <timurrrr at google.com>
>> > wrote:
>> >
>> > This has regressed on our internal build, I'll send you the details off
>> > the
>> > list.
>> >
>> > 2014-10-14 1:48 GMT+04:00 David Majnemer <david.majnemer at gmail.com>:
>> >>
>> >> Author: majnemer
>> >> Date: Mon Oct 13 16:48:30 2014
>> >> New Revision: 219634
>> >>
>> >> URL: http://llvm.org/viewvc/llvm-project?rev=219634&view=rev
>> >> Log:
>> >> InstCombine: Don't miscompile (x lshr C1) udiv C2
>> >>
>> >> We have a transform that changes:
>> >> (x lshr C1) udiv C2
>> >> into:
>> >> x udiv (C2 << C1)
>> >>
>> >> However, it is unsafe to do so if C2 << C1 discards any of C2's bits.
>> >>
>> >> This fixes PR21255.
>> >>
>> >> Modified:
>> >> llvm/trunk/include/llvm/ADT/APInt.h
>> >> llvm/trunk/lib/Support/APInt.cpp
>> >> llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
>> >> llvm/trunk/test/Transforms/InstCombine/div.ll
>> >>
>> >> Modified: llvm/trunk/include/llvm/ADT/APInt.h
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/APInt.h?rev=219634&r1=219633&r2=219634&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/include/llvm/ADT/APInt.h (original)
>> >> +++ llvm/trunk/include/llvm/ADT/APInt.h Mon Oct 13 16:48:30 2014
>> >> @@ -945,7 +945,8 @@ public:
>> >> APInt sdiv_ov(const APInt &RHS, bool &Overflow) const;
>> >> APInt smul_ov(const APInt &RHS, bool &Overflow) const;
>> >> APInt umul_ov(const APInt &RHS, bool &Overflow) const;
>> >> - APInt sshl_ov(unsigned Amt, bool &Overflow) const;
>> >> + APInt sshl_ov(const APInt &Amt, bool &Overflow) const;
>> >> + APInt ushl_ov(const APInt &Amt, bool &Overflow) const;
>> >>
>> >> /// \brief Array-indexing support.
>> >> ///
>> >>
>> >> Modified: llvm/trunk/lib/Support/APInt.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/APInt.cpp?rev=219634&r1=219633&r2=219634&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/lib/Support/APInt.cpp (original)
>> >> +++ llvm/trunk/lib/Support/APInt.cpp Mon Oct 13 16:48:30 2014
>> >> @@ -2064,19 +2064,29 @@ APInt APInt::umul_ov(const APInt &RHS, b
>> >> return Res;
>> >> }
>> >>
>> >> -APInt APInt::sshl_ov(unsigned ShAmt, bool &Overflow) const {
>> >> - Overflow = ShAmt >= getBitWidth();
>> >> +APInt APInt::sshl_ov(const APInt &ShAmt, bool &Overflow) const {
>> >> + Overflow = ShAmt.uge(getBitWidth());
>> >> if (Overflow)
>> >> - ShAmt = getBitWidth()-1;
>> >> + return APInt(BitWidth, 0);
>> >>
>> >> if (isNonNegative()) // Don't allow sign change.
>> >> - Overflow = ShAmt >= countLeadingZeros();
>> >> + Overflow = ShAmt.uge(countLeadingZeros());
>> >> else
>> >> - Overflow = ShAmt >= countLeadingOnes();
>> >> + Overflow = ShAmt.uge(countLeadingOnes());
>> >>
>> >> return *this << ShAmt;
>> >> }
>> >>
>> >> +APInt APInt::ushl_ov(const APInt &ShAmt, bool &Overflow) const {
>> >> + Overflow = ShAmt.uge(getBitWidth());
>> >> + if (Overflow)
>> >> + return APInt(BitWidth, 0);
>> >> +
>> >> + Overflow = ShAmt.ugt(countLeadingZeros());
>> >> +
>> >> + return *this << ShAmt;
>> >> +}
>> >> +
>> >>
>> >>
>> >>
>> >>
>> >> Modified:
>> >> llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp?rev=219634&r1=219633&r2=219634&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp
>> >> (original)
>> >> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp Mon
>> >> Oct
>> >> 13 16:48:30 2014
>> >> @@ -965,11 +965,17 @@ Instruction *InstCombiner::visitUDiv(Bin
>> >> return Common;
>> >>
>> >> // (x lshr C1) udiv C2 --> x udiv (C2 << C1)
>> >> - if (Constant *C2 = dyn_cast<Constant>(Op1)) {
>> >> + {
>> >> Value *X;
>> >> - Constant *C1;
>> >> - if (match(Op0, m_LShr(m_Value(X), m_Constant(C1))))
>> >> - return BinaryOperator::CreateUDiv(X, ConstantExpr::getShl(C2,
>> >> C1));
>> >> + const APInt *C1, *C2;
>> >> + if (match(Op0, m_LShr(m_Value(X), m_APInt(C1))) &&
>> >> + match(Op1, m_APInt(C2))) {
>> >> + bool Overflow;
>> >> + APInt C2ShlC1 = C2->ushl_ov(*C1, Overflow);
>> >> + if (!Overflow)
>> >> + return BinaryOperator::CreateUDiv(
>> >> + X, ConstantInt::get(X->getType(), C2ShlC1));
>> >> + }
>> >> }
>> >>
>> >> // (zext A) udiv (zext B) --> zext (A udiv B)
>> >>
>> >> Modified: llvm/trunk/test/Transforms/InstCombine/div.ll
>> >> URL:
>> >>
>> >> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/div.ll?rev=219634&r1=219633&r2=219634&view=diff
>> >>
>> >>
>> >> ==============================================================================
>> >> --- llvm/trunk/test/Transforms/InstCombine/div.ll (original)
>> >> +++ llvm/trunk/test/Transforms/InstCombine/div.ll Mon Oct 13 16:48:30
>> >> 2014
>> >> @@ -132,11 +132,11 @@ define i32 @test15(i32 %a, i32 %b) nounw
>> >> }
>> >>
>> >> define <2 x i64> @test16(<2 x i64> %x) nounwind {
>> >> - %shr = lshr <2 x i64> %x, <i64 3, i64 5>
>> >> - %div = udiv <2 x i64> %shr, <i64 4, i64 6>
>> >> + %shr = lshr <2 x i64> %x, <i64 5, i64 5>
>> >> + %div = udiv <2 x i64> %shr, <i64 6, i64 6>
>> >> ret <2 x i64> %div
>> >> ; CHECK-LABEL: @test16(
>> >> -; CHECK-NEXT: udiv <2 x i64> %x, <i64 32, i64 192>
>> >> +; CHECK-NEXT: udiv <2 x i64> %x, <i64 192, i64 192>
>> >> ; CHECK-NEXT: ret <2 x i64>
>> >> }
>> >>
>> >> @@ -264,3 +264,13 @@ define i32 @test30(i32 %a) {
>> >> ; CHECK-LABEL: @test30(
>> >> ; CHECK-NEXT: ret i32 %a
>> >> }
>> >> +
>> >> +define <2 x i32> @test31(<2 x i32> %x) nounwind {
>> >> + %shr = lshr <2 x i32> %x, <i32 31, i32 31>
>> >> + %div = udiv <2 x i32> %shr, <i32 2147483647, i32 2147483647>
>> >> + ret <2 x i32> %div
>> >> +; CHECK-LABEL: @test31(
>> >> +; CHECK-NEXT: %[[shr:.*]] = lshr <2 x i32> %x, <i32 31, i32 31>
>> >> +; CHECK-NEXT: udiv <2 x i32> %[[shr]], <i32 2147483647, i32
>> >> 2147483647>
>> >> +; CHECK-NEXT: ret <2 x i32>
>> >> +}
>> >>
>> >>
>> >> _______________________________________________
>> >> llvm-commits mailing list
>> >> llvm-commits at cs.uiuc.edu
>> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>> >
>> > _______________________________________________
>> > llvm-commits mailing list
>> > llvm-commits at cs.uiuc.edu
>> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> >
>> >
>
>
More information about the llvm-commits
mailing list