- User Since
- Aug 20 2012, 11:51 AM (269 w, 3 h)
Sep 10 2017
FWIW, this makes sense to me. (and I like the naming/path choices)
Sep 7 2017
Sorry for the delay.
Aug 30 2017
Aug 29 2017
Seems pretty straightforward. LGTM.
Aug 7 2017
One way to break up this large RefSCC is to notice that the fanout from the vtable ref edges in the ctors/dtors is contributing a lot to it being so large. If those edges are made more precise, then it can reduce the size. For example, by breaking the vtable ref edges we prevent the ref edges to virtual GetMetadata methods and so the huge RefSCC breaks up as I mentioned earlier.
Aug 5 2017
Sorry, to be clear, the entire RefSCC is something like:
When compiling the IR for this file with 'opt' and 'O3', this patch reduces the total time by 8-9%.
Jul 18 2017
Jul 15 2017
Jul 13 2017
I think you should be able to deal with most of those issues by sorting the list of sections with an appropriate sorting predicate that coalesces sections that need to be adjacent, which should be fairly unintrusive.
Jul 11 2017
It is too fragile to manually specify file offsets. The actual layout that yaml2obj does right now should be considered opaque, such that any file offsets you could manually choose are "wrong" (even if they are incidentally correct with the current layout of sections that yaml2obj chooses).
Jun 30 2017
Another possible naming convention that might avoid the scenario Matthias is worried about is all/everything like clang's -W options.
IIRC the intention was that in-tree work should not be burdened by experimental targets, up to and including breaking them. E.g. an in-tree API migration wouldn't have to update experimental targets. So in that sense LLVM_TARGETS_TO_BUILD=all is working as intended I think.
Jun 15 2017
Thanks, I like this idea. We might want to do something similar for the tests.
Jun 9 2017
Apr 24 2017
Mar 24 2017
Mar 21 2017
Btw, I think this is one of the clearest proofs that LLD has crossed a big milestone and is now entering "production quality". You don't see this kind of hack in a non-production-quality program.
Lgtm with a some suggestions that I don't feel too strongly about.
Mar 20 2017
Overlapping candidates are now handled entirely by the pruneOverlaps method that handled anything the suffix tree pruning didn't handle. Since that method is on average O(n), this behaves far better.
Mar 6 2017
Feb 28 2017
Feb 27 2017
Nice to see this finally land!
Feb 25 2017
btw, down the road you may want to have this pass really know in detail the encoded length of each instruction on x86. There are quite a few *single instructions* that would be beneficial from a code size perspective to outline (if the outlined function is set to have alignment of 1). A quick analysis of an LLD binary (which contains all of LLVM linked in for LTO) shows there is over 5% code size savings just from outlining single instructions (since many x86 instructions encode to be larger than a CALL instruction which is 5 bytes). About half of the benefit (so about 2-3% of the total on this test case) comes from instructions that reference the stack via %rsp (mostly zeroing out stack slots), which could still be outlined if the offset was rewritten.
Feb 24 2017
Also, this pass will almost surely introduce timing side-channel attacks into cryptography code (code that would otherwise by "constant time" and needs to be for security).
I just tested on SPECCPU2006 (FullLTO) and no assertion failures!
Feb 23 2017
FYI this doesn't build on Linux with gcc
Feb 22 2017
Feb 21 2017
r295807, thanks! (and sorry for the delay)
Feb 17 2017
Thanks, this LGTM. Do you need me to commit this for you?
Feb 3 2017
This change does two things (as you mention in the description):
Feb 1 2017
One key issue here is that this analysis mutates the IR, which is rarely done in LLVM, and I think we are trying to remove all cases of doing that (as Chandler puts it in http://llvm.org/devmtg/2014-04/PDFs/Talks/Passes.pdf slide 17 "Forms a new, sub-set IR, which is problematic"). On that point, you probably want to start a discussion on LLVMdev (with a link to this review). I anticipate at least Chandler is going to have something to say about this w.r.t the new PM, so make sure to CC him.
Thanks for working on this!
Jan 23 2017
We have a chdir in lib/Basic/VirtualFileSystem.cpp that probably can be moved to using this API in a followup patch.
Jan 21 2017
Jan 20 2017
Are you sure that this CONSTRUCTORS is trying to be a linker keyword? The nearby DATA_DATA is actually a macro expanded from http://src.illumos.org/source/xref/linux-master/include/asm-generic/vmlinux.lds.h#205
Is the CONSTRUCTORS still in file after it has been preprocessed? (I'm guessing so since otherwise this patch wouldn't fix the build, but just wanted to be sure).
Remember to keep in mind that these linker scripts all get preprocessed by the C preprocessor, so remember to look at the preprocessed linker script instead of the original source.
Jan 17 2017
In case you're wondering, that didn't take that long to reduce to that point. Just have to learn the tools and workflow. We have amazing tools for doing test case reduction and an easily understandable test-suite build system! (mad props to those who made bugpoint; the folks that made test-suite awesome; and LLD's -save-temps; and a bit of -globalopt and -metarenamer to clean things up)
I did a quick run on SPEC CPU 2006 with FullLTO and it seems like I ran into 3 different assertion failures across various programs: https://reviews.llvm.org/P7954
There seem to be 3 different assertions getting hit.
Jan 16 2017
Jan 14 2017
It would be good to build up some verification machinery here at some point in the future.
I couldn't come up with a good name for this either. I described a design in my log here: https://docs.google.com/document/d/1-nrq2y_hTiZhrsJDmH8TzFjFEeApfccs6wwGyaXjSkg/edit#heading=h.ut7bc0x07mmh
Jan 10 2017
Jan 8 2017
Jan 7 2017
can you put the declaration/definition closer to sys::path::native? Other than that this LGTM.
Dec 29 2016
And FWIW, based on my experience working on the new PM, I do agree that the new PM unit tests are currently harder to understand and debug than they need to be (for reasons that you outlined well in that LLVMdev thread years ago and in the description of this patch). Especially getting more useful error reports out when the tests fail would be fantastic, so I'd really like to see that aspect of gmock! When you post a patch updating some new PM tests to use gmock, definitely include some example error output from when the test fails.
I think that #2 is relatively rarely useful, but there *are* cases where
it is useful. Historically, I think misuse of actual mocking as
described in #2 has led to resistance towards this framework. I am
actually sympathetic to this -- mocking can easily be overused. However
I think this is not a significant concern in LLVM. First and foremost,
LLVM has very careful and rare exposure of abstract interfaces or
dependency injection, which are the most prone to abuse with mocks. So
there are few opportunities to abuse them. Second, a large fraction of
LLVM's unittests are testing *generic code* where mocks actually make
tremendous sense. And gmock is well suited to building interfaces that
exercise generic libraries. Finally, I still think we should be willing
to have testing utilities in tree even if they should be used rarely. We
can use code review to help guide the usage here.
Dec 24 2016
This is a good idea. Tar is a much more common format that users will be more familiar with. I'm not an expert on Tar files, so I can't review that part of the patch, but hopefully the bots will show if any tar implementations don't like the files we produce.
Dec 21 2016
Dec 20 2016
Nice! Thanks for adding the unittest too!
Dec 18 2016
This transform seems like it is sensitive to the order in which functions are visited when running function passes, which I think we try to avoid. I.e. if the ordering is right, we can propagate across arbitrarily many transitive calls; but if the ordering is wrong (e.g. we visit callers before callees) then we don't be able to propagate as much.
error: file-dbg2.o: duplicate symbol 'zed' error: file-dbg2.a(file-dbg2.o): previous definition was here note: definitions found in conflict-debug.s(.text+0x0) and conflict-debug.s(.text+0x0)
Thanks for doing this, LGTM with a nit. Overall, it seems that keeping two copies for now is fine, since it is such a small amount of code.
Dec 14 2016
A couple nits. Overall, this is looking a lot better. It's awesome that you sat down with Justin to hash this out.
Committed in r289733
Update local variable name to be more consistent.
Dec 13 2016
Dec 12 2016
A couple more small comments as take another pass looking at the patch.
Dec 11 2016
A couple suggestions to make this patch more understadable.
Dec 9 2016
Some stylistic comments.
Simon has done a pretty good job with the style review. Once his comments are cleaned up it will be easier to provide further comments.
Also, uhm, this needs tests. Maybe you can get away with just adding some more RUN lines to some LegacyGVN tests?
Dec 6 2016
When running with just a single core, can we still do a sharded visitation, but instead of doing NumShards passes through all the pieces (skipping ones not in the current shard), only do a single pass? We should be able to get identical layout in that case.
Dec 5 2016
Dec 4 2016
LGTM with a couple wording suggestions. Thanks.
LGTM. Thanks for working on this!
Dec 2 2016
Please do this kind of cleanup with post-commit review in the future.
If you feel strongly about this then go ahead, though if you're renaming this better to rename it lookUpAnalysis.
This is fine, but you can use post-commit review on this kind of thing.
Feel free to commit this kind of cleanup with post-commit review.
This type of cleanup can be done with post commit review btw.