Update the doc. @efriedma is this ok?

sorry for the late reply.

In D87817#2280618, @efriedma wrote:

If there are transformations that are specifically illegal with corouties, I'd like to see a section in https://llvm.org/docs/Coroutines.html describing what transforms are and are not legal. Just saying that the current version of the coroutine lowering code can't handle it isn't sufficient.

LGTM, Thanks for the fix.

In D87817#2278717, @junparser wrote:

We try to find an general solution to handle some conservative & mismatch analysis in transformations caused by the change of cfg with coro.suspend intrinsic. However afaik, it seems quiet difficult to deal this without change framework fo coroutine.
This patch just fixes the known issue, it may happens in other passes, FYI.

We try to find an general solution to handle some conservative & mismatch analysis in transformations caused by the change of cfg with coro.suspend intrinsic. However afaik, it seems quiet difficult to deal this without change framework fo coroutine.
This patch just fixes the known issue, it may happens in other passes, FYI.

what does this intrinsic use for?

@lxfind , could you backport this to branch 11?

LGTM， thanks！

Thanks for the change. LGTM, and testcase？

In D86190#2227340, @efriedma wrote:

Are you sure this is the right fix? At first glance, ensuring %p lives long enough seems like the sort of thing coroutine lowering should be able to deal with. If it isn't, at the very least we need documentation describing what optimizations are legal.

In D85812#2220117, @wenlei wrote:

Looking at sample profile loader inline and your test case (either always inline or with sample profile), I'm guessing your change in the general llvm::getInlineCost was for sample profile loader inline, not CGSCC inline? If that's the case, it might be better to find a way to do this targeting just always inline and sample profile loader inline, then assert this shouldn't happen for CGSCC inline (would be a pass ordering bug).

LGTM，Thank you, Please merge the testcases into one file.

LGTM, Thank you!

LGTM

In D82314#2125713, @lxfind wrote:

In D82314#2124662, @junparser wrote:

In D82314#2124661, @junparser wrote:

@lxfind This patch causes some mismatch when variable is used in both resume and destroy function. Besides, we should move this patch and the check in buildCoroutineFrame.

@lxfind, Would you try to fix this？ If you do not have time, then I'll try do this. Thanks

Could you please help take a look, if you have a local repro? Thanks!

of course.

In D82314#2124661, @junparser wrote:

@lxfind This patch causes some mismatch when variable is used in both resume and destroy function. Besides, we should move this patch and the check in buildCoroutineFrame.

@lxfind This patch causes some mismatch when variable is used in both resume and destroy function. Besides, we should move this patch and the check in buildCoroutineFrame.

LGTM， Thank you！

This generally looks good to me. I'll accept this if others do not have different ideas about this. Thanks!

@lxfind, Thank you! And could you please add some testcases?

In D82314#2109437, @lxfind wrote:

In D82314#2107910, @junparser wrote:

Rather than doing it here, can we build await_resume call expression with MaterializedTemporaryExpr when expand the coawait expression. That's how gcc does.

There doesn't appear to be a way to do that in Clang. It goes from the AST to IR directly, and there needs to be a MaterializedTemporaryExpr to wrap the result of co_await. Could you elaborate on how this might be done in Clang?

In D82314#2109893, @rsmith wrote:

In D82314#2109728, @lxfind wrote:

@rsmith Thanks. That's a good point. Do you know if there already exists optimization passes in LLVM that attempts to shrink the range of lifetime intrinsics? If so, I am curious why that does not help in this case. Or is it generally unsafe to move the lifetime intrinsics, and we could only do it here with specific context knowledge about coroutines.

I don't know for sure, but I would expect someone to have implemented such a pass already. Moving a lifetime start intrinsic later, past instructions that can't possibly reference the object in question, seems like it should always be safe and (presumably) should always be a good thing to do, and similarly for moving lifetime end markers earlier. It could be that such a pass exists but it is run too late in the pass pipeline, so the coroutine split pass doesn't get to take advantage of it.

Rather than doing it here, can we build await_resume call expression with MaterializedTemporaryExpr when expand the coawait expression. That's how gcc does.

In D82029#2100675, @modocache wrote:

Excellent, thank you! The test failures on the diff appear to be legitimate, they reproduce for me when I apply this patch to my local checkout and run ninja check-clang. Could you take a look?

This LGTM otherwise! Also adding @junparser in case they have any thoughts, if not I'll accept after the test failures are addressed, Thanks!

@aaron.ballman thanks for the review. I have updated the patch.

