save an iteration over the loop's basic blocks (which is what getLoopLatch does)

In D51255#1217833, @sqlbyme wrote:

First off, thanks for offering to help make the website work correctly! This all looks pretty reasonable to me. I'm wondering though, would it be possible to break this patch up into a few smaller patches? Since we do not have any type of staging infrastructure for llvm.org, I would prefer we not change so many files all at once. I feel like three steps here might be appropriate:

1. Try with one file (one of the files with a one or two line change) to ensure the changes work as we expect them to.
2. Apply changes to the rest of the files with the exception of footer.incl.
3. Apply the last patch to footer.incl which adds the javascript bits in the footer (my experience tells me this is the most likely to break something, though the JS you have provided LGTM).

Use relative links where possible (thus if we stay in the same subdomain http{,s}://llvm.org/.*)

In D51255#1214424, @asl wrote:

I think we need to preserve the protocol. E.g. if the page is loaded via http we need to load css via http as well.

In D51255#1214403, @kpn wrote:

Is there a good reason to have the hostname in the URL? Why not just link to "/Users.html" instead of "https://llvm.org/Users.html", for example? Using the hostname is only needed when linking to a page on a different server.

Use call sites for now and improve testing of non-null deduction

Remove large macro in favor of a switch

Fixed spelling, stripped casts, added the tests from D4609

Ping. Are you going to upstream this?

This patch looks fine to me.

Fix spelling and improve comments

Fix capture problem and small improvements

Thanks for these initial comments. Second revision and actual RFC mail is coming.

Misused arcanist, sorry.

In D46735#1096084, @Meinersbur wrote:

Some context: Pankaj is a GSoC student on a project to add more Polly-optimizable benchmarks. Currently, only SingleSource/Benchmarks/Polybench is really optimizable, most other benchmarks contain some kind of pre-optimization that makes it difficult for Polly to preserve semantics, even if the algorithm itself is 100% optimizable.

In D41692#1052643, @Meinersbur wrote:

I ran a comparison on the test-suite and here is the result:

Program                                                                        leone_N20_polly  leone_N30_errheuristic diff
 SingleSource/Benchmarks/Misc/flops                                            4.332            6.311                    45.7% 
 S...urce/Benchmarks/Polybench/linear-algebra/solvers/gramschmidt/gramschmidt  1.443            1.637                    13.4% 
 Polybench/Polybench-421/stencils/adi/P421_adi                                21.096           19.344                    -8.3%  
 External/SPEC/CFP2017rate/507.cactuBSSN_r/507.cactuBSSN_r                    31.899           33.395                     4.7%

I am confused. It seems you want to be smarter about the error blocks (which is necessary!) but this will just make them more greedy (too greedy in my experience). Did anybody check how this affects the test-suite or why the test cases fail [, after all, this says "ready to land"] ?

@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.