In D31582#716278, @dberlin wrote:

It's early in the morning, but I'm having a little trouble understanding the overall goal. Is this fixing a bug, it just being done because it's a waste of time to add the metadata in this situation.

Utilize the ParallelRegionInfo pass and emit parallel loop metadata

Still trying to separate a single commit

Try to make arcanist upload only one commit

Add more functionality

In D30376#692486, @GorNishanov wrote:

I agree that verifier could be more rigorous with checking that coroutine intrinsics are used properly, but, I would not want to change the tests ex0 - ex5.ll as you suggested.
ex?.ll tests are examples from the coroutine walkthrough in LLVM Coroutines doc and I would like to keep them in sync with the tutorial. They represent minimally useful, but still sensible coroutines.

I would not object of adding new tests along the lines that your suggested (possibly with verifier changes to give better diagnostics) where assumptions about coroutine structure are violated.

And I would like to add that I appreciate very much your interest in LLVM coroutines and proposing this patch.

In D29250#659722, @majnemer wrote:

Is there another patch which updates the langref? The semantics are most important of all.

Please see https://reviews.llvm.org/D30353 for a draft documentation and semantics.

Update to match new D29250

Fix smaller issues and minor renaming

In D29250#659722, @majnemer wrote:

Is there another patch which updates the langref? The semantics are most important of all.

In D29250#659678, @deadalnix wrote:

I'm not sure who's the proper person to review that, but that wouldn't be me. Did you discussed adding these instruction with someone ? If so, who is that ? maybe you want to add these people as reviewer.

Thanks for the tip!

LGTM.

LGTM.

You also have to adjust the call location to

buildMemIntrinsicAccessRelation

but I think this is the best way to go.

In D25706#573332, @efriedma wrote:

We ran into a case similar to the included testcase, where we need to generate a runtime alias check with a memset in order to vectorize a loop. (memset isn't really non-affine, but we currently model it that way.)

While I agree with the commit message, I am not aware of any real benefit this will provide us given that we only leverage the constant range ScalarEvolution range analysis. If we would use a better one, e.g., along the lines of https://reviews.llvm.org/D20230, I think this patch does make sense.

I realized that we should check hoisting is actually performed.

Assuming all checks prior to code generation can handle this case properly this is the "right way" to fix this. One modification and it should be good to go.

Looks almost good. I am not sure about the i1 thing and this one test case though. Does lnt pass?

Hi Eli,

I won't.

my 2 cents.

In general this LGTM. Maybe look at my last comments and once you finalize this patch ask somebody to push it for you.

Except the single comment LGTM.

LGTM.

Does this pass LNT when it is part of the Polly pipeline [profitable but also unprofitable] or are there any crashes? If not I would go ahead and commit it for you.

Some minor remarks but after they are fixed it looks good to go in.

LGTM, thanks for the patch.

More comments

Please look at the comments and add some test cases as you did with the scopinfo function pass.