[llvm] r253964 - [TableGen] Use std::remove_if instead of manually coded loops that call erase multiple times. NFC
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Nov 24 11:39:30 PST 2015
On Tue, Nov 24, 2015 at 11:30 AM, Craig Topper <craig.topper at gmail.com>
wrote:
>
>
> On Tue, Nov 24, 2015 at 10:06 AM, David Blaikie <dblaikie at gmail.com>
> wrote:
>
>>
>>
>> On Tue, Nov 24, 2015 at 12:20 AM, Craig Topper via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: ctopper
>>> Date: Tue Nov 24 02:20:47 2015
>>> New Revision: 253964
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=253964&view=rev
>>> Log:
>>> [TableGen] Use std::remove_if instead of manually coded loops that call
>>> erase multiple times. NFC
>>>
>>> Modified:
>>> llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp
>>>
>>> Modified: llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp?rev=253964&r1=253963&r2=253964&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp (original)
>>> +++ llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp Tue Nov 24 02:20:47
>>> 2015
>>> @@ -385,55 +385,55 @@ bool EEVT::TypeSet::EnforceSmallerThan(E
>>> // type size is smaller than the scalar size of the smallest type. For
>>> // vectors, we also need to make sure that the total size is no
>>> larger than
>>> // the size of the smallest type.
>>> - TypeSet InputSet(Other);
>>> - MVT Smallest = TypeVec[0];
>>> - for (unsigned i = 0; i != Other.TypeVec.size(); ++i) {
>>> - MVT OtherVT = Other.TypeVec[i];
>>> - // Don't compare vector and non-vector types.
>>> - if (OtherVT.isVector() != Smallest.isVector())
>>> - continue;
>>> - // The getSizeInBits() check here is only needed for vectors, but is
>>> - // a subset of the scalar check for scalars so no need to qualify.
>>> - if (OtherVT.getScalarSizeInBits() <= Smallest.getScalarSizeInBits()
>>> ||
>>> - OtherVT.getSizeInBits() < Smallest.getSizeInBits()) {
>>> - Other.TypeVec.erase(Other.TypeVec.begin()+i--);
>>> - MadeChange = true;
>>> + {
>>>
>>
>> Is this explicit scope ^ particularly helpful?
>>
>
> The scope is only there to void having the very similar code for Largest
> have to declare differently named temporaries or assume they already
> existed and just assign them instead of assigning and declaring. Thoughts?
>
Ah, I see, thanks for the explanation (I skimmed the patch & was wondering
why I was seeing the same diff twice, but figured I might've just scrolled
weirdly or something... ;)).
That makes sense - and it looks like the differences are enoguh that it'd
be difficult/annoying/not beneficial to refactor out into some generic
routine that could be used easily for both largest and smallest. And yeah,
dropping the scope & having the first one have declarations and the second
not - that's probably what I'd do, but the explicit scopes aren't a bad
tool to use there either, no worries.
>
>
>>
>>
>>> + TypeSet InputSet(Other);
>>> + MVT Smallest = TypeVec[0];
>>> + auto I = std::remove_if(Other.TypeVec.begin(), Other.TypeVec.end(),
>>> + [Smallest](MVT OtherVT) {
>>>
>>
>> ^ Just as a general idea, I'd like to encourage the use of default
>> capture by reference ("[&]") for any lambda that's only being used in the
>> scope of its definition (ie: any standard algorithm or similar device -
>> almost any time the lambda isn't escaping through a std::function,
>> potentially). I think it simplifies the code, avoids curious gotchas (I've
>> seen some recent local/algorithm lambdas with value-by-default ("[=]")
>> which I imagine could trip someone up on refactoring.
>>
>> What do you think/what's your preference/model here?
>>
>
> I think that makes sense.
>
>
>>
>>
>>> + // Don't compare vector and non-vector types.
>>> + if (OtherVT.isVector() != Smallest.isVector())
>>> + return false;
>>> + // The getSizeInBits() check here is only needed for vectors,
>>> but is
>>> + // a subset of the scalar check for scalars so no need to
>>> qualify.
>>> + return OtherVT.getScalarSizeInBits() <=
>>> Smallest.getScalarSizeInBits()||
>>> + OtherVT.getSizeInBits() < Smallest.getSizeInBits();
>>> + });
>>> + MadeChange |= I != Other.TypeVec.end(); // If we're about to remove
>>> types.
>>> + Other.TypeVec.erase(I, Other.TypeVec.end());
>>> +
>>> + if (Other.TypeVec.empty()) {
>>> + TP.error("Type inference contradiction found, '" +
>>> InputSet.getName() +
>>> + "' has nothing larger than '" + getName() +"'!");
>>> + return false;
>>> }
>>> }
>>>
>>> - if (Other.TypeVec.empty()) {
>>> - TP.error("Type inference contradiction found, '" +
>>> InputSet.getName() +
>>> - "' has nothing larger than '" + getName() +"'!");
>>> - return false;
>>> - }
>>> -
>>> // Okay, find the largest type from the other set and remove anything
>>> the
>>> // same or smaller from the current set. We need to ensure that the
>>> scalar
>>> // type size is larger than the scalar size of the largest type. For
>>> // vectors, we also need to make sure that the total size is no
>>> smaller than
>>> // the size of the largest type.
>>> - InputSet = TypeSet(*this);
>>> - MVT Largest = Other.TypeVec[Other.TypeVec.size()-1];
>>> - for (unsigned i = 0; i != TypeVec.size(); ++i) {
>>> - MVT OtherVT = TypeVec[i];
>>> - // Don't compare vector and non-vector types.
>>> - if (OtherVT.isVector() != Largest.isVector())
>>> - continue;
>>> - // The getSizeInBits() check here is only needed for vectors, but is
>>> - // a subset of the scalar check for scalars so no need to qualify.
>>> - if (OtherVT.getScalarSizeInBits() >= Largest.getScalarSizeInBits()
>>> ||
>>> - OtherVT.getSizeInBits() > Largest.getSizeInBits()) {
>>> - TypeVec.erase(TypeVec.begin()+i--);
>>> - MadeChange = true;
>>> + {
>>> + TypeSet InputSet(*this);
>>> + MVT Largest = Other.TypeVec[Other.TypeVec.size()-1];
>>> + auto I = std::remove_if(TypeVec.begin(), TypeVec.end(),
>>> + [Largest](MVT OtherVT) {
>>> + // Don't compare vector and non-vector types.
>>> + if (OtherVT.isVector() != Largest.isVector())
>>> + return false;
>>> + return OtherVT.getScalarSizeInBits() >=
>>> Largest.getScalarSizeInBits() ||
>>> + OtherVT.getSizeInBits() > Largest.getSizeInBits();
>>> + });
>>> + MadeChange |= I != TypeVec.end(); // If we're about to remove types.
>>> + TypeVec.erase(I, TypeVec.end());
>>> +
>>> + if (TypeVec.empty()) {
>>> + TP.error("Type inference contradiction found, '" +
>>> InputSet.getName() +
>>> + "' has nothing smaller than '" + Other.getName() +"'!");
>>> + return false;
>>> }
>>> }
>>>
>>> - if (TypeVec.empty()) {
>>> - TP.error("Type inference contradiction found, '" +
>>> InputSet.getName() +
>>> - "' has nothing smaller than '" + Other.getName() +"'!");
>>> - return false;
>>> - }
>>> -
>>> return MadeChange;
>>> }
>>>
>>> @@ -448,17 +448,17 @@ bool EEVT::TypeSet::EnforceVectorEltType
>>> TypeSet InputSet(*this);
>>>
>>> // Filter out all the types which don't have the right element type.
>>> - for (unsigned i = 0; i != TypeVec.size(); ++i) {
>>> - assert(isVector(TypeVec[i]) && "EnforceVector didn't work");
>>> - if (MVT(TypeVec[i]).getVectorElementType().SimpleTy != VT) {
>>> - TypeVec.erase(TypeVec.begin()+i--);
>>> - MadeChange = true;
>>> - }
>>> - }
>>> + auto I = std::remove_if(TypeVec.begin(), TypeVec.end(),
>>> + [VT](MVT VVT) {
>>> + return VVT.getVectorElementType().SimpleTy != VT;
>>> + });
>>> + MadeChange |= I != TypeVec.end();
>>> + TypeVec.erase(I, TypeVec.end());
>>>
>>> if (TypeVec.empty()) { // FIXME: Really want an SMLoc here!
>>> TP.error("Type inference contradiction found, forcing '" +
>>> - InputSet.getName() + "' to have a vector element");
>>> + InputSet.getName() + "' to have a vector element of type "
>>> +
>>> + getEnumName(VT));
>>> return false;
>>> }
>>>
>>> @@ -535,13 +535,13 @@ bool EEVT::TypeSet::EnforceVectorSubVect
>>> // Only keep types that have less elements than VTOperand.
>>> TypeSet InputSet(VTOperand);
>>>
>>> - for (unsigned i = 0; i != VTOperand.TypeVec.size(); ++i) {
>>> - assert(isVector(VTOperand.TypeVec[i]) && "EnforceVector didn't
>>> work");
>>> - if (MVT(VTOperand.TypeVec[i]).getVectorNumElements() >= NumElems)
>>> {
>>> - VTOperand.TypeVec.erase(VTOperand.TypeVec.begin()+i--);
>>> - MadeChange = true;
>>> - }
>>> - }
>>> + auto I = std::remove_if(VTOperand.TypeVec.begin(),
>>> VTOperand.TypeVec.end(),
>>> + [NumElems](MVT VVT) {
>>> + return VVT.getVectorNumElements() >=
>>> NumElems;
>>> + });
>>> + MadeChange |= I != VTOperand.TypeVec.end();
>>> + VTOperand.TypeVec.erase(I, VTOperand.TypeVec.end());
>>> +
>>> if (VTOperand.TypeVec.empty()) { // FIXME: Really want an SMLoc
>>> here!
>>> TP.error("Type inference contradiction found, forcing '" +
>>> InputSet.getName() + "' to have less vector elements
>>> than '" +
>>> @@ -559,13 +559,13 @@ bool EEVT::TypeSet::EnforceVectorSubVect
>>> // Only keep types that have more elements than 'this'.
>>> TypeSet InputSet(*this);
>>>
>>> - for (unsigned i = 0; i != TypeVec.size(); ++i) {
>>> - assert(isVector(TypeVec[i]) && "EnforceVector didn't work");
>>> - if (MVT(TypeVec[i]).getVectorNumElements() <= NumElems) {
>>> - TypeVec.erase(TypeVec.begin()+i--);
>>> - MadeChange = true;
>>> - }
>>> - }
>>> + auto I = std::remove_if(TypeVec.begin(), TypeVec.end(),
>>> + [NumElems](MVT VVT) {
>>> + return VVT.getVectorNumElements() <=
>>> NumElems;
>>> + });
>>> + MadeChange |= I != TypeVec.end();
>>> + TypeVec.erase(I, TypeVec.end());
>>> +
>>> if (TypeVec.empty()) { // FIXME: Really want an SMLoc here!
>>> TP.error("Type inference contradiction found, forcing '" +
>>> InputSet.getName() + "' to have more vector elements
>>> than '" +
>>> @@ -597,13 +597,13 @@ bool EEVT::TypeSet::EnforceVectorSameNum
>>> // Only keep types that have same elements as VTOperand.
>>> TypeSet InputSet(VTOperand);
>>>
>>> - for (unsigned i = 0; i != VTOperand.TypeVec.size(); ++i) {
>>> - assert(isVector(VTOperand.TypeVec[i]) && "EnforceVector didn't
>>> work");
>>> - if (MVT(VTOperand.TypeVec[i]).getVectorNumElements() != NumElems)
>>> {
>>> - VTOperand.TypeVec.erase(VTOperand.TypeVec.begin()+i--);
>>> - MadeChange = true;
>>> - }
>>> - }
>>> + auto I = std::remove_if(VTOperand.TypeVec.begin(),
>>> VTOperand.TypeVec.end(),
>>> + [NumElems](MVT VVT) {
>>> + return VVT.getVectorNumElements() !=
>>> NumElems;
>>> + });
>>> + MadeChange |= I != VTOperand.TypeVec.end();
>>> + VTOperand.TypeVec.erase(I, VTOperand.TypeVec.end());
>>> +
>>> if (VTOperand.TypeVec.empty()) { // FIXME: Really want an SMLoc
>>> here!
>>> TP.error("Type inference contradiction found, forcing '" +
>>> InputSet.getName() + "' to have same number elements as
>>> '" +
>>> @@ -617,13 +617,13 @@ bool EEVT::TypeSet::EnforceVectorSameNum
>>> // Only keep types that have same elements as 'this'.
>>> TypeSet InputSet(*this);
>>>
>>> - for (unsigned i = 0; i != TypeVec.size(); ++i) {
>>> - assert(isVector(TypeVec[i]) && "EnforceVector didn't work");
>>> - if (MVT(TypeVec[i]).getVectorNumElements() != NumElems) {
>>> - TypeVec.erase(TypeVec.begin()+i--);
>>> - MadeChange = true;
>>> - }
>>> - }
>>> + auto I = std::remove_if(TypeVec.begin(), TypeVec.end(),
>>> + [NumElems](MVT VVT) {
>>> + return VVT.getVectorNumElements() !=
>>> NumElems;
>>> + });
>>> + MadeChange |= I != TypeVec.end();
>>> + TypeVec.erase(I, TypeVec.end());
>>> +
>>> if (TypeVec.empty()) { // FIXME: Really want an SMLoc here!
>>> TP.error("Type inference contradiction found, forcing '" +
>>> InputSet.getName() + "' to have same number elements
>>> than '" +
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>>
>>
>
>
> --
> ~Craig
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151124/e1918c2c/attachment-0001.html>
More information about the llvm-commits
mailing list