- User Since
- Jun 17 2015, 7:07 AM (213 w, 4 d)
Fri, Jul 19
I prefer having the documentation change to be in the same patch as the functional change. Makes it easier to check whether they match.
Thu, Jul 18
Generally, the patch looks fine. I'd would prefer if someone who has contributed to VPlan to accept the patch.
[serious] Need to update docs.
Wed, Jul 17
LGTM, some nitpicks left.
So far, we have the following guarantees of a loop guard:
- If branching into the direction of the loop, the loop will be executed
- If branching to the other direction, the loop will not be executed.
- If branching to the other direction, no code will be executed that would not be executed if going to the direction of the loop.
- Loop guard and the post-dominating 'other' block form a Single-Entry-Single-Exit region. [edit: not true; there can be edges from the outside to a loop exit block]
Mon, Jul 15
IMHO, this loops like an option of a particular transformation, not an independent pragma. E.g.
#pragma clang loop vectorize(enable) vectorize_remainder(predicated)
There could be multiple choices for how to execute remainder iterations, e.g. instead of an epilogue, the first iterations could be executed in an prologue. Or an option where the compiler may assume that the iteration-count is always a multiple of the vector width, etc.
Fri, Jul 12
Thu, Jul 11
Wed, Jul 10
To not to be stuck in details and not block the dependence graph patches, we maybe should land the patch and work on it in-tree when its users materialize?
Windows seems to work. Good job!
Tue, Jul 9
LGTM! Many thanks!
Mon, Jul 8
@fhahn Do you still want to look over this patch?
What's the intended use?
To reduce the number of allocations, have you thought about making EdgeList contain the edges objects instead of pointers? The edges would have to be copied/moved into the list and edges could not be compared by identity. Is this semantic needed/are edge objects large?
I think you are ready to get commit access, if you like. Could you follow https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access? Send him some links to commits that I committed for you.
Note that there is a move to GitHub pending (end of this year, as far as I remember), which may require another procedure (SVN Username+Pwd hash do not directly translate to GitHub accounts).
Thank your for working on the documentation!
Wed, Jun 26
Thank you for applying most of my suggestions.
Could you clarify the title to make clear that it moves more than just the finalizeAccesses method, such as "Move finalizeAccesses and its callees to ScopBuilder"?
Can I commit this patch for you?
Tue, Jun 25
[suggestion] RecordedAssumptions itself is only used during SCoP constructions (since it's empty once the SCoP is full constructed). It could be removed entirely into ScopBuilder, maybe in a future patch?
Jun 18 2019
How would a pass use this analysis? It computes a cost for the current IR, but there is nothing to compare it to unless the transformation pass emits the transformed loop nest next to the original pass such that the LoopCacheAnalysis can compute its cost.
Jun 17 2019
What do you plan as the first use of this transformation?
Jun 14 2019
LGTM, please wait for @fhahn's ok as well.
Jun 13 2019
Thanks for the patch. Do you happen to have performance, code size numbers?
Jun 12 2019
Jun 11 2019
LGTM apart from the nitpicks.
LGTM and thank you!
LGTM. Thank you.
Jun 10 2019
As I understand the language reference, ("The optimizers may change the order of volatile operations relative to non-volatile operations.") the loop can be distributed as long as there all volatile operations stay in the same loop. I this is meant to test the current implementation, can you add a comment to make this clear (such as "TODO: distribution of volatile may be possible under some circumstance, but the current implementation does not touch them")
Jun 7 2019
What problem is this solving?
Would an assertion in LoopVersioning checking whether it accidentally copies a convergent operation make sense?
Jun 6 2019
- Address @hfinkel's remarks
Just an idea: We could avoid the explicit calls to 'initializeXYZPass' in opt/bugpoint/clang by adding Polly.cpp as a source file or object library to the executables. This would guarantee that its static initializer is called without dynamic library.
Can you move getNonHoistableCtx/addInvariantLoads to ScopBuilder as well? they contain logic only relevant for hoisting.
Jun 5 2019
Sorry, you might have tested it with LLVM_LINK_POLLY_INTO_TOOLS=OFF and/or BUILD_SHARED_LIBS/LLVM_LINK_LLVM_DYLIV where it might work, but unfortunately not with the default configuration using static component libraries.
This fails with Polly/Linux regression tests:
******************** FAIL: Polly :: Simplify/gemm.ll (1148 of 1149) ******************** TEST 'Polly :: Simplify/gemm.ll' FAILED ******************** Script: -- : 'RUN: at line 1'; opt -polly-process-unprofitable -polly-remarks-minimal -polly-use-llvm-names -polly-import-jscop-dir=/home/meinersbur/src/llvm/tools/polly/test/Simplify -polly-codegen-verify -polly-import-jscop -polly-import-jscop-postfix=transformed -polly-simplify -analyze < /home/meinersbur/src/llvm/tools/polly/test/Simplify/gemm.ll | FileCheck /home/meinersbur/src/llvm/tools/polly/test/Simplify/gemm.ll -- Exit Code: 2
Jun 4 2019
Jun 3 2019
Do you want me to commit?
It still fails with the same error. LINK_POLLY_INTO_TOOLS is only set after the polly subdirectory is processed. Because it is an option, the ON value will be stored in the CMakeCache.txt and be available during the next run, it actually works after running the cmake configure step a second time. We should not expect users to do so. Because of this, any option(...) or set(... CACHED) should be done at the beginning of the file.
May 31 2019
May 30 2019
Do you want me to commit?
May 29 2019
For LTO, the llvm_extensions might need to be linked into lld as well. The recent polly-dev thread: https://groups.google.com/d/msg/polly-dev/BeC_v_oJIVM/LAA7vVT9AwAJ