[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 12:07:00 PST 2014
This reverts r222416 [and its descendants] and the reason for revert was
explained on the r222416 commit thread.
On Thu Nov 20 2014 at 8:26:16 PM Philip Reames <listmail at philipreames.com>
wrote:
> Please include it in the revert comment if possible. Or an email in
> response to that thread on the mailing list. There are many of us who
> follow the commits list and a hint as to where to look for further
> information is extremely useful.
>
>
> Philip
>
>
> On 11/20/2014 07:50 AM, Timur Iskhodzhanov wrote:
>
> 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
>>
>>
>
> _______________________________________________
> llvm-commits mailing listllvm-commits at cs.uiuc.eduhttp://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/5a0ae517/attachment.html>
More information about the llvm-commits
mailing list