[llvm] r222416 - SimplifyCFG: Refactor GatherConstantCompares() result in a struct

Mehdi Amini mehdi.amini at apple.com
Thu Nov 20 15:31:21 PST 2014


I tried a new version, let me know if you see any other issue.

Thanks,

Mehdi

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141120/2a86c769/attachment.html>


More information about the llvm-commits mailing list