[llvm] ef89409 - Fix 'unused-lambda-capture' gcc warning. NFCI.

[David Blaikie via llvm-commits]

On Mon, Oct 17, 2022 at 6:03 PM Alexey Bataev <a.bataev at hotmail.com> wrote:
>
>
>
> Best regards,
> Alexey Bataev
>
> > 17 окт. 2022 г., в 8:49 PM, David Blaikie <dblaikie at gmail.com> написал(а):
> > On Mon, Oct 17, 2022 at 5:46 PM Alexey Bataev <a.bataev at hotmail.com> wrote:
> >>
> >>
> >>
> >> Best regards,
> >> Alexey Bataev
> >>
> >>> 17 окт. 2022 г., в 8:36 PM, David Blaikie <dblaikie at gmail.com> написал(а):
> >>> On Mon, Oct 17, 2022 at 4:58 PM Alexey Bataev <a.bataev at hotmail.com> wrote:
> >>>> Best regards,
> >>>> Alexey Bataev
> >>>>>> 17 окт. 2022 г., в 7:26 PM, David Blaikie <dblaikie at gmail.com> написал(а):
> >>>>> ping on this
> >>>>>> On Mon, Oct 3, 2022 at 12:44 PM David Blaikie <dblaikie at gmail.com> wrote:
> >>>>>>> On Tue, Sep 27, 2022 at 7:16 AM Simon Pilgrim via llvm-commits
> >>>>>>> <llvm-commits at lists.llvm.org> wrote:
> >>>>>>> Author: Simon Pilgrim
> >>>>>>> Date: 2022-09-27T15:15:43+01:00
> >>>>>>> New Revision: ef89409a59f3b79ae143b33b7d8e6ee6285aa42f
> >>>>>>> URL: https://github.com/llvm/llvm-project/commit/ef89409a59f3b79ae143b33b7d8e6ee6285aa42f
> >>>>>>> DIFF: https://github.com/llvm/llvm-project/commit/ef89409a59f3b79ae143b33b7d8e6ee6285aa42f.diff
> >>>>>>> LOG: Fix 'unused-lambda-capture' gcc warning. NFCI.
> >>>>>>> Added:
> >>>>>>> Modified:
> >>>>>>>  llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
> >>>>>>> Removed:
> >>>>>>> ################################################################################
> >>>>>>> diff  --git a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
> >>>>>>> index b5c73819335e..9584c421bcca 100644
> >>>>>>> --- a/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
> >>>>>>> +++ b/llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp
> >>>>>>> @@ -6075,7 +6075,7 @@ InstructionCost BoUpSLP::getEntryCost(const TreeEntry *E,
> >>>>>>> bool NeedToShuffleReuses = !E->ReuseShuffleIndices.empty();
> >>>>>>> // FIXME: it tries to fix a problem with MSVC buildbots.
> >>>>>>> TargetTransformInfo &TTIRef = *TTI;
> >>>>>>> -  auto &&AdjustExtractsCost = [this, &TTIRef, CostKind, VL, VecTy,
> >>>>>>> +  auto &&AdjustExtractsCost = [this, &TTIRef, CostKind, VL,
> >>>>>>>                              VectorizedVals, E](InstructionCost &Cost) {
> >>>>>> Generally if a lambda's only going to be used in the current scope,
> >>>>>> I'd suggest using default capture "[&]" so the list doesn't need to be
> >>>>>> maintained.
> >>>> Thera are no strict rules about it, I prefer to keep explicit list where possible to be sure what's used or can be dropped.
> >>> It's a bit of a maintenance burden, though - in the same way that we
> >>> don't enumerate what variables are used in a while loop or other scope
> >>> - I think it's suitable for local lambdas not to enumerate them so
> >>> it's easier to update/modify the code without having to maintain the
> >>> list.
> >>>>>> Also rvalue reference lifetime extension's probably not helpful here,
> >>>>>> and just plain `auto AdjustExtractsCost` could be used.
> >>>> It saves us from one move constructor call in AST (not in codegen, it is elided then).
> >>> Yeah, nothing to save there - as you say, it doesn't get codegen'd
> >>> that way. Using `auto` non-reference is used in almost all cases - be
> >>> nice to change this for consistency, so it doesn't raise questions of
> >>> "why is this different? is there something importantly odd/unique
> >>> going on here?"
> >>
> >> Some time before I was asked to do different thing.
> >> Actually we save some memory on this.
> >
> > Got a link? Not sure how we'd save memory here - Generally I'd expect
> > we'd use memory if we explicitly enumerate captures compared to
> > default ref capture. (oh, I guess if the objects are smaller than a
> > pointer, then capture by value could save memory) - but I really don't
> > think it's worthwhile for a locally scoped lambda in terms of
> > maintenance costs when modifying code - unless there's some
> > particularly compelling use case/data in specific uses that shows it
> > needs to be further tuned/hand-optimized like that.
> >
>
> No, it was some time ago, will not find it already.
> The memory saved because we do not create AST node for constructor call.
> But, IIRC, in C++17 it becomes r-value reference already. We try to qualify auto type as much as possible (with const, * and &), shall we do here the same?

Nah, that's OK - the rule is "if it's implicit in auto, and
cheap/short to make it explicit, we should" - so if the auto is hiding
a qualifier (const, etc), pointer or reference type, we should make it
explicit instead of it being hidden inside auto. If auto isn't hiding
any of those things, no need to add them.

>
> > - Dave
> >
> >>
> >>> - Dave
> >>>>>>>   ScalarizationOverheadBuilder ScalarizationCost;
> >>>>>>>   SmallPtrSet<Value *, 4> CheckedExtracts;
> >>>>>>> _______________________________________________
> >>>>>>> llvm-commits mailing list
> >>>>>>> llvm-commits at lists.llvm.org
> >>>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits