[llvm] r222416 - SimplifyCFG: Refactor GatherConstantCompares() result in a struct
Mehdi Amini
mehdi.amini at apple.com
Thu Nov 20 08:33:56 PST 2014
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> 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/34db7fc3/attachment.html>
More information about the llvm-commits
mailing list