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

[David Blaikie]

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/20150112/22bf0544/attachment.html>