[PATCH] D54885: Assigning to a local object in a return statement prevents copy elision. NFC.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/Support/BranchProbability.h:131-133
     BranchProbability Prob(*this);
-    return Prob += RHS;
+    Prob += RHS;
+    return Prob;
----------------
Quuxplusone wrote:
> dblaikie wrote:
> > Quuxplusone wrote:
> > > dblaikie wrote:
> > > > Would an alternative fix for this be to write ref-qualified overloads of opX=, then use them like:
> > > > 
> > > >   return BranchProbability(*this) += RHS;
> > > > 
> > > > ? (or, I suppose, "BranchProbability Prob(*this); return std::move(Prob) += RHS;")
> > > Yes, absolutely that would work; but it would be horribly un-idiomatic.
> > > Also, some wise people say that every assignment operator should ideally be //lvalue//-ref-qualified, and you're [hypothetically] suggesting to add //rvalue//-ref-qualified versions purely to enable an un-idiomatic style... I don't think it would be a good idea.
> > Could we do something the other way around?
> > 
> > Could op+ be implemented with either a value passed left parameter?
> > 
> > Alternatively - should this warning/fixes be restricted to types with non-trivial copy construction? Because I'm not sure this code would be made any worse by the code being as-is? I would imagine it might produce the same code with or without your change, once optimized?
> > Could op+ be implemented with either a value passed left parameter?
> 
> Yes, and sometimes it's done that way, idiomatically. Then there'd be no question of copy-elision (copy-elision would be impossible by definition). But `return lhs += rhs;` would still disable implicit move and force the return object to be initialized by //copy// instead of by //move//.
> 
> In this case, because `BranchProbability` is trivially copyable, the difference between copy and move doesn't matter; and because it's no bigger than 16 bytes and all these methods are inlined, copy elision doesn't matter either, you're right. Once you get up to 20 bytes, copy elision starts looking good even for trivially copyable types.
> https://godbolt.org/z/Tfukus
> 
> I still think this should be committed for code-cleanliness purposes (e.g. if we get a clang-tidy check to warn about potentially pessimizing `return x @= y` constructions, then this "false positive" in the codebase would be annoying)... but I suspect you're right that it cannot possibly matter to the physical codegen in this case.
*nod* I'm wondering whether such a theoretical/actual clang-tidy warning could be improved to not warn on this code, due to the trivial copy/move.

I guess if some of the operations weren't inlined, then LLVM wouldn't understand the aliasing relationships well enough to be able to essentially do NRVO itself? So it'd be hard for such a clang-tidy warning to differentiate this entirely benign case from similar-but-different non-benign cases (even when all cases under cconsideration had trivial move/copy ops, etc)




Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54885/new/

https://reviews.llvm.org/D54885