[clang-tools-extra] r225629 - Make LoopConvert work with containers that are used like arrays.

[Daniel Jasper]

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.

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/328f5f0c/attachment.html>