This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Quuxplusone on Nov 25 2018, 9:07 PM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

dblaikie added inline comments.
include/llvm/Support/BranchProbability.h
130–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 ↗(On Diff #175193)

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 marked 2 inline comments as done.Nov 26 2018, 2:28 PM
Quuxplusone added inline comments.
include/llvm/Support/BranchProbability.h
130–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 ↗(On Diff #175193)

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

dblaikie added inline comments.Nov 26 2018, 3:04 PM
include/llvm/Support/BranchProbability.h
130–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 ↗(On Diff #175193)

Sure! Done in r347609

Quuxplusone marked 2 inline comments as done.Nov 26 2018, 4:38 PM
Quuxplusone added inline comments.
include/llvm/Support/BranchProbability.h
130–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 ↗(On Diff #175193)

Thanks!

dblaikie added inline comments.Nov 26 2018, 5:11 PM
include/llvm/Support/BranchProbability.h
130–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 added a subscriber: steveire.

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.

Quuxplusone edited the summary of this revision. (Show Details)Nov 27 2018, 9:39 AM
dblaikie accepted this revision.Nov 27 2018, 4:06 PM

Sounds alright to me - thanks!

This revision is now accepted and ready to land.Nov 27 2018, 4:06 PM
Quuxplusone edited the summary of this revision. (Show Details)

@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?

Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2019, 11:06 AM
This revision was automatically updated to reflect the committed changes.