- User Since
- Mar 16 2019, 2:09 PM (45 w, 1 d)
Sat, Jan 25
Fri, Jan 24
Tue, Jan 21
Fri, Jan 17
Thu, Jan 16
Wed, Jan 15
I did a few experiment with bottom-up algorithm before the patch i showed on phabricator. my implementation of the bottom-up had similar average complie-time to the current pass on the test-suite but it was only barely removing more stores than the current pass.
i gave up on it because i didn't found a good way forward to make it deal with cases like to the following:
store i32 0, i32* %Ptr. ; DEAD br i1 %cond, label %a, label %b a: store i32 2, i32* %Ptr br label %c b: store i32 2, i32* %Ptr br label %c
which is IMO definitely something we want to do. by the way gcc does this optimization. https://godbolt.org/z/VFBf-c
maybe there is a bottom-up way to deal with this that i didn't thought about. any thought ?
Tue, Jan 14
i renamed the files to KnowledgeRetention.* to be more general. we can put other related utilities like the query API in the same files. i had a hard time finding a good name and i am not sure i succeeded what do you think ?
Mon, Jan 13
something like that seem fine.
@rsmith friendly reminder
Sun, Jan 12
Fri, Jan 10
Thu, Jan 9
i changed the pass and the test to actually perform the transformation. it is less confusing this way.
Tue, Jan 7
Mon, Jan 6
Sun, Jan 5
Yes, PostDominators is the second thing we need to get better at preserving. IIRC all places where we already use DomTreeUpdater we basically preserve PDT as well without any additional work, besides passing in the PDT to the updater. There's getAnalysisIfAvailable (legacy pass manager)/getCachedResult (new PM) to get an analysis, only if it is already computed. The only caveat is that the legacy pass manager cannot preserve function analysis if it is followed by a function pass (IIUC).
Sat, Jan 4
Ok seems reasonable, so we will change the core loop for each store will have to change in a following patch.
IMO the most productive way forward would be to get in a patch with limited but solid support as a baseline and once that is in we can work on adding missing functionality in parallel. What do you think about this approach?
you are right. i probably went a bit to far for a first patch.
Besides extending the functionality, I think we will also have to spend a fair amount of work on making sure that preceding and succeeding passes in the pipeline preserve MemorySSA, to avoid computing it from scratch just for DSE.
yeah. and we may have to do the same thing for the post dominator tree.
is there a way to have a pass preserve an analysis if it is still valid without depending on it but not updating it if it is not valid ?
improved the test suite.
Fri, Jan 3
i haven't looked in details at your patch but i was working on something similar D72182.
This update should fixes the issue.
the assert was incorrect. because file offsets are 0-based where as column numbers are 1-based.
Mon, Dec 30
Dec 22 2019
Dec 19 2019
i agree that it is the best solution.
Dec 18 2019
my intent was not necessarily to create a generic solution but to get a solution that is sufficient to not have false positives on the Wmisleading-indentation warning for code base that have mixes of tabs and spaces.
Dec 14 2019
Dec 12 2019
Dec 10 2019
now that the issue with uniqueness of expressions is solved. we should be able to keep going on that review @rsmith.
https://reviews.llvm.org/D63960 should be i think close to completion. so maybe for testing i could use immediate invocation as a source for ConstantExpr instead of the code i added to make constexpr variables emit ConstantExpr ?
Dec 9 2019
Dec 5 2019
comments were fixed.
i have not tested the performance impact. but i tried to do the heavier checks last to minimize the impact.
i added your tests file to the improvement commit.
fixed aaron's comment
i ran a build of the linux kernel using the stage1 compiler, about 30% of files crashed the compiler in the optimizer. everything was building fine 2 weeks ago with the same configuration but a previous compiler.
but aside from that from that file that complied fine, there was only one warning left. this warning is exactly the same case we had in llvm's source code.
Dec 4 2019
the warning was not affected by -ftabstop. i made a patch that fix this.
i did a few test on the linux kernel on prior version of this patchs and the mix of spaces and tabs caused some false positives. but i do believe there is still a bug here.
for the mix of space and tabs. gcc has a -ftabstop=X to specify how large tabs should be counted as.
clang has it as well i am going to check that it is taken in account.
Dec 3 2019
Dec 2 2019
improved based on aaron's comment.