[PATCH] D116327: [Coroutines] Enhance symmetric transfer for constant CmpInst

Thu Jan 27 18:09:47 PST 2022

ChuanqiXu added a comment.

In D116327#3275975 <https://reviews.llvm.org/D116327#3275975>, @erichkeane wrote:

> In D116327#3274414 <https://reviews.llvm.org/D116327#3274414>, @ChuanqiXu wrote:
>
>> In D116327#3273168 <https://reviews.llvm.org/D116327#3273168>, @erichkeane wrote:
>>
>>> In D116327#3247285 <https://reviews.llvm.org/D116327#3247285>, @ChuanqiXu wrote:
>>>
>>>> In D116327#3243349 <https://reviews.llvm.org/D116327#3243349>, @sidorovd wrote:
>>>>
>>>>> @ChuanqiXu hi, with this patch the generated LLVM IR snippet is looking like:
>>>>>
>>>>>   %_Z5task0v.Frame = type { void (%_Z5task0v.Frame*)*, void (%_Z5task0v.Frame*)*, %struct.TaskPromise, i1 }
>>>>>
>>>>> (see https://godbolt.org/z/b3s9a4xnW - it's the same example from the patch's headed, but with -S -emit-llvm).
>>>>>
>>>>> In the following patch you add a regression test clang/test/CodeGenCoroutines/coro-elide.cpp , which checks an opposite, which is a bit confusing
>>>>>
>>>>>   // CHECK: %_Z5task1v.Frame = type {{.*}}%_Z5task0v.Frame
>>>>>
>>>>> Could you please tell if it's a typo or it was done intentional? If intentional, I keen to know, where could I learn more about the coroutine frame jump, thanks :)
>>>>
>>>> Yeah, it is intentional. I added two regression tests in the following commit. But only one is related to this revision, while the other one you mentioned is related to another optimization for coroutine. The optimization is called Coroutine Elision. You could find the original design in: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0981r0.html. For the more detailed implementation, you might need to take a look at CoroElide pass. For the coroutine intrinsic, it is good to visit https://llvm.org/docs/Coroutines.html. It is not completed now. For example, I found that it lacks a ABI document just now...
>>>
>>> So the problem I'm seeing in our downstream (which might be behind) is that the _Z5task1v.Frame is:
>>>
>>> `%_Z5task1v.Frame = type { void (%_Z5task1v.Frame*)*, void (%_Z5task1v.Frame*)*, %"struct.Task::promise_type", i2 }`
>>>
>>> Note that this does NOT match the 'CHECK' line.
>>>
>>> HOWEVER, in upstream, I see:
>>> `%_Z5task1v.Frame = type { void (%_Z5task1v.Frame*)*, void (%_Z5task1v.Frame*)*, %"struct.Task::promise_type", %_Z5task0v.Frame, i2 }`
>>>
>>> Note the `%_Z5task0v.Frame` before the 'i2'.
>>>
>>> Do you perhaps have any hints as to where that could have gone?
>>
>> Do you talk about https://github.com/llvm/llvm-project/commit/bf5f2354fa6e3f31a1acea75a229fee54359e279#diff-0b5d68fb7c4bb0c5aad6c883444d80715cf6879e3d27e24e38c22a61c6971730?
>>
>> This is a regression test for an optimization called CoroElide, which going to eliminate the heap allocation.
>>
>> For example, without the optimization, when we call `task1`, it would call `std::new` to generate `task1v.Frame` and call `std::new` again to generate `task0v.Frame`. So here are 2 calls to allocate on the heap. But with the optimization, it would call `std::new` only once when we call `task1`. So we decrease the time to allocate on heap.
>>
>> If this tests fails, it implies the optimization fails in the case. hmmm since it fails in the downstream, I am not sure how could I help you. Any thoughts?
>>
>> BTW, if coroutine is not important to you, I think you could delete the tests in downstream. It shouldn't do something harm to the semantics. It is just an optimization. It doesn't matter about the semantics in the standard. (The idea of CoroElide comes from a language proposal. But it doesn't show up in the standard. I guess the reason may be that the standard don't care optimization like this one)
>
> Yep, thats the file.
>
> I'm just shocked to see a CFE test that depends on optimization/does optimization checks like this.  This is pretty far from typical, and of course causes confusion.  I presume one of our optimizations is working a bit differently (another reason we typically don't have lit tests like this in the CFE) from community.
>
> We LIKE to keep as many of the tests in downstream, as it helps with correctness, but I'm beginning to doubt the validity of this test; it seems it'll be quite fragile.

Yeah, before I created the file, I searched O2 <https://reviews.llvm.org/owners/package/2/>/O3 <https://reviews.llvm.org/owners/package/3/> in clang/test/CodeGen* directory and I found many results. So I feel it might not be bad to have test in clang to depend on optimization in middle end directly. The best practice maybe we check the pattern generated in the frontend and we check the transformation for the pattern in the middle end. I would like to do this but I am a little bit busying now and I am going to take a vacation next week so it might wait for a longer time.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116327