[llvm] r237224 - Change a loop in LoopInfo to foreach. NFC

[David Blaikie]

On Wed, May 13, 2015 at 1:00 PM, Pete Cooper <peter_cooper at apple.com> wrote:

>
> On May 13, 2015, at 11:56 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Wed, May 13, 2015 at 11:48 AM, Pete Cooper <peter_cooper at apple.com>
> wrote:
>
>>
>> On May 13, 2015, at 10:51 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Tue, May 12, 2015 at 6:12 PM, Pete Cooper <peter_cooper at apple.com>
>> wrote:
>>
>>> Author: pete
>>> Date: Tue May 12 20:12:09 2015
>>> New Revision: 237224
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=237224&view=rev
>>> Log:
>>> Change a loop in LoopInfo to foreach.  NFC
>>>
>>> Modified:
>>>     llvm/trunk/lib/Analysis/LoopInfo.cpp
>>>
>>> Modified: llvm/trunk/lib/Analysis/LoopInfo.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/LoopInfo.cpp?rev=237224&r1=237223&r2=237224&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/Analysis/LoopInfo.cpp (original)
>>> +++ llvm/trunk/lib/Analysis/LoopInfo.cpp Tue May 12 20:12:09 2015
>>> @@ -65,8 +65,8 @@ bool Loop::isLoopInvariant(const Value *
>>>  /// hasLoopInvariantOperands - Return true if all the operands of the
>>>  /// specified instruction are loop invariant.
>>>  bool Loop::hasLoopInvariantOperands(const Instruction *I) const {
>>> -  for (unsigned i = 0, e = I->getNumOperands(); i != e; ++i)
>>> -    if (!isLoopInvariant(I->getOperand(i)))
>>> +  for (auto &Op : I->operands())
>>> +    if (!isLoopInvariant(Op))
>>>        return false;
>>>
>>
>> Could be written as:
>>
>>   return std::all_of(I->operands().begin(), I->operands().end(),
>> [&](Value *V) { return isLoopInvariant(V); );
>>
>> If you like. (but the range-based-ness does make that a bit inconvenient,
>> though std::all_of adds a little readability improvement, I think - at some
>> point we'll just write range-based versions of algorithms like this so we
>> don't have to pass begin/end everywhere)
>>
>> Thanks.  Something like this:
>>
>> namespace llvm {
>>
>> template<typename T, class UnaryPredicate>
>>   static inline bool all_of(llvm::iterator_range<T> Range,
>> UnaryPredicate p) {
>>   return std::all_of(Range.begin(), Range.end(), p);
>> }
>>
>> };
>>
>> bool Loop::hasLoopInvariantOperands(const Instruction *I) const {
>>   return all_of(I->operands(), [&](Value *V) { return isLoopInvariant(V);
>> });
>> }
>>
>> I chose to constrain is to iterator_range for now just to avoid
>> complicating it to require begin/end of any kind of container.
>>
>
> I wouldn't bother with the constraint - I'd just start with a generic ref
> range R:
>
> Sounds good to me.  Unfortunately operands is an iterator_range which
> can’t be passed by reference:
>
> *../lib/Analysis/LoopInfo.cpp:84:10: **error: **no matching function for
> call to 'all_of'*
>   return all_of(I->operands(), [this](Value *V) { return
> isLoopInvariant(V); });
> *         ^~~~~~*
> *../lib/Analysis/LoopInfo.cpp:67:20: note: *candidate function [with R =
> llvm::iterator_range<const llvm::Use *>, UnaryPredicate = (lambda at
> ../lib/Analysis/LoopInfo.cpp:84:32)] not viable: expects an l-value for 1st
> argument
> static inline bool all_of(R &Range, UnaryPredicate p) {
>
> So I ended up having to define by value and by reference all_of.
>

Probably better to have a const ref & non-const ref overload. By value
risks copying things in cases like:

  const std::vector<int> &v = ...;
  ...
  any_of(v, ...);

That's what I was trying to avoid by suggesting pass by ref rather than
value.


>
> template<typename R, class UnaryPredicate>
> static inline bool all_of(R Range, UnaryPredicate p) {
>   return std::all_of(Range.begin(), Range.end(), p);
> }
>
> template<typename R, class UnaryPredicate>
> static inline bool all_of(R &Range, UnaryPredicate p) {
>   return std::all_of(Range.begin(), Range.end(), p);
> }
>
> Pete
>
>
> template<typename R, typename UnaryPredicate>
> bool all_of(R &Range, UnaryPredicate p) {
>   return std::all_of(Range.begin(), Range.end(), p);
> }
>
> And if we one day need to, we can SFINAE this as appropriate. (heh, now
> I'm imagining writing this as an adaptor... but of course it wouldn't work
> because we need template argument deduction, etc... but it'd be fun to
> "rangify(std::all_of, Range, Predicate)" ;))
>
>
>>  What do you think?
>>
>> Cheers,
>> Pete
>>
>>
>>
>>>
>>>    return true;
>>>
>>>
>>> _______________________________________________
>>> 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/20150513/5e9dd095/attachment.html>