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