@jdoerfert @grosser
Could we please add as an XFAIL for now, so that it doesn't keep failing the polly buildbots?

My local Polly does not pass this test anyway, but I guess Tobias can help you out or maybe @Meinersbur or @bollu. In any case it doesn't seem to be a "loop bounds test" so it should be possible to change it a little bit and keep it around.

In D35471#883365, @grosser wrote:

I never said your approach is wrong or has an entirely wrong design, but said that a similar approach caused issues in graphite,

In D38255#880989, @grosser wrote:

Hi Johannes,

thanks for posting the patch. Obviously it misses comments, and unittests for the C++ interface, but it already gives a very good idea where you are heading. I also feel this patch covers a couple of areas:

1. A new C++ interface which seems to serve multiple purposes
   • Automatic memory management
   • Add some additional math functionality not available in isl at the moment
   • Rename a couple of functions to provide a better interface to LLVM
   • Add a couple of helpers / convenience features on top of isl (e.g. automatic space adjustment)
   • Add some functionality that is directly needed for the domain/range modeling, but not necessarily belongs to the core properties of an isl set.
     
     All of these ideas make a lot of sense. I am however slightly worried regarding the large amount of hand-written interfacing code -- which is often replicated across different classes. How much of isl are you planning to cover? Will you also introduce callbacks and stuff? I wonder if there would be a way to avoid a lot of this duplication and possibly make the separation between the parts that are just isl interfaces and the parts that are indeed LLVM additions more clear. We also have a larger set of isl extensions in Polly -- including corresponding unit tests. I think sharing these would indeed make a lot of sense.
2. The polyhedral value analysis
   
   The general interface makes a lot of sense - I played with this myself (after having introduced islpp), but waited for your proposal to come out. It is a clear extension of the LLVM-IR modeling we discussed together in Polly and you nicely mixed it with the ScalarEvolution design of LLVM. Cool! The code is currently a little too large to comment on specific items. Some of the things you do are already things we did in Polly but unfortunately missing most comments, others are clearly new and I need to read through them in detail. As I said on the mailing list thread, I feel that this analysis generally makes a lot of sense -- assuming we agree -- I would like to help getting it into LLVM. If you would like to do this incrementally -- the RISC-V model would make a lot of sense here. Do you believe there is a way to split this into smaller pieces? Maybe this can be split into:
3. Basic Interface
4. Analysis of scalar values without control flow
5. Simple loops
6. Wrapping
   
   I am especially interested in understanding (and having documented well) how integer wrapping is modeled.
7. Specifically, I would like this analysis to speculate the wrapping introduced by casts as they commonly arise e.g. in OpenCL code get_global_id / get_local_id. Not sure if this is currently possibly / optional.
8. Also, did you consider multi-dimensional output spaces. I discussed with Siddharth and we might be able to model certain polynomial expressions this way. Most interestingly, we might be able to do delinearization on this model, which would allow us to effectively delinearize even in the presence of integer casts.
9. Can you make wrapping optional -- meaning can you potentially model precise wrapping?
   
   Also, do you happen to have a design description why all this works? If not, I wonder if we should create this together when building up the code incrementally in LLVM.
   
   I also have a random set of comments, but these are clearly by far not complete but just passby comments.

I forgot to include a softlink of the isl include folder. In the llvm src directory the following should be sufficent:

Just out of curiosity, did you compare the results (compile time, runtime, number of required overflow tracking intrinsics,...) to the simple bit-width tracking that was implemented in the IslExpr builder?

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.