[llvm] r222416 - SimplifyCFG: Refactor GatherConstantCompares() result in a struct
Timur Iskhodzhanov
timurrrr at google.com
Thu Nov 20 12:44:59 PST 2014
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/20141120/c7d6c71f/attachment.html>
More information about the llvm-commits
mailing list