[PATCH] D12706: Handle non-constant shifts in computeKnownBits, and use computeKnownBits for constant folding in InstCombine/Simplify

Fri Oct 23 11:17:26 PDT 2015

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

 -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