Assigning to a local object in a return statement prevents copy elision. NFC.

[• Quuxplusone]

Summary

I added a diagnostic along the lines of -Wpessimizing-move to detect return x = y suppressing copy elision, but I don't know if the diagnostic is really worth it. Anyway, here are the places where my diagnostic reported that copy elision would have been possible if not for the assignment.

P1155R1 in the post-San-Diego WG21 (C++ committee) mailing discusses whether WG21 should fix this pitfall by just changing the core language to permit copy elision in cases like these.

(Kona update: The bulk of P1155 is proceeding to CWG review, but specifically *not* the parts that explored the notion of permitting copy-elision in these specific cases.)

[dblaikie]

include/llvm/Support/BranchProbability.h

131–133

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;")

lib/Target/AArch64/AArch64ISelLowering.cpp

7845

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

[• Quuxplusone]

include/llvm/Support/BranchProbability.h

131–133

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.

lib/Target/AArch64/AArch64ISelLowering.cpp

7845

Agreed.
I don't have commit privs; could someone commit this for me?

[dblaikie]

include/llvm/Support/BranchProbability.h

131–133

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?

lib/Target/AArch64/AArch64ISelLowering.cpp

7845

Sure! Done in r347609

[• Quuxplusone]

include/llvm/Support/BranchProbability.h

131–133

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.

lib/Target/AArch64/AArch64ISelLowering.cpp

7845

Thanks!

[dblaikie]

include/llvm/Support/BranchProbability.h

131–133

*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)

[• Quuxplusone]

Extend my check to include return ++x, which catches a couple more cases. These are iterator types, which means they are presumably "cheap" to copy; but they aren't trivial, unless my checker has a bug. :)

@steveire suggests that I should try turning my checker into a clang-tidy matcher.

[dblaikie]

Sounds alright to me - thanks!

[• Quuxplusone]

@dblaikie merged one of the original diffs on 2018-11-26, but the rest are still extant.
Rebased on master. Could I get someone to land the remainder of this patch for me and then close it?

include/llvm/Support/BranchProbability.h

BranchProbability &operator/=(uint32_t RHS) {
     assert(N != UnknownN &&
            "Unknown probability cannot participate in arithmetics.");
     assert(RHS > 0 && "The divider cannot be zero.");
     N /= RHS;
     return *this;
   }
 
   BranchProbability operator+(BranchProbability RHS) const {
     BranchProbability Prob(*this);
-    return Prob += RHS;
+    Prob += RHS;
+    return Prob;
   }
 
   BranchProbability operator-(BranchProbability RHS) const {
     BranchProbability Prob(*this);
-    return Prob -= RHS;
+    Prob -= RHS;
+    return Prob;
   }
 
   BranchProbability operator*(BranchProbability RHS) const {
     BranchProbability Prob(*this);
-    return Prob *= RHS;
+    Prob *= RHS;
+    return Prob;
   }
 
   BranchProbability operator*(uint32_t RHS) const {
     BranchProbability Prob(*this);
-    return Prob *= RHS;
+    Prob *= RHS;
+    return Prob;
   }
 
   BranchProbability operator/(uint32_t RHS) const {
     BranchProbability Prob(*this);
-    return Prob /= RHS;
+    Prob /= RHS;
+    return Prob;
   }
 
   bool operator==(BranchProbability RHS) const { return N == RHS.N; }
   bool operator!=(BranchProbability RHS) const { return !(*this == RHS); }
 
   bool operator<(BranchProbability RHS) const {
     assert(N != UnknownN && RHS.N != UnknownN &&
            "Unknown probability cannot participate in comparisons.");

lib/Target/AArch64/AArch64ISelLowering.cpp

       return SDValue();
 
     Cmp = DAG.getNode(ISD::OR, dl, CmpVT, Cmp, Cmp2);
   }
 
   Cmp = DAG.getSExtOrTrunc(Cmp, dl, Op.getValueType());
 
   if (ShouldInvert)
-    return Cmp = DAG.getNOT(dl, Cmp, Cmp.getValueType());
+    Cmp = DAG.getNOT(dl, Cmp, Cmp.getValueType());
 
   return Cmp;
 }
 
 static SDValue getReductionSDNode(unsigned Op, SDLoc DL, SDValue ScalarOp,
                                   SelectionDAG &DAG) {
   SDValue VecOp = ScalarOp.getOperand(0);
   auto Rdx = DAG.getNode(Op, DL, VecOp.getSimpleValueType(), VecOp);