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

[Arthur O'Dwyer via Phabricator via llvm-commits]

Quuxplusone marked 2 inline comments as done.
Quuxplusone added inline comments.


================
Comment at: include/llvm/Support/BranchProbability.h:131-133
     BranchProbability Prob(*this);
-    return Prob += RHS;
+    Prob += RHS;
+    return Prob;
----------------
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.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:7845
   if (ShouldInvert)
-    return Cmp = DAG.getNOT(dl, Cmp, Cmp.getValueType());
+    Cmp = DAG.getNOT(dl, Cmp, Cmp.getValueType());
 
----------------
dblaikie wrote:
> Quuxplusone wrote:
> > dblaikie wrote:
> > > This looks like a good change regardless - the original phrasing is pretty weird, given that Cmp is going out of scope anyway, it could've been written as "return DAG.getNOT(...);" & skipped the assignment, but your solution here's more consistent with the surrounding code, I think
> > Agreed.
> > I don't have commit privs; could someone commit this for me?
> Sure! Done in r347609
Thanks!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54885