<p dir="ltr">Thanks!<br>
</p>
<br><div class="gmail_quote">пт, 21 нояб. 2014, 2:31, Mehdi Amini <<a href="mailto:mehdi.amini@apple.com">mehdi.amini@apple.com</a>>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">I tried a new version, let me know if you see any other issue.<div><br></div><div>Thanks,</div><div><br></div><div>Mehdi</div></div><div style="word-wrap:break-word"><div><br><div><blockquote type="cite"><div>On Nov 20, 2014, at 12:44 PM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com" target="_blank">timurrrr@google.com</a>> wrote:</div><br><div>The bots were green, but in some of my configurations I got a compiler warning:<br><span style="line-height:19.7999992370605px">llvm/lib/</span><u style="line-height:19.7999992370605px"></u><span style="line-height:19.7999992370605px">Tran</span><span style="line-height:19.7999992370605px">sforms/Utils/</span><span style="line-height:19.7999992370605px">SimplifyCFG</span><span style="line-height:19.7999992370605px">.</span><u style="line-height:19.7999992370605px"></u><span style="line-height:19.7999992370605px">cpp:</span><span style="line-height:19.7999992370605px">392:24: error: suggest parentheses around assignment used as truth value [-Werror=parentheses]</span><div><span style="line-height:19.7999992370605px"><br></span></div><div><span style="line-height:19.7999992370605px">I've looked at the code and (based on the comment) thought there had to be == rather than =.  Doing that broke some things and you were not available for advice, so I've decided to revert this altogether and let you reland with fixes when it's convenient for you.</span></div><div><span style="line-height:19.7999992370605px"><br></span></div><div><span style="line-height:19.7999992370605px">Sorry if I was a bit too concise in my communications...</span></div><div><span style="line-height:19.7999992370605px"><br></span></div><div><span style="line-height:19.7999992370605px">Thanks,</span></div><div><span style="line-height:19.7999992370605px">Tim<br></span><br><div class="gmail_quote">On Thu Nov 20 2014 at 7:33:58 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"><div style="word-wrap:break-word">Oh for some reason my mail client did not find this thread I used the search function, I apologize.<div><br></div><div>So the “typo” was indeed not a typo (but I agree it is not well written and I’ll fix it).</div><div><br></div><div>The bot is green in r222422 before your change, isn’t it?</div><div><br></div><div>Thanks!</div></div><div style="word-wrap:break-word"><div><br></div><div>Mehdi</div></div><div style="word-wrap:break-word"><div><br></div><div><br></div><div><br></div><div><br></div><div><blockquote type="cite"><div>On Nov 20, 2014, at 4:08 AM, Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com" target="_blank">timurrrr@google.com</a>> wrote:</div><br><div>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><a href="http://lab.llvm.org:8011/builders/clang-x86_64-debian-fast/builds/20839" target="_blank">http://lab.llvm.org:8011/<u></u>builders/clang-x86_64-debian-<u></u>fast/builds/20839</a></div><div><br><div class="gmail_quote">On Thu Nov 20 2014 at 2:51:06 PM Timur Iskhodzhanov <<a href="mailto:timurrrr@google.com" target="_blank">timurrrr@google.com</a>> wrote:<br><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">mehdi.amini@apple.com</a>> wrote:<br></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>
Date: Thu Nov 20 00:51:02 2014<br>
New Revision: 222416<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=222416&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>pr<u></u><u></u><u></u>oject?rev=222416&view=rev</a><br>
Log:<br>
SimplifyCFG: Refactor GatherConstantCompares() result in a struct<br>
<br>
Code seems cleaner and easier to understand this way<br>
<br>
<br>
<br>
Modified:<br>
    llvm/trunk/lib/Transforms/<u></u>Util<u></u><u></u><u></u>s/SimplifyCFG.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/<u></u>Util<u></u><u></u><u></u>s/SimplifyCFG.cpp<br>
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">http://llvm.org/viewvc/llvm-<u></u>pr<u></u><u></u><u></u>oject/llvm/trunk/lib/<u></u>Transform<u></u><u></u><u></u>s/Utils/SimplifyCFG.<u></u>cpp?rev=<u></u>22<u></u><u></u>2416&r1=222415&r2=<u></u>222416&<u></u>view=<u></u><u></u>diff</a><br>
==============================<u></u><u></u><u></u><u></u>==============================<u></u><u></u><u></u><u></u>==================<br>
--- llvm/trunk/lib/Transforms/<u></u>Util<u></u><u></u><u></u>s/SimplifyCFG.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/<u></u>Util<u></u><u></u><u></u>s/SimplifyCFG.cpp Thu Nov 20 00:51:02 2014<br>
@@ -357,160 +357,170 @@ 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 = nullptr; /// Value found for the switch comparison<br>
+  Value *Extra = nullptr;  /// Extra clause to be checked before the switch<br>
+  SmallVector<ConstantInt*, 8> Vals; /// Set of integers to match in switch<br>
+  unsigned UsedICmps = 0; /// 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>Instr<u></u><u></u><u></u>uction *Cond, const DataLayout *DL) {<br>
+    gather(Cond, DL);<br>
+  }<br>
+<br>
+  /// Prevent copy<br>
+  ConstantComparesGatherer(const ConstantComparesGatherer&) = delete;<br>
+  ConstantComparesGatherer &operator=(const ConstantComparesGatherer&) = delete;<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></blockquote><div><br></div></div><div class="gmail_quote"><div>This looks like a typo.</div></div><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+  }<br>
+<br></blockquote><div> </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>
+  /// 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<u></u><u></u><u></u>), DL)))) {<br>
+      return false;<br>
+    }<br>
<br>
+    Value *RHSVal;<br>
+    ConstantInt *RHSC;<br>
<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>In<u></u><u></u><u></u>struction *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<u></u><u></u><u></u>), DL)))) {<br>
-    return nullptr;<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>IC<u></u><u></u><u></u>MP_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>ge<u></u><u></u><u></u>t(C->getContext(),<br>
-                                        C->getValue() | Not));<br>
-        UsedICmps++;<br>
-        return RHSVal;<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>IC<u></u><u></u><u></u>MP_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>ge<u></u><u></u><u></u>t(C->getContext(),<br>
+                                          C->getValue() | Not));<br>
+          UsedICmps++;<br>
+          return true;<br>
+        }<br>
       }<br>
