Page MenuHomePhabricator

[clang] don't mark as Elidable CXXConstruct expressions used in NRVO
ClosedPublic

Authored by mizvekov on Sep 14 2021, 6:20 PM.

Details

Summary

See PR51862.

The consumers of the Elidable flag in CXXConstructExpr assume that
an elidable construction just goes through a single copy/move construction,
so that the source object is immediately passed as an argument and is the same
type as the parameter itself.

With the implementation of P2266 and after some adjustments to the
implementation of P1825, we started (correctly, as per standard)
allowing more cases where the copy initialization goes through
user defined conversions.

With this patch we stop using this flag in NRVO contexts, to preserve code
that relies on that assumption.
This causes no known functional changes, we just stop firing some asserts
in a cople of included test cases.

Diff Detail

Event Timeline

mizvekov created this revision.Sep 14 2021, 6:20 PM
mizvekov updated this revision to Diff 372828.Sep 15 2021, 4:06 PM
mizvekov retitled this revision from test commit. to [clang] don't mark as Elidable CXXConstruct expressions used in NRVO.
mizvekov edited the summary of this revision. (Show Details)

.

mizvekov published this revision for review.Sep 15 2021, 4:33 PM
mizvekov added reviewers: rsmith, rjmccall.
mizvekov added a subscriber: Quuxplusone.
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2021, 4:33 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

There might be some tooling out there that cares about CXXConstructExprs that might be elided due to NRVO being marked as elidable. But it's probably OK to lose that marking if nothing in-tree is relying on it, and use isElidable only for copy elision from rvalues and not for NRVO.

I would have expected that we'd also assert for a case like this (in C++14 and earlier):

struct b {};
struct a : b {
  a();
  a(a &);
  a(const b &);
};
a c() {
  if (0) { return a(); }

  return a();
}

... and that this patch wouldn't help there. But it looks like we don't mark the a(const b&) constructor call here as elidable, which seems like a bug, but might not be one worth fixing?

I can have a look on what is happening with that test case. At best if we figure out what is wrong, we could add some FIXMEs and circle back to it after we make this whole thing work again.

mizvekov updated this revision to Diff 372845.Sep 15 2021, 6:28 PM

Add a couple more test cases where we don't perform copy-elision
on a returned prvalue in pre-C++17 modes, and the corresponding
FIXMEs in the source code.

mizvekov updated this revision to Diff 372852.Sep 15 2021, 6:57 PM

add another FIXME.

dim added a subscriber: dim.Sep 16 2021, 10:09 AM

I can at least confirm that both the original test case for bug 51862 (from https://github.com/Macaulay2/frobby ) and the reduced test case compile successfully with this patch added.

FWIW, this is almost entirely above my pay grade. Getting rid of that extra bool parameter throughout looks awesome, though. :)

I do think it might be a good idea to include one or two tests that actually run all the way through codegen, e.g. https://godbolt.org/z/54eTrj54M
The scary failure mode here (if you are like me and don't really understand why this is impossible ;)) would be if the compiler somehow elided one of the implicit conversions without eliding the other one (like how compiler bugs have in the past led to eliding a constructor without eliding the matching destructor).

Also, is it worth testing anything in constexpr functions? My understanding is that constexpr functions never do copy elision, although maybe(?) they're permitted to?

Getting rid of that extra bool parameter throughout looks awesome, though. :)

It will come back later, this patch is just a quick thing we need in order to ship clang-13.
But perhaps we bring it back in enum class form? :)

I do think it might be a good idea to include one or two tests that actually run all the way through codegen, e.g. https://godbolt.org/z/54eTrj54M

I think in that case, if we pursue that direction, what we would want is to have these tests that only go until LLVM IR, and then another set of tests that go from IR to arch specific LL, or perhaps just to optimized IR.
To be frank, I don't see how anything interesting here would happen after clang CodeGen (except in UB cases), but I am not very familiar with that side of the project.

The scary failure mode here (if you are like me and don't really understand why this is impossible ;)) would be if the compiler somehow elided one of the implicit conversions without eliding the other one (like how compiler bugs have in the past led to eliding a constructor without eliding the matching destructor).

Right, when we get to that point where we can elide that double conversion sequence, we got to add tests, but I suspect they don't need to go further than clang CodeGen.

Also, is it worth testing anything in constexpr functions? My understanding is that constexpr functions never do copy elision, although maybe(?) they're permitted to?

We do have analogous situation to CodeGen there, we step over a MaterializeTemporary in the isElidable case, and if we improve in this area to support double conversions, that has to be fixed too.
But this patch should not be changing anything in the rvalue path, only the NRVO one, which again I could not find anything that uses it anywhere in LLVM tree.
I will add an assert and FIXME there though.

mizvekov updated this revision to Diff 373323.Sep 17 2021, 1:21 PM
mizvekov edited the summary of this revision. (Show Details)

add a couple more FIXMEs and a couple of asserts in the constexpr path for rvalue copy elision.

dim added inline comments.Sep 19 2021, 10:22 AM
clang/lib/AST/ExprConstant.cpp
9937 ↗(On Diff #373323)

s/simples/simplest/

clang/lib/CodeGen/CGExprCXX.cpp
618–619

s/simples/simplest/

rsmith accepted this revision.Sep 21 2021, 12:10 PM
This revision is now accepted and ready to land.Sep 21 2021, 12:10 PM
This revision was landed with ongoing or failed builds.Sep 21 2021, 12:41 PM
This revision was automatically updated to reflect the committed changes.