This is an archive of the discontinued LLVM Phabricator instance.

[Coroutines] Remove unused coroutine builtin/intrinsics llvm.coro.param (NFC-ish)
ClosedPublic

Authored by ChuanqiXu on Dec 6 2021, 10:52 PM.

Details

Summary

I found that the coroutine intrinsic llvm.coro.param in documentation (https://llvm.org/docs/Coroutines.html#id101) didn't get used actually since there isn't lowering codes in LLVM. I also checked the implementation of libstdc++ and libc++. Both of them didn't use llvm.coro.param. So I am pretty sure that the llvm.coro.param intrinsic is unused. I think it would be better t to remove it to avoid possible misleading understandings.

Diff Detail

Event Timeline

ChuanqiXu created this revision.Dec 6 2021, 10:52 PM
ChuanqiXu requested review of this revision.Dec 6 2021, 10:52 PM

I imagine Gor hoped for this optimization to be implemented someday, assuming it's still allowed by the language specification. Maybe you would be interested in pursuing that? It sounds like it's really just (1) emitting the intrinsic in the frontend and then (2) checking if the copied parameter variable is actually used after reaching a suspend point, other than in ways that wouldn't happen if the intrinsic returned false.

I imagine Gor hoped for this optimization to be implemented someday, assuming it's still allowed by the language specification. Maybe you would be interested in pursuing that? It sounds like it's really just (1) emitting the intrinsic in the frontend and then (2) checking if the copied parameter variable is actually used after reaching a suspend point, other than in ways that wouldn't happen if the intrinsic returned false.

To my knowledge of language specification, the optimization is not allowed now. Both the construction and destruction is required now. But I am indeed interested to implement this. Since I know that many implementation of C++ features comes before they become standard. So I am wondering if this could be a proposal demo to the language specification : )
BTW, I am still prefer to remove the intrinsic to avoid misunderstanding. Or is It sufficient to add a TODO/FIXME marker in the Documentation? How do you think about it?

Like a lot of the switched-resume lowering, this intrinsic is extremely tied to C++ semantics. If C++ doesn't actually allow the optimization anymore, then I completely agree that we should go ahead and remove the intrinsic. If it's allowed and we just haven't implemented it yet, then okay, it might be best to remove it, but an unused intrinsic that we're planning to start using soon-ish isn't really doing any harm.

Like a lot of the switched-resume lowering, this intrinsic is extremely tied to C++ semantics. If C++ doesn't actually allow the optimization anymore, then I completely agree that we should go ahead and remove the intrinsic. If it's allowed and we just haven't implemented it yet, then okay, it might be best to remove it, but an unused intrinsic that we're planning to start using soon-ish isn't really doing any harm.

Understood. I think it is not allowed and I would consult with experts in EWG to make sure.

Like a lot of the switched-resume lowering, this intrinsic is extremely tied to C++ semantics. If C++ doesn't actually allow the optimization anymore, then I completely agree that we should go ahead and remove the intrinsic. If it's allowed and we just haven't implemented it yet, then okay, it might be best to remove it, but an unused intrinsic that we're planning to start using soon-ish isn't really doing any harm.

From the feedback from Lewis, this optimization is allowed. Here is the link: http://eel.is/c++draft/class.copy.elision#1.3. I would add it to my TOOD list. But the priority might not be high since I am working on C++20's module recently. So it might be the case that the optimization would start soon. So I think it might be better to remove it.

rjmccall accepted this revision.Dec 8 2021, 11:36 AM

Okay, that's fine, I have no objection to removing it. LGTM.

This revision is now accepted and ready to land.Dec 8 2021, 11:36 AM