[LoopInterchange] Enable it by default #124911
Conversation
This is a work in progress patch to enable loop-interchange by default and is a continuation of the RFC: https://discourse.llvm.org/t/enabling-loop-interchange/82589 Basically, we promised to fix any compile-time and correctness issues in the different components involved here (loop-interchange and dependence analaysis.) before discussing enabling interchange by default. We think are close to complete this; I would like to explain where we are and wanted to check if there are any thoughts or concerns. A quick overview of the correctness and compile-time improvements that we have made include: Correctness: - [LoopInterchange] Remove 'S' Scalar Dependencies (llvm#119345) - [LoopInterchange] Fix overflow in cost calculation (llvm#111807) - [LoopInterchange] Handle LE and GE correctly (PR llvm#124901) @kasuga-fj - [DA] disambiguate evolution of base addresses (llvm#116628) Compile-times: - [LoopInterchange] Constrain number of load/stores in a loop (llvm#118973) - [LoopInterchange] Bail out early if minimum loop nest is not met (llvm#115128) - [LoopInterchange] Hoist isComputableLoopNest() in the control flow (llvm#124247) And in terms of remaining work, we think we are very close to fixing these depenence analysis issues: - [DA] do not handle array accesses of different offsets (llvm#123436) - [DA] Dependence analysis does not handle array accesses of different sizes (llvm#116630) - [DA] use NSW arithmetic llvm#116632 The compile-time increase with a geomean increase of 0.19% looks good (after committing llvm#124247), I think: stage1-O3: Benchmark kimwitu++ +0.10% sqlite3 +0.14% consumer-typeset +0.07% Bullet +0.06% tramp3d-v4 +0.21% mafft +0.39% ClamAVi +0.06% lencod +0.61% SPASS +0.17% 7zip +0.08% geomean +0.19% See also: http://llvm-compile-time-tracker.com/compare.php?from=19a7fe03b4f58c4f73ea91d5e63bc4c6e61f987b&to=b24f1367d68ee675ea93ecda4939208c6b68ae4b&stat=instructions%3Au We might want to look into lencod to see if we can improve more, but not sure it is strictly necessary.
sjoerdmeijer
requested review from
nikic,
sebpop,
Meinersbur,
kiranchandramohan,
madhur13490,
CongzheUalberta and
kasuga-fj
|
Could you provide numbers for performance changes (improvments and regressions), e.g. highlights from llvm-test-suite? Would help illustrating that we also gain something from slighlty increased compile-time. |
Hi @Meinersbur, thanks for the comments, that's a very fair question! I would like to use two metrics for success here: the number of times loop-interchange triggers in the test-suite, and performance changes, with a higher weight for the former, initially. Let me explain this. We see this as a first enablement step: i.e. we have prioritised correctness and made interchange more conservative, in order to get a first version running by default. Enablement of interchange, loopcache analysis, and data dependence analysis would a big step forward. Even before we made interchange more conservative, interchange was not triggering on our motivating examples. So after this first step, we will follow up with a second optimisation step and lift restrictions to let it trigger on our examples. The number of times interchange triggers is a good metric for the potential of the pass and how general it is. I have counted 37 occurrences of interchange if external benchmark ffmpeg is included and 22 times without. So that is really good, I think. I will paste the build log with optimisation remarks at the end of this message for more details. About perf changes: for me a good result is if this shows no performance regressions, and maybe a few minor uplifts here and there. And the reason is what I wrote above, we very much see this as a first enablement step, and know we have to work on making it more optimal. And when I measured the llvm test-suite, this is exactly what I saw: no regression, minor uplifts (in Polybench and its solvers). But that was before the latest interchange patches went in, so I will verify and confirm this with the top of trunk. I would like to add that I have issues with the llvm test-suite for evaluating this kind of codegen changes as they may not be very visible, and I briefly talked about that in my llvm dev conference talk here (sorry for the plug, but it's a link to the exact timestamp). So llvm test-suite performance numbers need to be taken with a pinch of salt, but again, will do confirm. |
| @@ -201,7 +201,7 @@ static cl::opt<bool> RunNewGVN("enable-newgvn", cl::init(false), cl::Hidden, | |||
| cl::desc("Run the NewGVN pass")); | |||
|
|
|||
| static cl::opt<bool> EnableLoopInterchange( | |||
| "enable-loopinterchange", cl::init(false), cl::Hidden, | |||
| "enable-loopinterchange", cl::init(true), cl::Hidden, | |||
There was a problem hiding this comment.
Consider to also add switches to the clang driver like -fvectorize/-fno-vectorze and -funroll-loops/fno-unroll-loops. GCC's is called -floop-interchange.
There was a problem hiding this comment.
Thanks for the suggestion, agree that this would be convenient to have. I will create a separate patch for this.
|
Has #116144 (correctness issue) been addressed? |
Correct, should have been fixed. |
|
Re: perf number, @Meinersbur : I indeed see no regressions and 3 improvements of ~70% that are real, I think. There are a couple of minor improvements, but that could be real or noise, but didn't look further into it. I did 3 runs with interchange disabled ("before") and 3 runs with interchange enabled ("after"), and lower is better: |
This introduces options -floop-interchange and -fno-loop-interchange to enable/disable the loop-interchange pass. This is part of the work that tries to get that pass enabled by default (llvm#124911), where it was remarked that a user facing option to control this would be convenient to have. The option (name) is the same as GCC's.
I tried these three cases on my local and couldn't reproduce this result. I investigated and found that the loops aren't interchanged in all the three cases. I assume that all the loops that were interchanged in your experiment are the same one that initializes the array This loop has |
This introduces options -floop-interchange and -fno-loop-interchange to enable/disable the loop-interchange pass. This is part of the work that tries to get that pass enabled by default (llvm#124911), where it was remarked that a user facing option to control this would be convenient to have. The option (name) is the same as GCC's.
This introduces options -floop-interchange and -fno-loop-interchange to enable/disable the loop-interchange pass. This is part of the work that tries to get that pass enabled by default (llvm#124911), where it was remarked that a user facing option to control this would be convenient to have. The option (name) is the same as GCC's.
This introduces options -floop-interchange and -fno-loop-interchange to enable/disable the loop-interchange pass. This is part of the work that tries to get that pass enabled by default (llvm#124911), where it was remarked that a user facing option to control this would be convenient to have. The option (name) is the same as GCC's.
This introduces options `-floop-interchange` and `-fno-loop-interchange` to enable/disable the loop-interchange pass. This is part of the work that tries to get that pass enabled by default (#124911), where it was remarked that a user facing option to control this would be convenient to have. The option name is the same as GCC's.
This introduces options `-floop-interchange` and `-fno-loop-interchange` to enable/disable the loop-interchange pass. This is part of the work that tries to get that pass enabled by default (llvm#124911), where it was remarked that a user facing option to control this would be convenient to have. The option name is the same as GCC's.
I've checked and you're right. I used a slightly older build, but with all the correctness issues addressed it indeed doesn't trigger anymore on cholesky, lu and ludcmp. It still triggers on other polybench kernels (correlation and covariance), but due to their extremely low runtime and/or isn't on the hot path, doesn't show improvements. I think this indeed shows the potential of interchange, and is something I would like to address after enabling this by default. |
I agree with you. |
|
I would like to change the status from WIP to ready-to-merge as I believe the outstanding issues has been resolved: The last two DependenceAnalysis bugs and patches that address them have been merged:
We have enabled interchange in the Flang driver and compiler: This has been running for a while now, and one bug was found and fixed (thanks @kasuga-fj ): And another interchange bug was fixed: With all of this fixed, I think loop interchange is now ready to be turned on here too. Shall we merge this, @Meinersbur , @kasuga-fj , @sebpop, @nikic, @mshockwave? |
sjoerdmeijer
changed the title
|
Could you please provide updated compile-time and performance numbers? The implementation has changed a lot in the meantime. |
Yep, that makes sense. We focused a lot on compile-times, and after our last patch to improve this, we measured a 0.19% geomean increase. For different configurations the picture looked very similar to this: But I will measure again and confirm. |
|
@nikic Hi, let me ask some questions.
(NOTE: I think fixing the above one is not very difficult. If we should fix it, I will probably be able to submit a PR for it within a few days.) |
|
@kasuga-fj Yes, this is valid. The delinearization reasoning based on GEP source element types needs to be completely removed. |
OK, thanks, so I'll work on it. |
Here are the new compile time numbers @nikic : It shows almost no increase in compile-times. The reason is that it doesn't trigger in CTMark. For the entire LLVM test-suite, it now only triggers 2 times: So, one way to explain the numbers is that interchange is now really good in deciding that it is not going to do anything, which isn't a bad thing. While collecting compile-time numbers, I noticed that there is a problem with this patch. I.e. the patch doesn't actually add the pass to the pipeline, so it is not running interchange. This is related to the |
I don't think this actually enables loop interchange? You add the extra checks for EnableLoopInterchange, but EnableLoopInterchange itself is still false. |
|
Maybe we should modify here? llvm-project/clang/lib/Frontend/CompilerInvocation.cpp Lines 1964 to 1965 in 80b79ce |
Yep, you're right, here are the right numbers: |
This patch needs rebasing, it's old, in the meantime, I've added support for |
I personally think it's okay to remove |
|
I think the cl::opt option is mainly useful for use with opt rather than clang. (Ideally we'd not use cl::opt for this and instead allow specifying pipeline tuning options from the command line...) |
|
Thanks for your thoughts on this. I will go ahead and remove the cl::opt option. If I am not mistaken, I don't see precedent for having both the clang and cl::opt option, and for the use of opt I don't think we'll need the -enable-loopinterchange option. |
I think this is quite a serious regression, especially given that there are no binary changes. Is the regression coming mostly from DA? In that case, can we try to early-exit in LoopInterchange and avoid computing DA, if there are likely going to be no binary changes? |
We see geomean compile-time increases of 0.20% 0.30%, 0.13%, 0.17% and 0.18%, which I think is actually really good given that we are in fact talking about enabling/running 4 components: interchange, DependenceAnalysis, Delinearization and LoopCacheAnalysis. Most of the compile-time is spent on the required extra analysis, and we have spent significant effort on reducing it to this minimum compile time increases. You're right it's not triggering much, but that is also by design at this point, and again it still requires to calculate extra analysis info. |
|
@sjoerdmeijer What compilation options and target did you use? The result of Regarding the compile-time increase, I also think it's quite significant, and there is still room for improvement. Here are some suggestions:
For now, I'll work on the first one. As for the others, I haven't tried them and I'm not sure if they will really help, but I believe they are worth investigating. |
+1 on this, there are many many cases where we only want to run |
Thanks, I didn't know that file. I think we can do it. |
@kasuga-fj @artagnon Thanks for the comment. As mentioned in the patch description, the following patches are purely based on early exits, and they handle major cases. These patches have brought down the impact significantly. #118973 However, as @kasuga-fj mentions, there can still be some opportunities left, but unless we quantify, we are not sure how much benefit it will offer. Some of the opportunities you mentioned are known to me, but I have not quantified the gain yet. Tuning I'd also like to take a pause and ask what constitutes "significant" and what does not for compile-time numbers. I feel we are doing quite well on the geomean as the benefits of enabling the pass outweigh the cost. We can continue improving compile-time (patches welcome!), but the question remains unanswered - When do we say the penalty on compile-time is acceptable? |
ConstraintElim is comparable in complexity: it gets turned on here. Note that there were a lot of binary changes, which justified a geomean regression of 0.3% on ThinLTO. It also does a good job of not blowing up compile-time in degenerate cases (so, never a 0.76%). That said, I'm very enthusiastic about the prospect of DA operating in LLVM: once the patch lands, I will attempt to add runtime-checks functionality to DA, and gradually move LAA's users to DA (this will be a long-running project). Once there are more users of DA in the tree, and with the potential deprecation of LAA, I expect the costs to eventually be amortized. That said, we don't have very many contributors who have expertise in DA/LCA/LI at this point, and this is a chicken-and-egg problem: I hope that we will further be able to reduce/amortize costs after landing. |
Have you tried them? As far as my quick experiment, at least delaying the calculation of
Since interchange fails in most cases anyway, I don’t think reducing the value would cause major practical issues (although I’m not sure whether it actually helps with compile-time). We can always raise it again after improving
I think we also need to consider individual cases, not just the geomean. A compile-time increase of over 20% despite no changes is, at the very least, quite substantial. Although I'm very interested in enabling |
This is a work in progress patch to enable loop-interchange by default and is a continuation of the RFC:
https://discourse.llvm.org/t/enabling-loop-interchange/82589
Basically, we promised to fix any compile-time and correctness issues in the different components involved here (loop-interchange and dependence analaysis.) before discussing enabling interchange by default. We think are close to complete this; I would like to explain where we are and wanted to check if there are any thoughts or concerns. A quick overview of the correctness and compile-time improvements that we have made include:
Correctness:
Compile-times:
And in terms of remaining work, we think we are very close to fixing these depenence analysis issues:
The compile-time increase with a geomean increase of 0.19% looks good (after committing #124247), I think:
See also:
http://llvm-compile-time-tracker.com/compare.php?from=19a7fe03b4f58c4f73ea91d5e63bc4c6e61f987b&to=b24f1367d68ee675ea93ecda4939208c6b68ae4b&stat=instructions%3Au
We might want to look into lencod to see if we can improve more, but not sure it is strictly necessary.