Page MenuHomePhabricator

[coroutines] Fix application of NRVO to Coroutine "Gro" or return object.
ClosedPublic

Authored by EricWF on Jan 20 2018, 7:19 PM.

Details

Summary

Fix NRVO for Gro variable.

Previously, we only marked the GRO declaration as an NRVO variable
when its QualType and the function return's QualType matched exactly
(using operator==). However, this was incorrect for two reasons:

  1. We were marking non-class types, such as ints, as being NRVO variables.
  1. We failed to handle cases where the canonical types were the same, but the actual QualType objects were different. For example, if one was represented by a typedef. (Example: https://godbolt.org/g/3UFgsL)

This patch fixes these bugs by marking the Gro variable as supporting NRVO only
when BuildReturnStmt marks the Gro variable as a coroutine candidate.

Diff Detail

Event Timeline

EricWF created this revision.Jan 20 2018, 7:19 PM
majnemer added inline comments.
test/CodeGenCoroutines/coro-gro-nrvo.cpp
18

SizeT -> __SIZE_TYPE__ ?

EricWF marked an inline comment as done.Jan 21 2018, 2:06 AM
EricWF added inline comments.
test/CodeGenCoroutines/coro-gro-nrvo.cpp
18

SGTM.

EricWF updated this revision to Diff 130801.Jan 21 2018, 2:06 AM
EricWF marked an inline comment as done.
  • Address inline comments in test.
EricWF edited the summary of this revision. (Show Details)Jan 21 2018, 2:35 AM
EricWF edited the summary of this revision. (Show Details)
EricWF updated this revision to Diff 131334.Jan 24 2018, 1:17 PM
EricWF edited the summary of this revision. (Show Details)
  • Use a better formulation for detecting when the Gro should be an NRVO variable.
This revision is now accepted and ready to land.Jan 29 2018, 9:19 AM
This revision was automatically updated to reflect the committed changes.
EricWF updated this revision to Diff 132495.Feb 1 2018, 3:46 PM

I had to revert due to failing tests when using a non-assert Clang build.

This change fixes those tests to no longer depend on label names.