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

[Hal Finkel via llvm-commits]

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

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