- User Since
- Mar 18 2014, 10:33 AM (379 w, 2 d)
This LGTM, once you add the RUN line for llvm-dis I suggested in the last comment. It's up to you whether to split out the changes for moving the LLVMContext construction after command-line parsing into a prep commit... there could be an argument for keeping it together as well.
LGTM, once @MaskRay is happy. I have a couple of minor comments inline too.
(Apologies for disappearing; I was out for a bit, and then lost track of this review.)
LGTM, once all the code is migrated.
Given how large this is, would it be reasonable to split this up a bit more?
Minor update to the testcase:
Tue, Jun 22
Mon, Jun 21
I agree with @dblaikie that it'd be nice to get a clear answer for the behaviour (and documenting it in LangRef). Also, I wonder about ignoring the instruction attribute when the function declaration exists... seems like that is a step toward removing the instruction attribute entirely (maybe that's the right thing to do (not sure it is), but if so then it should be a high-level bit, not a side effect).
Fri, Jun 18
I pushed this as 05d0f1a8ea012a6b7b8ea65893ec4121106444b5 last night, but I just realized I forgot to include the link to the review :(. Closing manually.
Thu, Jun 17
Another thing to consider: a client that wants the minimized source to be "no bigger than" the original source can use the original source if it ends up growing. For example, https://reviews.llvm.org/D104462 could check the resulting size, and just return the original source in the (extremely unlikely?) corner case where the minimized sources are bigger.
I'm not sure D104465 (the motivation for this patch) is the right approach, but we can have that discussion over there.
This patch fixes dependency scanning of a TU with prebuilt modular/PCH dependencies.
Such modules/PCH might have been built using non-minimized files, while dependency scanner does use the minimized versions of source files. Implicit build doesn't like the discrepancy in file sizes and errors out.
The issues is worked around by padding the minimized files with whitespace in such scenarios. This approach throws away one advantage of source minimization (the memory savings), but still keeps the benefits of faster preprocessing/lexing.
This patch ensures the output of source minimizer is never larger than the input. This property will be handy in a follow up patch that makes it possible to pad the minimized output to the size of the original input.
Tue, Jun 15
Sun, Jun 13
I'm not sure the performance problems with std::map will matter in practice here, but have you considered sorting before emission rather than relying on the data structure's iteration order? (It'd make it easy to switch to StringMap in the future.)
Okay, LGTM. (Sorry for the delay, I've been out.)
LGTM, with one suggestion inline.
Thu, Jun 3
This looks much cleaner; thanks for the update!
Wed, Jun 2
Tue, Jun 1
@zahen , what author information should I use for the Git commit?
LGTM; thanks for you patience.
Is there (or could there be) a mode where clang-scan-deps prints out the command-lines it sends to the dependency scanning action (maybe instead of actually scanning), so this could be tested?
Fri, May 28
Thanks for your patience! I thought I already looked at this last week.
Thu, May 27
This doesn't look like a particularly expensive verifier check (although maybe I've missed something). What's the motivation for hiding it behind EnableSwiftTailCCMustTailCheck? Have you considered using bitcode AutoUpgrade to fix old IR?
LGTM; thanks for splitting this out.
Wed, May 26
Naively, this sounds like it could be a non-trivial tax on build times. But it looks like it's only called in Clang from Sema::diagnoseMissingImport, which only happens on error anyway.
@dblaikie, wouldn't mind your thoughts on this idea (expanding the interface of llvm::thread to include an optional stack size parameter).
I wonder, could this be used to simplify RunSafelyOnThread, and/or llvm_execute_on_thread_impl? I figure all that complexity is because of the same limitation, but maybe it's not something we can fix, since we exposed a libclang API (clang_executeOnThread) that allows specifying a stack size.
May 21 2021
LGTM! I suggest mentioning the original commit (0a8e12fdbbf54e81954b091ac9fb11cd5f5148e1) and the commit that changed the bitcode (2a078c3072043541ee0595aea6c8d7909f94c6f9) in your commit message.
May 20 2021
May 19 2021
Can you include the revert to attributes-3.3.ll.bc as part of this?
May 17 2021
May 15 2021
May 14 2021
The new paragraph or something similar LGTM, although probably @theraven should be the one to sign off given the post-commit feedback.
LGTM, thanks for splitting out and moving the tool to clang/utils.
Looks great — thanks for splitting this out!
The code looks mostly good; some inline comments.
LGTM, although I'd slightly prefer the change to CompilerInstance.h be split out and committed after.
May 13 2021
LGTM too. This is great to have; thanks for writing this up!
May 11 2021
May 10 2021
The code LGTM too, just a couple of nits on docs and tests. Not sure if @t.p.northover has time to look right now, but I added him in case he does. I'm excited to see this moving along!
Ping! Happy to weaken the commit message to "likely NFC" in case there's some way that we get back here after a fatal error.
LGTM with a couple of nits about the test.
May 5 2021
May 4 2021
May 3 2021
Yep, this fixes a regression (https://bugs.llvm.org/show_bug.cgi?id=50033) caused by ad7aaa475e5e632242b07380ec47d2f35d077209, where I somehow missed that modules and PCHs had different actual behaviour around -fno-temp-file despite sharing the same comments.