snarkmaster wrote: @ChuanqiXu9, please take a look at the update. I incorporated all the feedback from you and @yuxuanchen1997, as collected in my prior comment / checklist.
I additionally made a change that Yuxuan mentioned, and that I thought I wouldn't be able to make. Specifically, I went back to a member function attribute, from a class attribute. The big upside of this is that every overload of `await_suspend` can independently decide whether it uses `await_suspend_destroy()`. I am actually going to end up using this in a near-term revision of `folly::result`, since I am migrating to a `co_await or_unwind()` syntax for extracting the value both in short-circuiting **and** in suspending coros. This commit has the detailed changes: https://github.com/llvm/llvm-project/pull/152623/commits/f1e885c45837b0e1bd3925fb69e16c6189ebbb80 -- but, things look different enough it's probably worth a fresh read-through. --- Regarding the "no intrinsics" attribute discussion -- > I feel confused about [[clang::coro_may_inline_suspend_with_barrier]]. It > makes things complex. In my mind, we only need: > - A class attribute to hint the compiler to not emit await.suspend > intrinsics. > - As you said, the __builtin_suspend_barrier is helpful for users to give > finer grained control. More likely, I am confused -- you know the code much better. The reason I proposed this very specific attribute is that I was worried about end users. Without the compiler enforcing barrier usage, people might apply the attribute to a production coro task, without understanding the subtle miscompilations you might see by dropping the suspend intrinsics. This sort of thing can show up in production as a random bug (heisenbug), which can make it extremely expensive to track down. So, my goal in coupling "remove intrinsics" with "force a reordering barrier" was to give the end users some margin of safety. I think the high-level goal of safety is a good one. On the other hand, I don't feel like I understand the internals of LLVM well enough to have a good opinion on the implementability of that specific "safer" idea, or whether there's a different way to make it safer. It might also be fine to provide them as separate features, but then the docs for the attribute must strongly encourage the users to use the barrier. https://github.com/llvm/llvm-project/pull/152623 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
