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>