This reverts r222416 [and its descendants] and the reason for revert was explained on the r222416 commit thread.<br><br><div class="gmail_quote">On Thu Nov 20 2014 at 8:26:16 PM Philip Reames <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
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.</div><div bgcolor="#FFFFFF" text="#000000"><br>
<br>
Philip</div><div bgcolor="#FFFFFF" text="#000000"><br>
<br>
<div>On 11/20/2014 07:50 AM, Timur
Iskhodzhanov wrote:<br>
</div>
<blockquote type="cite">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" target="_blank">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-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/Utils/SimplifyCFG.cpp<br>
><br>
> Modified: llvm/trunk/lib/Transforms/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-project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=222428&r1=222427&r2=222428&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
(original)<br>
> +++ llvm/trunk/lib/Transforms/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(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(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::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::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->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(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(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(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());<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::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::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))<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::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(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());<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));<br>
> - DFT.push_back(I->getOperand(0));<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::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));<br>
> + DFT.push_back(I->getOperand(0));<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(TerminatorInst
*TI) {<br>
> Instruction *Cond = nullptr;<br>
> @@ -2823,17 +2810,18 @@ static bool
SimplifyBranchOnICmpChain(Br<br>
> Instruction *Cond = dyn_cast<Instruction>(BI->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>
> _______________________________________________<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/mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote>
</div>
<br>
<fieldset></fieldset>
<br>
<pre>_______________________________________________
llvm-commits mailing list
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
<br>
</div></blockquote></div>