[PATCH] D12706: Handle non-constant shifts in computeKnownBits, and use computeKnownBits for constant folding in InstCombine/Simplify
Hal Finkel via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 23 12:28:12 PDT 2015
----- Original Message -----
> From: "Hal Finkel" <hfinkel at anl.gov>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: reviews+D12706+public+8f107d44c62a13e2 at reviews.llvm.org, llvm-commits at lists.llvm.org, regehr at cs.utah.edu, "James
> Molloy" <james at jamesmolloy.co.uk>
> Sent: Friday, October 23, 2015 1:17:26 PM
> Subject: Re: [PATCH] D12706: Handle non-constant shifts in computeKnownBits, and use computeKnownBits for constant
> folding in InstCombine/Simplify
>
> ----- Original Message -----
> > From: "Hal Finkel via llvm-commits" <llvm-commits at lists.llvm.org>
> > To: "James Molloy" <james at jamesmolloy.co.uk>
> > Cc: reviews+D12706+public+8f107d44c62a13e2 at reviews.llvm.org,
> > llvm-commits at lists.llvm.org, regehr at cs.utah.edu
> > Sent: Friday, October 23, 2015 10:28:49 AM
> > Subject: Re: [PATCH] D12706: Handle non-constant shifts in
> > computeKnownBits, and use computeKnownBits for constant
> > folding in InstCombine/Simplify
> >
> > ----- Original Message -----
> > > From: "Hal Finkel" <hfinkel at anl.gov>
> > > To: "James Molloy" <james at jamesmolloy.co.uk>
> > > Cc: llvm-commits at lists.llvm.org, regehr at cs.utah.edu,
> > > reviews+D12706+public+8f107d44c62a13e2 at reviews.llvm.org, "Philip
> > > Reames" <listmail at philipreames.com>, "david majnemer"
> > > <david.majnemer at gmail.com>, sanjoy at playingwithpointers.com
> > > Sent: Thursday, October 22, 2015 9:14:49 AM
> > > Subject: Re: [PATCH] D12706: Handle non-constant shifts in
> > > computeKnownBits, and use computeKnownBits for constant
> > > folding in InstCombine/Simplify
> > >
> > > ----- Original Message -----
> > > > From: "James Molloy" <james at jamesmolloy.co.uk>
> > > > To: reviews+D12706+public+8f107d44c62a13e2 at reviews.llvm.org,
> > > > "Philip Reames" <listmail at philipreames.com>,
> > > > hfinkel at anl.gov, "david majnemer" <david.majnemer at gmail.com>,
> > > > sanjoy at playingwithpointers.com
> > > > Cc: llvm-commits at lists.llvm.org, regehr at cs.utah.edu
> > > > Sent: Thursday, October 22, 2015 8:56:51 AM
> > > > Subject: Re: [PATCH] D12706: Handle non-constant shifts in
> > > > computeKnownBits, and use computeKnownBits for constant
> > > > folding in InstCombine/Simplify
> > > >
> > > >
> > > > Hi Hal,
> > > >
> > > >
> > > > I'm keen to get this committed so I can work on the follow-on
> > > > suggestion I made myself.
> > > >
> > > >
> > > > Will you get time to commit this soon? or is it OK if I commit
> > > > it
> > > > on
> > > > your behalf instead?
> > >
> > > I'm planning on getting to this today. I need to fixup some ubsan
> > > tests for this it seems. If I don't get to this today, I'll let
> > > you
> > > know.
> >
> > To provide a quick update (aside from the fact that, no, I did not
> > end up with time to look at this yesterday), with this patch, a
> > number of ubsan tests fail. The test failures are the same between
> > x86_64 and PPC64, so it's not a backend problem. Furthermore, the
> > compiler output for the tests themselves is unchanged, so the
> > change
> > seems to be in the runtime library.
> >
> > A good example failure is this:
> > test/ubsan/TestCases/Integer/shift.cpp (the first RUN line as an
> > example):
> >
> > // RUN: %clangxx -DLSH_OVERFLOW -DOP='<<' -fsanitize=shift-base
> > -fno-sanitize-recover=shift %s -o %t1 && not %run %t1 2>&1 |
> > FileCheck %s --check-prefix=CHECK-LSH_OVERFLOW
> >
> > Instead of getting this:
> >
> > runtime error: left shift of negative value -2147483648 ...
> >
> > we're getting this (with this patch):
> >
> > runtime error: left shift of 2147483648 by 1 places cannot be
> > represented ...
> >
> > The relevant check in handleShiftOutOfBoundsImpl in
> > ubsan_handlers.cc
> > looks like this:
> >
> > } else if (LHSVal.isNegative()) {
> > R.setErrorType(ErrorType::InvalidShiftBase);
> > Diag(Loc, DL_Error, "left shift of negative value %0") <<
> > LHSVal;
> > } else {
> > R.setErrorType(ErrorType::InvalidShiftBase);
> > Diag(Loc, DL_Error,
> > "left shift of %0 by %1 places cannot be represented in
> > type
> > %2")
> > << LHSVal << RHSVal << Data->LHSType;
> >
> > where we have:
> >
> > /// Is this a negative integer?
> > bool isNegative() const {
> > return getType().isSignedIntegerTy() && getSIntValue() < 0;
> > }
> >
> >
> > and:
> >
> > SIntMax Value::getSIntValue() const {
> > CHECK(getType().isSignedIntegerTy());
> > if (isInlineInt()) {
> > // Val was zero-extended to ValueHandle. Sign-extend from
> > original width
> > // to SIntMax.
> > const unsigned ExtraBits =
> > sizeof(SIntMax) * 8 - getType().getIntegerBitWidth();
> > return SIntMax(Val) << ExtraBits >> ExtraBits;
> > }
> > ...
> >
> > my guess here is that we're miscompiling LHSVal.isNegative()
> > because
> > we somehow believe that 'SIntMax(Val) << ExtraBits >> ExtraBits'
> > can't be negative. If I have time to look at this further today, I
> > will. If someone else would like to help out, please do.
>
> Quick follow-up, with this patch we happily optimize this:
>
> $ cat /tmp/shift.cpp
> __extension__ typedef __int128 s128;
> typedef s128 SIntMax;
> typedef unsigned long uptr;
>
> bool isNegative(uptr Val, unsigned IntegerBitWidth) {
> const unsigned ExtraBits =
> sizeof(SIntMax) * 8 - IntegerBitWidth;
> SIntMax MV = SIntMax(Val) << ExtraBits >> ExtraBits;
> return MV < 0;
> }
>
> to:
>
> define zeroext i1 @_Z10isNegativemj(i64 %Val, i32 zeroext
> %IntegerBitWidth) #0 {
> entry:
> ret i1 false
> }
>
> which seems wrong.
I found the problem; the fix is easy. The commit will include some extra comments and a test case.
-Hal
>
> -Hal
>
> >
> > Thanks again,
> > Hal
> >
> > >
> > > -Hal
> > >
> > > >
> > > >
> > > > Cheers,
> > > >
> > > >
> > > > James
> > > >
> > > >
> > > > On Wed, 14 Oct 2015 at 18:49 Philip Reames via llvm-commits <
> > > > llvm-commits at lists.llvm.org > wrote:
> > > >
> > > >
> > > > reames accepted this revision.
> > > > reames added a comment.
> > > > This revision is now accepted and ready to land.
> > > >
> > > > LGTM w/minor comment addressed
> > > >
> > > > Are you planning on implementing the follow on suggestions? If
> > > > not,
> > > > we should make sure they get tracked either as TODOs or bugs.
> > > >
> > > >
> > > > ================
> > > > Comment at: lib/Analysis/ValueTracking.cpp:990
> > > > @@ +989,3 @@
> > > > +
> > > > + computeKnownBits(I->getOperand(1), KnownZero, KnownOne, DL,
> > > > Depth
> > > > +
> > > > 1, Q);
> > > > +
> > > > ----------------
> > > > It would be more clearly correct to use the two temporaries for
> > > > this
> > > > calculation. The current code is correct, but slightly
> > > > confusing.
> > > >
> > > >
> > > > http://reviews.llvm.org/D12706
> > > >
> > > >
> > > >
> > > > _______________________________________________
> > > > llvm-commits mailing list
> > > > llvm-commits at lists.llvm.org
> > > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> > > >
> > >
> > > --
> > > Hal Finkel
> > > Assistant Computational Scientist
> > > Leadership Computing Facility
> > > Argonne National Laboratory
> > >
> >
> > --
> > Hal Finkel
> > Assistant Computational Scientist
> > Leadership Computing Facility
> > Argonne National Laboratory
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
--
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory
More information about the llvm-commits
mailing list