This is an archive of the discontinued LLVM Phabricator instance.

[Clang] [P2025] More exhaustive tests for NRVO
ClosedPublic

Authored by Izaron on Feb 16 2022, 3:13 AM.

Details

Summary

This is a preliminary patch ahead of D119792 (I'll rebase that one on top of this).
This shows what Clang's _current_ behaviour is for calculating NRVO in various
common cases. Then, in D119792 (and future patches), I'll be able to demostrate
exactly how LLVM IR for each of these cases changes.

Diff Detail

Event Timeline

Izaron requested review of this revision.Feb 16 2022, 3:13 AM
Izaron created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2022, 3:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Excellent!

clang/test/CodeGenCXX/nrvo.cpp
622

Similar to what you did in test5, I think it'd be helpful to comment on the return line itself whether NRVO is (done|possible|impossible). E.g.
return x; // NRVO is impossible
or
return x; // FIXME: NRVO could happen, but doesn't
or
return x; // NRVO happens
Your existing comment "No NRVO" is ambiguous — you mean it doesn't happen, or it can't possibly happen? — and makes me think just a little more than I ought to, because I have to skim the code and figure out which variable(s) and which return statement(s) we're talking about.

(I can see it being arguable whether the comment should go on the return x; or on the X x;. Me personally, I'd put it on the return. But I don't care enough to argue if someone thinks it should go on the variable definition instead.)

1604

E.g. here, IIUC, I would comment this as

return x; // NRVO happens
Izaron updated this revision to Diff 409374.Feb 16 2022, 12:47 PM

Thanks! Yes I should've write comments that are understandable not only for me =)
I added comments to the existing tests as well.
Though NRVO attribute is bound to the variable, I'm also more comfortable to place comments in the "return" lines, as it is looks somewhat more clear.

Izaron marked an inline comment as done.Feb 16 2022, 12:48 PM
Izaron marked an inline comment as done.
Quuxplusone accepted this revision.Feb 16 2022, 1:05 PM

LGTM FWIW. You might want to wait for someone more authoritative to take a look; but it's also only adding test coverage, so it seems pretty uncontroversial, I would think.

clang/test/CodeGenCXX/nrvo.cpp
168

Technically, because B is a constant throughout this function, we probably could do NRVO here by hoisting the condition as if by

X x;
if (B) { X y; return y; } // NRVO is possible here
X y; return x; // NRVO is impossible

Whether we should is another question. :) Also, changing bool B to const bool& B would defeat my idea.

1152

It would be helpful to comment these tests also with their p2025 example numbers, e.g.

void test16() { // http://wg21.link/p2025r2#ex-9

It took me a while to realize that the numbers here don't actually match up to the examples' numbers in the paper.

1540

Nit: s/if/when/

This revision is now accepted and ready to land.Feb 16 2022, 1:05 PM
Izaron updated this revision to Diff 409389.Feb 16 2022, 1:30 PM

I placed links to corresponding p2025 examples.

Some of the examples are reasonably absent from the test, such as 1st (RVO, not NRVO), 13th (consteval methods are not getting to LLVM IR), 17th (there are no NRVOs for non-class types in Clang), etc.

The 19th example (about volatiles) is massive and has three corresponding functions in the test.

Izaron marked 2 inline comments as done.Feb 16 2022, 1:31 PM
Izaron marked an inline comment as done.Feb 16 2022, 1:39 PM
Izaron added inline comments.
clang/test/CodeGenCXX/nrvo.cpp
168

I have been thinking on "sorting" variables within the scope to get the optimal NRVO condition (right after seeing test2), but didn't come up with anything =(

I guess that it's not legal at all, because initializing constructors (unlike copy/move constructors) tend to have meaningful actions (starting a timer, etc.), and many things will break badly if we will shift the declarations as we want.

Izaron marked an inline comment as done.Feb 16 2022, 1:47 PM

@Quuxplusone Thanks for reviewing the patch! We can wait some time if someone else wants to take a look. Though I doubt if there may be major complaints on extendind the tests (especially with comments and references to a proposal).

Let me copy-paste here a standard disclaimer =) Should've done it right away, but I forgot about it.

P.S. If this review is eventually approved, kindly please merge the commit on my behalf =) As I don't have merge access. My name is Evgeny Shulgin and email is izaronplatz@gmail.com. Sorry for inconvenience!

clang/test/CodeGenCXX/nrvo.cpp
1540

Serendipitously, I just ran into almost this exact scenario in D120180, where C++20's reverse_iterator wants to do basically

{
  _Iter __tmp = current;
  --__tmp;
  if constexpr (is_pointer_v<_Iter>) {
    return __tmp;
  } else {
    return std::move(__tmp).operator->();
  }
}

and so we want NRVO on __tmp in the former case but URVO in the latter case. Of course in that specific case, we "want NRVO" only when __tmp is a pointer and thus NRVO doesn't apply anyway because pointers are returned in registers. But it would be nice to have a test case for as-close-as-possible to that pattern, if you don't mind adding one.

Izaron updated this revision to Diff 415904.Mar 16 2022, 10:45 AM

Add the reverse_iterator-like pattern to tests

Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2022, 10:45 AM

@Quuxplusone, thank you for the test idea! I added test25 that resembles the pattern.

Could you please look if we're okay with test25?

If everything is OK, I could merge the patch myself: I've got repository merge access =)

P. S. Sorry for the long delay, I was really busy lately.

Izaron marked an inline comment as done.Mar 16 2022, 11:44 AM
This revision was automatically updated to reflect the committed changes.