[clang-tools-extra] r225629 - Make LoopConvert work with containers that are used like arrays.
David Blaikie
dblaikie at gmail.com
Tue Jan 13 10:07:56 PST 2015
On Tue, Jan 13, 2015 at 6:59 AM, Daniel Jasper <djasper at google.com> wrote:
> I see. At any rate, the patch merely simplifies the loop variable for such
> containers. It does not change the behavior wrt. whether or not a loop is
> transformed.
>
Ah, fair enough.
What did the transformation look like before your change?
>
> On Mon, Jan 12, 2015 at 7:36 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>>
>>
>> On Mon, Jan 12, 2015 at 9:44 AM, Daniel Jasper <djasper at google.com>
>> wrote:
>>
>>> Thanks for checking. I'll look into it.
>>>
>>
>> For more context/my thoughts: I think there will be cases we just can't
>> detect/get right (if there's a function call in the loop and the container
>> leaks somehow (it's a member variable, etc) we can't know if the user wants
>> the special behavior or it's just a normal every day loop). I think
>> clang-modernize has buckets for degree-of-risk for different
>> transformations & this one may just need to go in a slightly risky bucket.
>>
>> I'm not sure if there's a nice way for a user to annotate such a loop to
>> make it clear to tools that this extra behavior is required.
>>
>>
>>> Do you have specific examples?
>>>
>>
>> greping around I did find at least one, in
>> llvm/lib/Analysis/IPA/InlineCost.cpp:
>>
>> // Note that we *must not* cache the size, this loop grows the worklist.
>> for (unsigned Idx = 0; Idx != BBWorklist.size(); ++Idx) {
>>
>> Finding the second kind (the one that caches size() deliberately so as to
>> only visit the existing elements while new ones are being added) is harder
>> to find, since it's more idiomatic in LLVM to cache the size. I know I ran
>> into at least one such loop in the last few weeks, though.
>>
>> - David
>>
>>
>>>
>>> On Mon, Jan 12, 2015 at 6:41 PM, David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>>
>>>>
>>>> On Mon, Jan 12, 2015 at 5:17 AM, Daniel Jasper <djasper at google.com>
>>>> wrote:
>>>>
>>>>> Author: djasper
>>>>> Date: Mon Jan 12 07:17:56 2015
>>>>> New Revision: 225629
>>>>>
>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=225629&view=rev
>>>>> Log:
>>>>> Make LoopConvert work with containers that are used like arrays.
>>>>>
>>>>
>>>> Are these transformations classified as 'risky' in some way? These sort
>>>> of loops appear in a few places in LLVM deliberately because we're adding
>>>> elements to the sequence as we're iterating - so we check size each time
>>>> through and we access the vector with [] because its buffer might've been
>>>> invalidated by new insertions. (even with size() accessed once and cached,
>>>> we sometimes do this to visit pre-existing elements and ignore newly added
>>>> elements - which would still be broken by a transformation to
>>>> range-based-for/iterators)
>>>>
>>>>
>>>>>
>>>>> Modified:
>>>>> clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp
>>>>>
>>>>> clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp
>>>>>
>>>>> Modified:
>>>>> clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp?rev=225629&r1=225628&r2=225629&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> ---
>>>>> clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp
>>>>> (original)
>>>>> +++
>>>>> clang-tools-extra/trunk/clang-modernize/LoopConvert/LoopActions.cpp Mon Jan
>>>>> 12 07:17:56 2015
>>>>> @@ -450,9 +450,16 @@ static bool isAliasDecl(const Decl *TheD
>>>>> const CXXOperatorCallExpr *OpCall =
>>>>> cast<CXXOperatorCallExpr>(Init);
>>>>> if (OpCall->getOperator() == OO_Star)
>>>>> return isDereferenceOfOpCall(OpCall, IndexVar);
>>>>> + if (OpCall->getOperator() == OO_Subscript) {
>>>>> + assert(OpCall->getNumArgs() == 2);
>>>>> + return true;
>>>>> + }
>>>>> break;
>>>>> }
>>>>>
>>>>> + case Stmt::CXXMemberCallExprClass:
>>>>> + return true;
>>>>> +
>>>>> default:
>>>>> break;
>>>>> }
>>>>>
>>>>> Modified:
>>>>> clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp
>>>>> URL:
>>>>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp?rev=225629&r1=225628&r2=225629&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> ---
>>>>> clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp
>>>>> (original)
>>>>> +++
>>>>> clang-tools-extra/trunk/test/clang-modernize/LoopConvert/naming-alias.cpp
>>>>> Mon Jan 12 07:17:56 2015
>>>>> @@ -7,6 +7,8 @@
>>>>> const int N = 10;
>>>>>
>>>>> Val Arr[N];
>>>>> +dependent<Val> v;
>>>>> +dependent<Val> *pv;
>>>>> Val &func(Val &);
>>>>> void sideEffect(int);
>>>>>
>>>>> @@ -50,6 +52,25 @@ void aliasing() {
>>>>> // CHECK-NEXT: int y = t.x;
>>>>> // CHECK-NEXT: int z = elem.x + t.x;
>>>>>
>>>>> + // The same for pseudo-arrays like std::vector<T> (or here
>>>>> dependent<Val>)
>>>>> + // which provide a subscript operator[].
>>>>> + for (int i = 0; i < v.size(); ++i) {
>>>>> + Val &t = v[i]; { }
>>>>> + int y = t.x;
>>>>> + }
>>>>> + // CHECK: for (auto & t : v)
>>>>> + // CHECK-NEXT: { }
>>>>> + // CHECK-NEXT: int y = t.x;
>>>>> +
>>>>> + // The same with a call to at()
>>>>> + for (int i = 0; i < pv->size(); ++i) {
>>>>> + Val &t = pv->at(i); { }
>>>>> + int y = t.x;
>>>>> + }
>>>>> + // CHECK: for (auto & t : *pv)
>>>>> + // CHECK-NEXT: { }
>>>>> + // CHECK-NEXT: int y = t.x;
>>>>> +
>>>>> for (int i = 0; i < N; ++i) {
>>>>> Val &t = func(Arr[i]);
>>>>> int y = t.x;
>>>>>
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>>
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150113/205a856f/attachment.html>
More information about the cfe-commits
mailing list