[PATCH] D109800: [clang] don't mark as Elidable CXXConstruct expressions used in NRVO

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov added a comment.

In D109800#3004288 <https://reviews.llvm.org/D109800#3004288>, @Quuxplusone wrote:

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109800