- User Since
- Aug 20 2012, 11:51 AM (252 w, 5 d)
Thu, Jun 15
Thanks, I like this idea. We might want to do something similar for the tests.
Fri, Jun 9
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.
This is fine, but it doesn't really matter; this is only called like once for an entire compilation (it's basically a hack to set a pipeline-global closure for a particular analysis; only AA uses it now)
Overall, I do like the idea of doing this and this LGTM (although let others chime in). One thing to think about is that if it takes this long to delete a file (even on tmpfs which Joerg mentioned), then how long does it take to allocate the memory for a new file? (let alone write it to disk)
Dec 1 2016
LGTM. Do you need me to commit this for you?
nice to see this feature finally implemented!
This LGTM. Thanks for looking so closely at this! It's a very nice speedup!
Abandoning now that D27156 has landed.
Nov 30 2016
Why are we unlinking at all? Hopefully we can just resize the file and ideally reuse most of the pages the kernel already has allocated. Doing so loses the atomicity of the replacement, but so does asynchronously unlinking like you do in this patch (although this patch would manifest the non-atomicity in a more benign way).
This is just wiring up the new machinery from D27198. Looks good.
This makes sense given your current trajectory for the new PM. LGTM with a nit.
Small fixup (delete some code that I forgot to remove in the last patch).
the original simple-minded constexpr containing a for-loop
Somewhat simpler template version. Let me know what you think. I think the improved clarity at the call site is worth it.
Nov 29 2016
Nice! The idea of storing current and next colors solves the nondeterminism in a very simple way!
Sorry for the delay.
I don't think that it makes sense to put this in a file with a generic name like "Concurrent", as this is a quite specialized data structure that depends on specific LLD types. Maybe just call it ConcurrentStringTableBuilder.h?
I'm a bit worried about the layering too, does this class introduce any circular dependencies?