[PATCH] D142621: [Couroutines] Modify CoroFrame materializable into a callback

Thu Feb 9 17:58:09 PST 2023

ChuanqiXu added a comment.

In D142621#4115723 <https://reviews.llvm.org/D142621#4115723>, @dstuttard wrote:

> In D142621#4115184 <https://reviews.llvm.org/D142621#4115184>, @dstuttard wrote:
>
>> In D142621#4114357 <https://reviews.llvm.org/D142621#4114357>, @ChuanqiXu wrote:
>>
>>> While this is innocent and no harmful, I prefer to land this after we found a usage for this someday. It wouldn't be a problem to do such a NFC change any times.
>>
>> We have a use-case for this change for one of our backends.
>
> To expand on this - we have a tool based on LLVM that already adds extra materialization options. This change was to enable generic support to extend materialization determination so that we can simplify our code  to not have patches on top of upstream llvm.
> I can imagine other groups having similar requirements - none of which would necessarily end up back in core llvm.

Got it. But it is still not good to modify the upstream due the requirement from the downstream. (I know this happens in llvm occasionally. But it is still not good, right?)

I think it may better for you to contribute the other materialization options you mentioned. Or you can try to add a unittest if you don't want to contribute the downstream extension to public for any reason . Otherwise, even all people here get in consensus. It is possible that other developers found the coding here is redundant and optimize it with a NFC patch directly. (This happens a lot in llvm)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142621