address the comments.

@rnk @aaron.ballman any comments?

@nikic @fhahn any comments on this?

kindly ping~

In D81544#2084732, @fhahn wrote:

In D81544#2084620, @junparser wrote:

since we still need discuss about the invoke of getValueInBlock, I'll split this patch into two parts.

I think this case could also be handled naturally by IPSCCP using PredicateInfo. Currently does only use range info from conditional branches, but I don't think there's any reason for not also using info from assume predicates, other than me not having time to implement it yet. If you want to give it a try, it would need to be added here: https://github.com/llvm/llvm-project/blob/master/llvm/lib/Transforms/Scalar/SCCP.cpp#L1243

since we still need discuss about the invoke of getValueInBlock, I'll split this patch into two parts.

address the comment.

address the comment.
hi @erichkeane, most of the function in vector-1.cpp are copied from ext-vector.c with vector type changed to gcc vector type, they should emit same ir. I add test7 and test8 which test logic operation of gcc vector type.

This patch implement the logic not operation of vector type. it keeps same behavior as gcc does (only allows in C++). I'll update the wrong testcases

@erichkeane @aaron.ballman , kindly ping :)

LGTM

In D79078#2040191, @spatel wrote:

I added slightly modified versions of the tests here with:
rG6211830fbabd
rG43017ceb7841

Because they are affected by a change that I split off from D79799:
rG81e9ede3a2db

Please rebase (although given what we've discussed here, I'm not sure how we want to solve the general problem of matching/transforming vector reductions).

In D79078#2038950, @lebedev.ri wrote:

I'll defer to @spatel, although i semi-weakly insist that adding such cut-offs is weird.
OTOH if this pass is taught to form such reductions we would have caught this regression for free,
because after D79799 we would have ended up in an endless combine loop here.

In D79078#2010024, @spatel wrote:

I agree that we need a phase ordering test - see llvm/test/Transforms/PhaseOrdering/X86/addsub.ll as an example test file that describes some expected handshake between vector-combine and other passes. Also, please commit the new test with complete baseline CHECK lines (use utils/update_test_checks.py), and then update the file to show the diffs in this review. Let me know if that's not clear.

Almost there - it should be next to the example test, not where it is now.

In D79078#2010024, @spatel wrote:

I also sympathize with trying to solve this here rather than SLP. One of the reasons vector-combine exists is because SLP became too hard to reason about. In hindsight, we should have created a separate pass for reductions - those are not traditional SLP concerns. Just my opinion. :)

I'm not sure what you have in mind here?
That *this* pass should also form such reductions?
Or that we should not disturb them after SLP formed them?
Or something else?

In D79078#2031347, @junparser wrote:

hi @lebedev.ri , sorry for the late response.
I have checked the reduction tree build in SLPVectorizer, for now it only support same operation. It seems we can revert the transformation when match reduction operation in matchAssociativeReduction. However, I don't think it is a good idea.

I'm having trouble parsing this response.
Are you saying that SLP can not be taught about this,
that it should not be taught about this or ???.

hi @lebedev.ri , @spatel , kindly ping.

hi @lebedev.ri , sorry for the late response.
I have checked the reduction tree build in SLPVectorizer, for now it only support same operation. It seems we can revert the transformation when match reduction operation in matchAssociativeReduction. However, I don't think it is a good idea.
Also the extra phase-ordering test shows that this patch has minimal impact. Except the first case which is vectorized by SLP, the other cases keep the same IR as before.

update the testcase.

In D79078#2009922, @lebedev.ri wrote:

In D79078#2009883, @junparser wrote:

In D79078#2009814, @lebedev.ri wrote:

This needs a phase-ordering test.

For now vector-combine is executed before slp in both legacy pm and new pm with O2, so either we handle it here or slp can handle this kind of pattern.

I'm not really sure what you mean.
We clearly have phase-ordering issue, and we should have a test that shows it.

Thanks, now i know what you mean.

Why shouldn't SLPVectorizer be taught about that pattern instead?

I think we can handle this in SLP, however it may much hard than doing here? I'm not sure.

It would be best to enhance SLP rather than adding potentially-numerous semi-arbitrarily bailout elsewhere, i think.

make sense, will check this later.

In D79078#2009814, @lebedev.ri wrote:

This needs a phase-ordering test.

For now vector-combine is executed before slp in both legacy pm and new pm with O2, so either we handle it here or slp can handle this kind of pattern.

I'm not sure whether I can approve this. If not, @modocache please ignore this.

minor update