[llvm] r197705 - Improved fix for PR17827 (instcombine of shift/and/compare).
Kay Tiong Khoo
kkhoo at perfwizard.com
Thu Dec 19 10:45:40 PST 2013
Thanks for pointing that out, Hal. Removed links in r197713. Previously
referenced Z3 programs pasted into bug report.
On Thu, Dec 19, 2013 at 11:24 AM, Hal Finkel <hfinkel at anl.gov> wrote:
> ----- Original Message -----
> > From: "Kay Tiong Khoo" <kkhoo at perfwizard.com>
> > To: llvm-commits at cs.uiuc.edu
> > Sent: Thursday, December 19, 2013 12:07:17 PM
> > Subject: [llvm] r197705 - Improved fix for PR17827 (instcombine of
> shift/and/compare).
> >
> > Author: kkhoo
> > Date: Thu Dec 19 12:07:17 2013
> > New Revision: 197705
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=197705&view=rev
> > Log:
> > Improved fix for PR17827 (instcombine of shift/and/compare).
> >
> > This change fixes the case of arithmetic shift right - do not attempt
> > to fold that case.
> > This change also relaxes the conditions when attempting to fold the
> > logical shift right and shift left cases.
> >
> > No additional IR-level test cases included at this time. See
> > http://llvm.org/bugs/show_bug.cgi?id=17827 for proofs that these are
> > correct transformations.
> >
> > Modified:
> > llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
> >
> > Modified:
> > llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
> > URL:
> >
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp?rev=197705&r1=197704&r2=197705&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp
> > (original)
> > +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCompares.cpp Thu
> > Dec 19 12:07:17 2013
> > @@ -1195,33 +1195,43 @@ Instruction *InstCombiner::visitICmpInst
> > ConstantInt *ShAmt;
> > ShAmt = Shift ? dyn_cast<ConstantInt>(Shift->getOperand(1)) :
> > 0;
> >
> > - // We can fold this as long as we can't shift unknown bits
> > - // into the mask. This can happen with signed shift
> > - // rights, as they sign-extend. With logical shifts,
> > - // we must still make sure the comparison is not signed
> > - // because we are effectively changing the
> > - // position of the sign bit (PR17827).
> > - // TODO: We can relax these constraints a bit more.
> > + // This seemingly simple opportunity to fold away a shift
> > turns out to
> > + // be rather complicated. See PR17827
> > + // ( http://llvm.org/bugs/show_bug.cgi?id=17827 ) for details.
> > if (ShAmt) {
> > bool CanFold = false;
> > unsigned ShiftOpcode = Shift->getOpcode();
> > if (ShiftOpcode == Instruction::AShr) {
> > - // To test for the bad case of the signed shr, see if any
> > - // of the bits shifted in could be tested after the mask.
> > - Type *ShiftType = Shift->getType();
> > - Type *AndType = AndCst->getType();
> > -
> > - unsigned ShiftBitWidth =
> > ShiftType->getPrimitiveSizeInBits();
> > - unsigned AndBitWidth = AndType->getPrimitiveSizeInBits();
> > -
> > - int ShAmtVal = ShiftBitWidth -
> > ShAmt->getLimitedValue(ShiftBitWidth);
> > -
> > - if ((APInt::getHighBitsSet(AndBitWidth, AndBitWidth -
> > ShAmtVal) &
> > - AndCst->getValue()) == 0)
> > + // There may be some constraints that make this possible,
> > + // but nothing simple has been discovered yet.
> > + CanFold = false;
> > + } else if (ShiftOpcode == Instruction::Shl) {
> > + // For a left shift, we can fold if the comparison is not
> > signed.
> > + // We can also fold a signed comparison if the mask value
> > and
> > + // comparison value are not negative. These constraints
> > may not be
> > + // obvious, but we can prove that they are correct using
> > an SMT
> > + // solver such as "Z3" :
> > + // http://rise4fun.com/Z3/DyMp
>
> Kay,
>
> While I'm a big fan of SMT solvers, I don't think putting these kinds in
> the source code is a good idea.
>
> 1. While Z3 is a great SMT solver, it is specifically made available for
> personal and non-commercial use only (see http://rise4fun.com/termsofuse),
> and we have a lot of contributors and users for whom that is an issue.
>
> 2. Who knows what the lifetime of those 'permalinks' will actually be. We
> should record the relevant details in some place that we control.
>
> Please record whatever input or output is relevant in the bug report, and
> remove the references to Z3 and the links from the source code.
>
> Thanks again,
> Hal
>
> > + if (!ICI.isSigned() || (!AndCst->isNegative() &&
> > !RHS->isNegative()))
> > + CanFold = true;
> > + } else if (ShiftOpcode == Instruction::LShr) {
> > + // For a logical right shift, we can fold if the
> > comparison is not
> > + // signed. We can also fold a signed comparison if the
> > shifted mask
> > + // value and the shifted comparison value are not
> > negative.
> > + // These constraints may not be obvious, but we can prove
> > that they
> > + // are correct using an SMT solver such as "Z3" :
> > + // http://rise4fun.com/Z3/Tslfh
> > + if (!ICI.isSigned())
> > CanFold = true;
> > - } else if (ShiftOpcode == Instruction::Shl ||
> > - ShiftOpcode == Instruction::LShr) {
> > - CanFold = !ICI.isSigned();
> > + else {
> > + ConstantInt *ShiftedAndCst =
> > + cast<ConstantInt>(ConstantExpr::getShl(AndCst,
> > ShAmt));
> > + ConstantInt *ShiftedRHSCst =
> > + cast<ConstantInt>(ConstantExpr::getShl(RHS, ShAmt));
> > +
> > + if (!ShiftedAndCst->isNegative() &&
> > !ShiftedRHSCst->isNegative())
> > + CanFold = true;
> > + }
> > }
> >
> > if (CanFold) {
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
> --
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131219/e19976f4/attachment.html>
More information about the llvm-commits
mailing list