<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Oh for some reason my mail client did not find this thread I used the search function, I apologize.<div class=""><br class=""></div><div class="">So the “typo” was indeed not a typo (but I agree it is not well written and I’ll fix it).</div><div class=""><br class=""></div><div class="">The bot is green in r222422 before your change, isn’t it?</div><div class=""><br class=""></div><div class="">Thanks!</div><div class=""><br class=""></div><div class="">Mehdi</div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><br class=""></div><div class=""><br class=""></div><div><blockquote type="cite" class=""><div class="">On Nov 20, 2014, at 4:08 AM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com" class="">timurrrr@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">Fixing this typo has resulted in buildbot errors, so I'm going to revert both mine and your changes [assuming you're not immediately available for a fix].<div class=""><a href="http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/20839" class="">http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/20839</a></div><div class=""><br class=""><div class="gmail_quote">On Thu Nov 20 2014 at 2:51:06 PM Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com" target="_blank" class="cremed">timurrrr@google.com</a>> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote">On Thu Nov 20 2014 at 9:53:42 AM Mehdi Amini <<a href="mailto:mehdi.amini@apple.com" target="_blank" class="cremed">mehdi.amini@apple.com</a>> wrote:<br class=""></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: mehdi_amini<br class="">
Date: Thu Nov 20 00:51:02 2014<br class="">
New Revision: 222416<br class="">
<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=222416&view=rev" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-<u class=""></u>pr<u class=""></u><u class=""></u>oject?rev=222416&view=rev</a><br class="">
Log:<br class="">
SimplifyCFG: Refactor GatherConstantCompares() result in a struct<br class="">
<br class="">
Code seems cleaner and easier to understand this way<br class="">
<br class="">
<br class="">
<br class="">
Modified:<br class="">
llvm/trunk/lib/Transforms/<u class=""></u>Util<u class=""></u><u class=""></u>s/SimplifyCFG.cpp<br class="">
<br class="">
Modified: llvm/trunk/lib/Transforms/<u class=""></u>Util<u class=""></u><u class=""></u>s/SimplifyCFG.cpp<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp?rev=222416&r1=222415&r2=222416&view=diff" target="_blank" class="cremed">http://llvm.org/viewvc/llvm-<u class=""></u>pr<u class=""></u><u class=""></u>oject/llvm/trunk/lib/<u class=""></u>Transform<u class=""></u><u class=""></u>s/Utils/SimplifyCFG.<u class=""></u>cpp?rev=<u class=""></u>22<u class=""></u>2416&r1=222415&r2=<u class=""></u>222416&<u class=""></u>view=<u class=""></u>diff</a><br class="">
==============================<u class=""></u><u class=""></u><u class=""></u>==============================<u class=""></u><u class=""></u><u class=""></u>==================<br class="">
--- llvm/trunk/lib/Transforms/<u class=""></u>Util<u class=""></u><u class=""></u>s/SimplifyCFG.cpp (original)<br class="">
+++ llvm/trunk/lib/Transforms/<u class=""></u>Util<u class=""></u><u class=""></u>s/SimplifyCFG.cpp Thu Nov 20 00:51:02 2014<br class="">
@@ -357,160 +357,170 @@ static ConstantInt *GetConstantInt(Value<br class="">
return nullptr;<br class="">
}<br class="">
<br class="">
+/// Given a chain of or (||) or and (&&) comparison of a value against a<br class="">
+/// constant, this will try to recover the information required for a switch<br class="">
+/// structure.<br class="">
+/// It will depth-first traverse the chain of comparison, seeking for patterns<br class="">
+/// like %a == 12 or %a < 4 and combine them to produce a set of integer<br class="">
+/// representing the different cases for the switch.<br class="">
+/// Note that if the chain is composed of '||' it will build the set of elements<br class="">
+/// that matches the comparisons (i.e. any of this value validate the chain)<br class="">
+/// while for a chain of '&&' it will build the set elements that make the test<br class="">
+/// fail.<br class="">
+struct ConstantComparesGatherer {<br class="">
+<br class="">
+ Value *CompValue = nullptr; /// Value found for the switch comparison<br class="">
+ Value *Extra = nullptr; /// Extra clause to be checked before the switch<br class="">
+ SmallVector<ConstantInt*, 8> Vals; /// Set of integers to match in switch<br class="">
+ unsigned UsedICmps = 0; /// Number of comparisons matched in the and/or chain<br class="">
+<br class="">
+ /// Construct and compute the result for the comparison instruction Cond<br class="">
+ ConstantComparesGatherer(<u class=""></u>Instr<u class=""></u><u class=""></u>uction *Cond, const DataLayout *DL) {<br class="">
+ gather(Cond, DL);<br class="">
+ }<br class="">
+<br class="">
+ /// Prevent copy<br class="">
+ ConstantComparesGatherer(const ConstantComparesGatherer&) = delete;<br class="">
+ ConstantComparesGatherer &operator=(const ConstantComparesGatherer&) = delete;<br class="">
+<br class="">
+private:<br class="">
+<br class="">
+ /// Try to set the current value used for the comparison, it succeeds only if<br class="">
+ /// it wasn't set before or if the new value is the same as the old one<br class="">
+ bool setValueOnce(Value *NewVal) {<br class="">
+ if(CompValue && CompValue != NewVal) return false;<br class="">
+ return CompValue = NewVal;<br class=""></blockquote><div class=""><br class=""></div></div><div class="gmail_quote"><div class="">This looks like a typo.</div></div><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ }<br class="">
+<br class=""></blockquote><div class=""> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ /// Try to match Instruction "I" as a comparison against a constant and<br class="">
+ /// populates the array Vals with the set of values that match (or do not<br class="">
+ /// match depending on isEQ).<br class="">
+ /// Return false on failure. On success, the Value the comparison matched<br class="">
+ /// against is placed in CompValue.<br class="">
+ /// If CompValue is already set, the function is expected to fail if a match<br class="">
+ /// is found but the value compared to is different.<br class="">
+ bool matchInstruction(Instruction *I, const DataLayout *DL, bool isEQ) {<br class="">
+ // If this is an icmp against a constant, handle this as one of the cases.<br class="">
+ ICmpInst *ICI;<br class="">
+ ConstantInt *C;<br class="">
+ if (!((ICI = dyn_cast<ICmpInst>(I)) &&<br class="">
+ (C = GetConstantInt(I->getOperand(<u class=""></u>1<u class=""></u><u class=""></u>), DL)))) {<br class="">
+ return false;<br class="">
+ }<br class="">
<br class="">
+ Value *RHSVal;<br class="">
+ ConstantInt *RHSC;<br class="">
<br class="">
-// Try to match Instruction I as a comparison against a constant and populates<br class="">
-// Vals with the set of value that match (or does not depending on isEQ).<br class="">
-// Return nullptr on failure, or return the Value the comparison matched against<br class="">
-// on success<br class="">
-// CurrValue, if supplied, is the value we want to match against. The function<br class="">
-// is expected to fail if a match is found but the value compared to is not the<br class="">
-// one expected. If CurrValue is supplied, the return value has to be either<br class="">
-// nullptr or CurrValue<br class="">
-static Value* GatherConstantComparesMatch(<u class=""></u>In<u class=""></u><u class=""></u>struction *I,<br class="">
- Value *CurrValue,<br class="">
- SmallVectorImpl<ConstantInt*> &Vals,<br class="">
- const DataLayout *DL,<br class="">
- unsigned &UsedICmps,<br class="">
- bool isEQ) {<br class="">
-<br class="">
- // If this is an icmp against a constant, handle this as one of the cases.<br class="">
- ICmpInst *ICI;<br class="">
- ConstantInt *C;<br class="">
- if (!((ICI = dyn_cast<ICmpInst>(I)) &&<br class="">
- (C = GetConstantInt(I->getOperand(<u class=""></u>1<u class=""></u><u class=""></u>), DL)))) {<br class="">
- return nullptr;<br class="">
- }<br class="">
-<br class="">
- Value *RHSVal;<br class="">
- ConstantInt *RHSC;<br class="">
-<br class="">
- // Pattern match a special case<br class="">
- // (x & ~2^x) == y --> x == y || x == y|2^x<br class="">
- // This undoes a transformation done by instcombine to fuse 2 compares.<br class="">
- if (ICI->getPredicate() == (isEQ ? ICmpInst::ICMP_EQ:ICmpInst::<u class=""></u>IC<u class=""></u><u class=""></u>MP_NE)) {<br class="">
- if (match(ICI->getOperand(0),<br class="">
- m_And(m_Value(RHSVal), m_ConstantInt(RHSC)))) {<br class="">
- APInt Not = ~RHSC->getValue();<br class="">
- if (Not.isPowerOf2()) {<br class="">
- // If we already have a value for the switch, it has to match!<br class="">
- if(CurrValue && CurrValue != RHSVal)<br class="">
- return nullptr;<br class="">
-<br class="">
- Vals.push_back(C);<br class="">
- Vals.push_back(ConstantInt::<u class=""></u>ge<u class=""></u><u class=""></u>t(C->getContext(),<br class="">
- C->getValue() | Not));<br class="">
- UsedICmps++;<br class="">
- return RHSVal;<br class="">
+ // Pattern match a special case<br class="">
+ // (x & ~2^x) == y --> x == y || x == y|2^x<br class="">
+ // This undoes a transformation done by instcombine to fuse 2 compares.<br class="">
+ if (ICI->getPredicate() == (isEQ ? ICmpInst::ICMP_EQ:ICmpInst::<u class=""></u>IC<u class=""></u><u class=""></u>MP_NE)) {<br class="">
+ if (match(ICI->getOperand(0),<br class="">
+ m_And(m_Value(RHSVal), m_ConstantInt(RHSC)))) {<br class="">
+ APInt Not = ~RHSC->getValue();<br class="">
+ if (Not.isPowerOf2()) {<br class="">
+ // If we already have a value for the switch, it has to match!<br class="">
+ if(!setValueOnce(RHSVal))<br class="">
+ return false;<br class="">
+<br class="">
+ Vals.push_back(C);<br class="">
+ Vals.push_back(ConstantInt::<u class=""></u>ge<u class=""></u><u class=""></u>t(C->getContext(),<br class="">
+ C->getValue() | Not));<br class="">
+ UsedICmps++;<br class="">
+ return true;<br class="">
+ }<br class="">
}<br class="">
- }<br class="">
<br class="">
- // If we already have a value for the switch, it has to match!<br class="">
- if(CurrValue && CurrValue != ICI->getOperand(0))<br class="">
- return nullptr;<br class="">
+ // If we already have a value for the switch, it has to match!<br class="">
+ if(!setValueOnce(ICI-><u class=""></u>getOpera<u class=""></u><u class=""></u>nd(0)))<br class="">
+ return false;<br class="">
<br class="">
- UsedICmps++;<br class="">
- Vals.push_back(C);<br class="">
- return ICI->getOperand(0);<br class="">
- }<br class="">
+ UsedICmps++;<br class="">
+ Vals.push_back(C);<br class="">
+ return ICI->getOperand(0);<br class="">
+ }<br class="">
+<br class="">
+ // If we have "x ult 3", for example, then we can add 0,1,2 to the set.<br class="">
+ ConstantRange Span = ConstantRange::makeICmpRegion(<u class=""></u><u class=""></u><u class=""></u>ICI->getPredicate(),<br class="">
+ C->getValue());<br class="">
+<br class="">
+ // Shift the range if the compare is fed by an add. This is the range<br class="">
+ // compare idiom as emitted by instcombine.<br class="">
+ Value *CandidateVal = I->getOperand(0);<br class="">
+ if(match(I->getOperand(0), m_Add(m_Value(RHSVal), m_ConstantInt(RHSC)))) {<br class="">
+ Span = Span.subtract(RHSC->getValue()<u class=""></u><u class=""></u><u class=""></u>);<br class="">
+ CandidateVal = RHSVal;<br class="">
+ }<br class="">
+<br class="">
+ // If this is an and/!= check, then we are looking to build the set of<br class="">
+ // value that *don't* pass the and chain. I.e. to turn "x ugt 2" into<br class="">
+ // x != 0 && x != 1.<br class="">
+ if (!isEQ)<br class="">
+ Span = Span.inverse();<br class="">
<br class="">
- // If we have "x ult 3", for example, then we can add 0,1,2 to the set.<br class="">
- ConstantRange Span = ConstantRange::makeICmpRegion(<u class=""></u><u class=""></u><u class=""></u>ICI->getPredicate(),<br class="">
- C->getValue());<br class="">
+ // If there are a ton of values, we don't want to make a ginormous switch.<br class="">
+ if (Span.getSetSize().ugt(8) || Span.isEmptySet()) {<br class="">
+ return false;<br class="">
+ }<br class="">
<br class="">
- // Shift the range if the compare is fed by an add. This is the range<br class="">
- // compare idiom as emitted by instcombine.<br class="">
- Value *CandidateVal = I->getOperand(0);<br class="">
- if(match(I->getOperand(0), m_Add(m_Value(RHSVal), m_ConstantInt(RHSC)))) {<br class="">
- Span = Span.subtract(RHSC->getValue()<u class=""></u><u class=""></u><u class=""></u>);<br class="">
- CandidateVal = RHSVal;<br class="">
- }<br class="">
+ // If we already have a value for the switch, it has to match!<br class="">
+ if(!setValueOnce(CandidateVal)<u class=""></u><u class=""></u><u class=""></u>)<br class="">
+ return false;<br class="">
<br class="">
- // If we already have a value for the switch, it has to match!<br class="">
- if(CurrValue && CurrValue != CandidateVal)<br class="">
- return nullptr;<br class="">
+ // Add all values from the range to the set<br class="">
+ for (APInt Tmp = Span.getLower(); Tmp != Span.getUpper(); ++Tmp)<br class="">
+ Vals.push_back(ConstantInt::<u class=""></u>ge<u class=""></u><u class=""></u>t(I->getContext(), Tmp));<br class="">
<br class="">
- // If this is an and/!= check, then we are looking to build the set of<br class="">
- // value that *don't* pass the and chain. I.e. to turn "x ugt 2" into<br class="">
- // x != 0 && x != 1.<br class="">
- if (!isEQ)<br class="">
- Span = Span.inverse();<br class="">
+ UsedICmps++;<br class="">
+ return true;<br class="">
<br class="">
- // If there are a ton of values, we don't want to make a ginormous switch.<br class="">
- if (Span.getSetSize().ugt(8) || Span.isEmptySet()) {<br class="">
- return nullptr;<br class="">
}<br class="">
<br class="">
- // Add all values from the range to the set<br class="">
- for (APInt Tmp = Span.getLower(); Tmp != Span.getUpper(); ++Tmp)<br class="">
- Vals.push_back(ConstantInt::<u class=""></u>ge<u class=""></u><u class=""></u>t(I->getContext(), Tmp));<br class="">
-<br class="">
- UsedICmps++;<br class="">
- return CandidateVal;<br class="">
-<br class="">
-}<br class="">
-<br class="">
-/// GatherConstantCompares - Given a potentially 'or'd or 'and'd together<br class="">
-/// collection of icmp eq/ne instructions that compare a value against a<br class="">
-/// constant, return the value being compared, and stick the constant into the<br class="">
-/// Values vector.<br class="">
-/// One "Extra" case is allowed to differ from the other.<br class="">
-static Value *<br class="">
-GatherConstantCompares(Value *V, SmallVectorImpl<ConstantInt*> &Vals, Value *&Extra,<br class="">
- const DataLayout *DL, unsigned &UsedICmps) {<br class="">
- Instruction *I = dyn_cast<Instruction>(V);<br class="">
- if (!I) return nullptr;<br class="">
-<br class="">
- bool isEQ = (I->getOpcode() == Instruction::Or);<br class="">
-<br class="">
- // Keep a stack (SmallVector for efficiency) for depth-first traversal<br class="">
- SmallVector<Value *, 8> DFT;<br class="">
-<br class="">
- // Initialize<br class="">
- DFT.push_back(V);<br class="">
-<br class="">
- // Will hold the value used for the switch comparison<br class="">
- Value *CurrValue = nullptr;<br class="">
-<br class="">
- while(!DFT.empty()) {<br class="">
- V = DFT.pop_back_val();<br class="">
-<br class="">
- if (Instruction *I = dyn_cast<Instruction>(V)) {<br class="">
+ /// gather - Given a potentially 'or'd or 'and'd together collection of icmp<br class="">
+ /// eq/ne/lt/gt instructions that compare a value against a constant, extract<br class="">
+ /// the value being compared, and stick the list constants into the Vals<br class="">
+ /// vector.<br class="">
+ /// One "Extra" case is allowed to differ from the other.<br class="">
+ void gather(Value *V, const DataLayout *DL) {<br class="">
+ Instruction *I = dyn_cast<Instruction>(V);<br class="">
+ bool isEQ = (I->getOpcode() == Instruction::Or);<br class="">
+<br class="">
+ // Keep a stack (SmallVector for efficiency) for depth-first traversal<br class="">
+ SmallVector<Value *, 8> DFT;<br class="">
+<br class="">
+ // Initialize<br class="">
+ DFT.push_back(V);<br class="">
+<br class="">
+ while(!DFT.empty()) {<br class="">
+ V = DFT.pop_back_val();<br class="">
+<br class="">
+ if (Instruction *I = dyn_cast<Instruction>(V)) {<br class="">
+ // If it is a || (or && depending on isEQ), process the operands.<br class="">
+ if (I->getOpcode() == (isEQ ? Instruction::Or : Instruction::And)) {<br class="">
+ DFT.push_back(I->getOperand(1)<u class=""></u><u class=""></u><u class=""></u>);<br class="">
+ DFT.push_back(I->getOperand(0)<u class=""></u><u class=""></u><u class=""></u>);<br class="">
+ continue;<br class="">
+ }<br class="">
<br class="">
- // If it is a || (or && depending on isEQ), process the operands.<br class="">
- if (I->getOpcode() == (isEQ ? Instruction::Or : Instruction::And)) {<br class="">
- DFT.push_back(I->getOperand(1)<u class=""></u><u class=""></u><u class=""></u>);<br class="">
- DFT.push_back(I->getOperand(0)<u class=""></u><u class=""></u><u class=""></u>);<br class="">
- continue;<br class="">
+ // Try to match the current instruction<br class="">
+ if (matchInstruction(I, DL, isEQ))<br class="">
+ // Match succeed, continue the loop<br class="">
+ continue;<br class="">
}<br class="">
<br class="">
- // Try to match the current instruction<br class="">
- if (Value *Matched = GatherConstantComparesMatch(I,<br class="">
- CurrValue,<br class="">
- Vals,<br class="">
- DL,<br class="">
- UsedICmps,<br class="">
- isEQ)) {<br class="">
- // Match succeed, continue the loop<br class="">
- CurrValue = Matched;<br class="">
+ // One element of the sequence of || (or &&) could not be match as a<br class="">
+ // comparison against the same value as the others.<br class="">
+ // We allow only one "Extra" case to be checked before the switch<br class="">
+ if (!Extra) {<br class="">
+ Extra = V;<br class="">
continue;<br class="">
}<br class="">
+ // Failed to parse a proper sequence, abort now<br class="">
+ CompValue = nullptr;<br class="">
+ break;<br class="">
}<br class="">
-<br class="">
- // One element of the sequence of || (or &&) could not be match as a<br class="">
- // comparison against the same value as the others.<br class="">
- // We allow only one "Extra" case to be checked before the switch<br class="">
- if (Extra == nullptr) {<br class="">
- Extra = V;<br class="">
- continue;<br class="">
- }<br class="">
- return nullptr;<br class="">
-<br class="">
}<br class="">
-<br class="">
- // Return the value to be used for the switch comparison (if any)<br class="">
- return CurrValue;<br class="">
-}<br class="">
+};<br class="">
<br class="">
static void EraseTerminatorInstAndDCECond(<u class=""></u><u class=""></u><u class=""></u>TerminatorInst *TI) {<br class="">
Instruction *Cond = nullptr;<br class="">
@@ -2810,18 +2820,17 @@ static bool SimplifyBranchOnICmpChain(Br<br class="">
Instruction *Cond = dyn_cast<Instruction>(BI-><u class=""></u>getC<u class=""></u><u class=""></u>ondition());<br class="">
if (!Cond) return false;<br class="">
<br class="">
-<br class="">
// Change br (X == 0 | X == 1), T, F into a switch instruction.<br class="">
// If this is a bunch of seteq's or'd together, or if it's a bunch of<br class="">
// 'setne's and'ed together, collect them.<br class="">
- Value *CompVal = nullptr;<br class="">
- SmallVector<ConstantInt*, 8> Values;<br class="">
- bool TrueWhenEqual = (Cond->getOpcode() == Instruction::Or);<br class="">
- Value *ExtraCase = nullptr;<br class="">
- unsigned UsedICmps = 0;<br class="">
<br class="">
// Try to gather values from a chain of and/or to be turned into a switch<br class="">
- CompVal = GatherConstantCompares(Cond, Values, ExtraCase, DL, UsedICmps);<br class="">
+ ConstantComparesGatherer ConstantCompare{Cond, DL};<br class="">
+ // Unpack the result<br class="">
+ SmallVectorImpl<ConstantInt*> &Values = ConstantCompare.Vals;<br class="">
+ Value *CompVal = ConstantCompare.CompValue;<br class="">
+ unsigned UsedICmps = ConstantCompare.UsedICmps;<br class="">
+ Value *ExtraCase = ConstantCompare.Extra;<br class="">
<br class="">
// If we didn't have a multiply compared value, fail.<br class="">
if (!CompVal) return false;<br class="">
@@ -2830,6 +2839,8 @@ static bool SimplifyBranchOnICmpChain(Br<br class="">
if (UsedICmps <= 1)<br class="">
return false;<br class="">
<br class="">
+ bool TrueWhenEqual = (Cond->getOpcode() == Instruction::Or);<br class="">
+<br class="">
// There might be duplicate constants in the list, which the switch<br class="">
// instruction can't handle, remove them now.<br class="">
array_pod_sort(Values.begin()<u class=""></u><u class=""></u>, Values.end(), ConstantIntSortPredicate);<br class="">
<br class="">
<br class="">
______________________________<u class=""></u><u class=""></u><u class=""></u>_________________<br class="">
llvm-commits mailing list<br class="">
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank" class="cremed">llvm-commits@cs.uiuc.edu</a><br class="">
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank" class="cremed">http://lists.cs.uiuc.edu/<u class=""></u>mailm<u class=""></u><u class=""></u>an/listinfo/llvm-commits</a><br class="">
</blockquote></div></blockquote></div></div>
</div></blockquote></div><br class=""></body></html>