[llvm] r222428 - Revert r222416, r222422, r222426: the former revision had problems and fixing them introduced bugs
Timur Iskhodzhanov
timurrrr at google.com
Thu Nov 20 07:50:07 PST 2014
I believe I did.
On Thu Nov 20 2014 at 6:29:56 PM Mehdi Amini <mehdi.amini at apple.com> wrote:
> Hi,
>
> I would expect that you shoot me an email when you revert my changes.
> And I would also expect some informations on the mentioned “bug”. Your
> commit message is far too laconic.
>
> Thanks
>
> Mehdi
>
>
>
> > On Nov 20, 2014, at 4:36 AM, Timur Iskhodzhanov <timurrrr at google.com>
> wrote:
> >
> > Author: timurrrr
> > Date: Thu Nov 20 06:36:43 2014
> > New Revision: 222428
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=222428&view=rev
> > Log:
> > Revert r222416, r222422, r222426: the former revision had problems and
> fixing them introduced bugs
> >
> > Modified:
> > llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
> >
> > Modified: llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Transforms/Utils/SimplifyCFG.cpp?rev=222428&r1=222427&r2=222428&view=diff
> > ============================================================
> ==================
> > --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
> > +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Thu Nov 20 06:36:43
> 2014
> > @@ -357,173 +357,160 @@ static ConstantInt *GetConstantInt(Value
> > return nullptr;
> > }
> >
> > -/// Given a chain of or (||) or and (&&) comparison of a value against a
> > -/// constant, this will try to recover the information required for a
> switch
> > -/// structure.
> > -/// It will depth-first traverse the chain of comparison, seeking for
> patterns
> > -/// like %a == 12 or %a < 4 and combine them to produce a set of integer
> > -/// representing the different cases for the switch.
> > -/// Note that if the chain is composed of '||' it will build the set of
> elements
> > -/// that matches the comparisons (i.e. any of this value validate the
> chain)
> > -/// while for a chain of '&&' it will build the set elements that make
> the test
> > -/// fail.
> > -struct ConstantComparesGatherer {
> > -
> > - Value *CompValue; /// Value found for the switch comparison
> > - Value *Extra; /// Extra clause to be checked before the switch
> > - SmallVector<ConstantInt *, 8> Vals; /// Set of integers to match in
> switch
> > - unsigned UsedICmps; /// Number of comparisons matched in the and/or
> chain
> > -
> > - /// Construct and compute the result for the comparison instruction
> Cond
> > - ConstantComparesGatherer(Instruction *Cond, const DataLayout *DL)
> > - : CompValue(nullptr), Extra(nullptr), UsedICmps(0) {
> > - gather(Cond, DL);
> > - }
> > -
> > - /// Prevent copy
> > - ConstantComparesGatherer(const ConstantComparesGatherer &)
> > - LLVM_DELETED_FUNCTION;
> > - ConstantComparesGatherer &
> > - operator=(const ConstantComparesGatherer &) LLVM_DELETED_FUNCTION;
> > -
> > -private:
> > -
> > - /// Try to set the current value used for the comparison, it succeeds
> only if
> > - /// it wasn't set before or if the new value is the same as the old
> one
> > - bool setValueOnce(Value *NewVal) {
> > - if(CompValue && CompValue != NewVal) return false;
> > - return CompValue == NewVal;
> > - }
> > -
> > - /// Try to match Instruction "I" as a comparison against a constant
> and
> > - /// populates the array Vals with the set of values that match (or do
> not
> > - /// match depending on isEQ).
> > - /// Return false on failure. On success, the Value the comparison
> matched
> > - /// against is placed in CompValue.
> > - /// If CompValue is already set, the function is expected to fail if
> a match
> > - /// is found but the value compared to is different.
> > - bool matchInstruction(Instruction *I, const DataLayout *DL, bool
> isEQ) {
> > - // If this is an icmp against a constant, handle this as one of the
> cases.
> > - ICmpInst *ICI;
> > - ConstantInt *C;
> > - if (!((ICI = dyn_cast<ICmpInst>(I)) &&
> > - (C = GetConstantInt(I->getOperand(1), DL)))) {
> > - return false;
> > - }
> > -
> > - Value *RHSVal;
> > - ConstantInt *RHSC;
> >
> > - // Pattern match a special case
> > - // (x & ~2^x) == y --> x == y || x == y|2^x
> > - // This undoes a transformation done by instcombine to fuse 2
> compares.
> > - if (ICI->getPredicate() == (isEQ ? ICmpInst::ICMP_EQ:ICmpInst::ICMP_NE))
> {
> > - if (match(ICI->getOperand(0),
> > - m_And(m_Value(RHSVal), m_ConstantInt(RHSC)))) {
> > - APInt Not = ~RHSC->getValue();
> > - if (Not.isPowerOf2()) {
> > - // If we already have a value for the switch, it has to match!
> > - if(!setValueOnce(RHSVal))
> > - return false;
> > -
> > - Vals.push_back(C);
> > - Vals.push_back(ConstantInt::get(C->getContext(),
> > - C->getValue() | Not));
> > - UsedICmps++;
> > - return true;
> > - }
> > - }
> >
> > - // If we already have a value for the switch, it has to match!
> > - if(!setValueOnce(ICI->getOperand(0)))
> > - return false;
> > +// Try to match Instruction I as a comparison against a constant and
> populates
> > +// Vals with the set of value that match (or does not depending on
> isEQ).
> > +// Return nullptr on failure, or return the Value the comparison
> matched against
> > +// on success
> > +// CurrValue, if supplied, is the value we want to match against. The
> function
> > +// is expected to fail if a match is found but the value compared to is
> not the
> > +// one expected. If CurrValue is supplied, the return value has to be
> either
> > +// nullptr or CurrValue
> > +static Value* GatherConstantComparesMatch(Instruction *I,
> > + Value *CurrValue,
> > + SmallVectorImpl<ConstantInt*>
> &Vals,
> > + const DataLayout *DL,
> > + unsigned &UsedICmps,
> > + bool isEQ) {
> > +
> > + // If this is an icmp against a constant, handle this as one of the
> cases.
> > + ICmpInst *ICI;
> > + ConstantInt *C;
> > + if (!((ICI = dyn_cast<ICmpInst>(I)) &&
> > + (C = GetConstantInt(I->getOperand(1), DL)))) {
> > + return nullptr;
> > + }
> >
> > - UsedICmps++;
> > - Vals.push_back(C);
> > - return ICI->getOperand(0);
> > - }
> > -
> > - // If we have "x ult 3", for example, then we can add 0,1,2 to the
> set.
> > - ConstantRange Span = ConstantRange::makeICmpRegion(
> ICI->getPredicate(),
> > - C->getValue());
> > -
> > - // Shift the range if the compare is fed by an add. This is the
> range
> > - // compare idiom as emitted by instcombine.
> > - Value *CandidateVal = I->getOperand(0);
> > - if(match(I->getOperand(0), m_Add(m_Value(RHSVal),
> m_ConstantInt(RHSC)))) {
> > - Span = Span.subtract(RHSC->getValue());
> > - CandidateVal = RHSVal;
> > - }
> > -
> > - // If this is an and/!= check, then we are looking to build the set
> of
> > - // value that *don't* pass the and chain. I.e. to turn "x ugt 2"
> into
> > - // x != 0 && x != 1.
> > - if (!isEQ)
> > - Span = Span.inverse();
> > + Value *RHSVal;
> > + ConstantInt *RHSC;
> >
> > - // If there are a ton of values, we don't want to make a ginormous
> switch.
> > - if (Span.getSetSize().ugt(8) || Span.isEmptySet()) {
> > - return false;
> > + // Pattern match a special case
> > + // (x & ~2^x) == y --> x == y || x == y|2^x
> > + // This undoes a transformation done by instcombine to fuse 2
> compares.
> > + if (ICI->getPredicate() == (isEQ ? ICmpInst::ICMP_EQ:ICmpInst::ICMP_NE))
> {
> > + if (match(ICI->getOperand(0),
> > + m_And(m_Value(RHSVal), m_ConstantInt(RHSC)))) {
> > + APInt Not = ~RHSC->getValue();
> > + if (Not.isPowerOf2()) {
> > + // If we already have a value for the switch, it has to match!
> > + if(CurrValue && CurrValue != RHSVal)
> > + return nullptr;
> > +
> > + Vals.push_back(C);
> > + Vals.push_back(ConstantInt::get(C->getContext(),
> > + C->getValue() | Not));
> > + UsedICmps++;
> > + return RHSVal;
> > + }
> > }
> >
> > // If we already have a value for the switch, it has to match!
> > - if(!setValueOnce(CandidateVal))
> > - return false;
> > -
> > - // Add all values from the range to the set
> > - for (APInt Tmp = Span.getLower(); Tmp != Span.getUpper(); ++Tmp)
> > - Vals.push_back(ConstantInt::get(I->getContext(), Tmp));
> > + if(CurrValue && CurrValue != ICI->getOperand(0))
> > + return nullptr;
> >
> > UsedICmps++;
> > - return true;
> > + Vals.push_back(C);
> > + return ICI->getOperand(0);
> > + }
> >
> > + // If we have "x ult 3", for example, then we can add 0,1,2 to the
> set.
> > + ConstantRange Span = ConstantRange::makeICmpRegion(
> ICI->getPredicate(),
> > + C->getValue());
> > +
> > + // Shift the range if the compare is fed by an add. This is the range
> > + // compare idiom as emitted by instcombine.
> > + Value *CandidateVal = I->getOperand(0);
> > + if(match(I->getOperand(0), m_Add(m_Value(RHSVal),
> m_ConstantInt(RHSC)))) {
> > + Span = Span.subtract(RHSC->getValue());
> > + CandidateVal = RHSVal;
> > }
> >
> > - /// gather - Given a potentially 'or'd or 'and'd together collection
> of icmp
> > - /// eq/ne/lt/gt instructions that compare a value against a constant,
> extract
> > - /// the value being compared, and stick the list constants into the
> Vals
> > - /// vector.
> > - /// One "Extra" case is allowed to differ from the other.
> > - void gather(Value *V, const DataLayout *DL) {
> > - Instruction *I = dyn_cast<Instruction>(V);
> > - bool isEQ = (I->getOpcode() == Instruction::Or);
> > -
> > - // Keep a stack (SmallVector for efficiency) for depth-first
> traversal
> > - SmallVector<Value *, 8> DFT;
> > -
> > - // Initialize
> > - DFT.push_back(V);
> > -
> > - while(!DFT.empty()) {
> > - V = DFT.pop_back_val();
> > -
> > - if (Instruction *I = dyn_cast<Instruction>(V)) {
> > - // If it is a || (or && depending on isEQ), process the
> operands.
> > - if (I->getOpcode() == (isEQ ? Instruction::Or :
> Instruction::And)) {
> > - DFT.push_back(I->getOperand(1));
> > - DFT.push_back(I->getOperand(0));
> > - continue;
> > - }
> > + // If we already have a value for the switch, it has to match!
> > + if(CurrValue && CurrValue != CandidateVal)
> > + return nullptr;
> > +
> > + // If this is an and/!= check, then we are looking to build the set of
> > + // value that *don't* pass the and chain. I.e. to turn "x ugt 2" into
> > + // x != 0 && x != 1.
> > + if (!isEQ)
> > + Span = Span.inverse();
> > +
> > + // If there are a ton of values, we don't want to make a ginormous
> switch.
> > + if (Span.getSetSize().ugt(8) || Span.isEmptySet()) {
> > + return nullptr;
> > + }
> > +
> > + // Add all values from the range to the set
> > + for (APInt Tmp = Span.getLower(); Tmp != Span.getUpper(); ++Tmp)
> > + Vals.push_back(ConstantInt::get(I->getContext(), Tmp));
> >
> > - // Try to match the current instruction
> > - if (matchInstruction(I, DL, isEQ))
> > - // Match succeed, continue the loop
> > - continue;
> > + UsedICmps++;
> > + return CandidateVal;
> > +
> > +}
> > +
> > +/// GatherConstantCompares - Given a potentially 'or'd or 'and'd
> together
> > +/// collection of icmp eq/ne instructions that compare a value against a
> > +/// constant, return the value being compared, and stick the constant
> into the
> > +/// Values vector.
> > +/// One "Extra" case is allowed to differ from the other.
> > +static Value *
> > +GatherConstantCompares(Value *V, SmallVectorImpl<ConstantInt*> &Vals,
> Value *&Extra,
> > + const DataLayout *DL, unsigned &UsedICmps) {
> > + Instruction *I = dyn_cast<Instruction>(V);
> > + if (!I) return nullptr;
> > +
> > + bool isEQ = (I->getOpcode() == Instruction::Or);
> > +
> > + // Keep a stack (SmallVector for efficiency) for depth-first traversal
> > + SmallVector<Value *, 8> DFT;
> > +
> > + // Initialize
> > + DFT.push_back(V);
> > +
> > + // Will hold the value used for the switch comparison
> > + Value *CurrValue = nullptr;
> > +
> > + while(!DFT.empty()) {
> > + V = DFT.pop_back_val();
> > +
> > + if (Instruction *I = dyn_cast<Instruction>(V)) {
> > +
> > + // If it is a || (or && depending on isEQ), process the operands.
> > + if (I->getOpcode() == (isEQ ? Instruction::Or :
> Instruction::And)) {
> > + DFT.push_back(I->getOperand(1));
> > + DFT.push_back(I->getOperand(0));
> > + continue;
> > }
> >
> > - // One element of the sequence of || (or &&) could not be match
> as a
> > - // comparison against the same value as the others.
> > - // We allow only one "Extra" case to be checked before the switch
> > - if (!Extra) {
> > - Extra = V;
> > + // Try to match the current instruction
> > + if (Value *Matched = GatherConstantComparesMatch(I,
> > + CurrValue,
> > + Vals,
> > + DL,
> > + UsedICmps,
> > + isEQ)) {
> > + // Match succeed, continue the loop
> > + CurrValue = Matched;
> > continue;
> > }
> > - // Failed to parse a proper sequence, abort now
> > - CompValue = nullptr;
> > - break;
> > }
> > +
> > + // One element of the sequence of || (or &&) could not be match as a
> > + // comparison against the same value as the others.
> > + // We allow only one "Extra" case to be checked before the switch
> > + if (Extra == nullptr) {
> > + Extra = V;
> > + continue;
> > + }
> > + return nullptr;
> > +
> > }
> > -};
> > +
> > + // Return the value to be used for the switch comparison (if any)
> > + return CurrValue;
> > +}
> >
> > static void EraseTerminatorInstAndDCECond(TerminatorInst *TI) {
> > Instruction *Cond = nullptr;
> > @@ -2823,17 +2810,18 @@ static bool SimplifyBranchOnICmpChain(Br
> > Instruction *Cond = dyn_cast<Instruction>(BI->getCondition());
> > if (!Cond) return false;
> >
> > +
> > // Change br (X == 0 | X == 1), T, F into a switch instruction.
> > // If this is a bunch of seteq's or'd together, or if it's a bunch of
> > // 'setne's and'ed together, collect them.
> > + Value *CompVal = nullptr;
> > + SmallVector<ConstantInt*, 8> Values;
> > + bool TrueWhenEqual = (Cond->getOpcode() == Instruction::Or);
> > + Value *ExtraCase = nullptr;
> > + unsigned UsedICmps = 0;
> >
> > // Try to gather values from a chain of and/or to be turned into a
> switch
> > - ConstantComparesGatherer ConstantCompare(Cond, DL);
> > - // Unpack the result
> > - SmallVectorImpl<ConstantInt*> &Values = ConstantCompare.Vals;
> > - Value *CompVal = ConstantCompare.CompValue;
> > - unsigned UsedICmps = ConstantCompare.UsedICmps;
> > - Value *ExtraCase = ConstantCompare.Extra;
> > + CompVal = GatherConstantCompares(Cond, Values, ExtraCase, DL,
> UsedICmps);
> >
> > // If we didn't have a multiply compared value, fail.
> > if (!CompVal) return false;
> > @@ -2842,8 +2830,6 @@ static bool SimplifyBranchOnICmpChain(Br
> > if (UsedICmps <= 1)
> > return false;
> >
> > - bool TrueWhenEqual = (Cond->getOpcode() == Instruction::Or);
> > -
> > // There might be duplicate constants in the list, which the switch
> > // instruction can't handle, remove them now.
> > array_pod_sort(Values.begin(), Values.end(), ConstantIntSortPredicate);
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141120/291eea07/attachment.html>
More information about the llvm-commits
mailing list