-    }<br>
<br>
-    // If we already have a value for the switch, it has to match!<br>
-    if(CurrValue && CurrValue != ICI->getOperand(0))<br>
-      return nullptr;<br>
+      // If we already have a value for the switch, it has to match!<br>
+      if(!setValueOnce(ICI-><u></u>getOpera<u></u><u></u><u></u>nd(0)))<br>
+        return false;<br>
<br>
-    UsedICmps++;<br>
-    Vals.push_back(C);<br>
-    return ICI->getOperand(0);<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><u></u><u></u><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><u></u><u></u><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>
<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><u></u><u></u><u></u>ICI->getPredicate(),<br>
-                                                     C->getValue());<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>
+    }<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><u></u><u></u><u></u>);<br>
-    CandidateVal = RHSVal;<br>
-  }<br>
+    // If we already have a value for the switch, it has to match!<br>
+    if(!setValueOnce(CandidateVal)<u></u><u></u><u></u><u></u>)<br>
+      return false;<br>
<br>
-  // If we already have a value for the switch, it has to match!<br>
-  if(CurrValue && CurrValue != CandidateVal)<br>
-    return nullptr;<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>ge<u></u><u></u><u></u>t(I->getContext(), Tmp));<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>
+    UsedICmps++;<br>
+    return true;<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>ge<u></u><u></u><u></u>t(I->getContext(), Tmp));<br>
-<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>
+  /// 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><u></u><u></u><u></u>);<br>
+          DFT.push_back(I->getOperand(0)<u></u><u></u><u></u><u></u>);<br>
+          continue;<br>
+        }<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><u></u><u></u><u></u>);<br>
-        DFT.push_back(I->getOperand(0)<u></u><u></u><u></u><u></u>);<br>
-        continue;<br>
+        // Try to match the current instruction<br>
+        if (matchInstruction(I, DL, isEQ))<br>
+          // Match succeed, continue the loop<br>
+          continue;<br>
       }<br>
<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>
+      // 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>
         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>
-  // Return the value to be used for the switch comparison (if any)<br>
-  return CurrValue;<br>
-}<br>
+};<br>
<br>
 static void EraseTerminatorInstAndDCECond(<u></u><u></u><u></u><u></u>TerminatorInst *TI) {<br>
   Instruction *Cond = nullptr;<br>
@@ -2810,18 +2820,17 @@ static bool SimplifyBranchOnICmpChain(Br<br>
   Instruction *Cond = dyn_cast<Instruction>(BI-><u></u>getC<u></u><u></u><u></u>ondition());<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>
-  CompVal = GatherConstantCompares(Cond, Values, ExtraCase, DL, UsedICmps);<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>
<br>
   // If we didn't have a multiply compared value, fail.<br>
   if (!CompVal) return false;<br>
@@ -2830,6 +2839,8 @@ 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()<u></u><u></u><u></u>, Values.end(), ConstantIntSortPredicate);<br>
<br>
<br>
______________________________<u></u><u></u><u></u><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>mailm<u></u><u></u><u></u>an/listinfo/llvm-commits</a><br>
</blockquote></div></blockquote></div></div>
</div></blockquote></div><br></div></blockquote></div></div>
</div></blockquote></div><br></div></div></blockquote></div>