[llvm] r222416 - SimplifyCFG: Refactor GatherConstantCompares() result in a struct
Timur Iskhodzhanov
timurrrr at google.com
Thu Nov 20 23:29:53 PST 2014
Thanks!
пт, 21 нояб. 2014, 2:31, Mehdi Amini <mehdi.amini at apple.com>:
> 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>
> 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>
>> 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
>>
>> On Thu Nov 20 2014 at 2:51:06 PM Timur Iskhodzhanov <timurrrr at google.com>
>> wrote:
>>
>>> On Thu Nov 20 2014 at 9:53:42 AM Mehdi Amini <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
>>>> 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/Transform
>>>> s/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
>>>> 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/20141121/d84ad9df/attachment.html>
More information about the llvm-commits
mailing list