In D104819#2839315, @mstorsjo wrote:

In D104819#2839263, @dexonsmith wrote:

Just be sure to clang-format when you do the mechanical changes in the follow up patches.)

Hmm, those would have to be manually reviewed, but I guess I can try to do that (discarding changes where the surrounding code isn't too well formatted now already so it does more extensive reformattings other than for the renaming).

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.)

In D104516#2838979, @rriddle wrote:

In D104516#2838941, @nemanjai wrote:

In D104516#2838887, @rriddle wrote:

In D104516#2838885, @lei wrote:

The MLIR bot, https://lab.llvm.org/buildbot/#/builders/88/builds/14468, Have been red for over 30hr when this patch landed. Please provide a fix or pull the related patches to bring the bot back to green.

Are those related to this patch? Those all look like orc failures.

I think pulling in additional code has caused a relocation overflow in the runtime linker. We might have to switch to the large code model for ORC on PPC.

Thanks for looking! That definitely looks like what is happening. Do you know how to adjust that? (I have very little experience/knowledge of ORC)

LGTM, once all the code is migrated.

Given how large this is, would it be reasonable to split this up a bit more?

In D104805#2836748, @dexonsmith wrote:

In D104805#2836738, @nikic wrote:

Can you please point me to the code where the use-list order prediction is made?

I'll need a minute to find it for BitcodeWriter.

In D104805#2836738, @nikic wrote:

Can you please point me to the code where the use-list order prediction is made?

Minor update to the testcase:

In D104740#2836529, @dexonsmith wrote:

I'll look for other similar bugs and post a patch.

In D104740#2836407, @dexonsmith wrote:

In D104740#2834433, @nikic wrote:

Preserve address space check, add test with argument.

While doing the latter, I found out that

define void @call(void (...)* %p) {
  call void (...) %p(void (...)* %p)
  ret void
}

fails verify-uselistorder (independently of this patch). I happened to use that as the first thing I tried :/

Interesting; given that there are two uses of %p maybe one of them isn't being tracked. Did you investigate?

In D104740#2834433, @nikic wrote:

Preserve address space check, add test with argument.

While doing the latter, I found out that

define void @call(void (...)* %p) {
  call void (...) %p(void (...)* %p)
  ret void
}

fails verify-uselistorder (independently of this patch). I happened to use that as the first thing I tried :/

In D104218#2836398, @mstorsjo wrote:

In D104218#2833781, @dexonsmith wrote:

In D104218#2832463, @mstorsjo wrote:

In D104218#2832251, @lattner wrote:

That said, xxx_lower() is a really terrible name here for insensitive comparisons. We have StringRef::lower() which returns a lower case string - it isn't insensitive. It seems that the whole family needs a rename. Could you take a crack at this as a follow-up? It should be a mechanical change.

Sure, I can give that a shot. Any suggestion on what the name pattern should be? xxx_caseinsensitive() is a bit of a mouthful, xxx_i() is terse but less understandable and discoverable.

A few ideas to consider:

• X_casei()
• X_nocase()
• X_ignorecase()
• X_foldcase() (term from text search)

Thanks for the suggestions! What do you think of @lattner ‘s suggestion (X_insensitive)?

In D104663#2834145, @aeubanks wrote:

As for preallocated, my stance is still the same. It's not in used at all anywhere (I'm the person who implemented it but never got around to finishing it up).

LGTM.

In D57688#2833512, @ldionne wrote:

I might revisit this later, but I'm not moving forward for the time being since a bunch of people said it was going to break them.

I think we should instead ship those headers as a separate project and people who need them can still get the headers from that project. IOW, they shouldn't be part of libcxx. But I don't have any time to work on this low priority cleanup at the moment, so abandoning.

In D104218#2832463, @mstorsjo wrote:

In D104218#2832251, @lattner wrote:

That said, xxx_lower() is a really terrible name here for insensitive comparisons. We have StringRef::lower() which returns a lower case string - it isn't insensitive. It seems that the whole family needs a rename. Could you take a crack at this as a follow-up? It should be a mechanical change.

Sure, I can give that a shot. Any suggestion on what the name pattern should be? xxx_caseinsensitive() is a bit of a mouthful, xxx_i() is terse but less understandable and discoverable.

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).

I pushed this as 05d0f1a8ea012a6b7b8ea65893ec4121106444b5 last night, but I just realized I forgot to include the link to the review :(. Closing manually.

In D104465#2825343, @dexonsmith wrote:

A few other ideas:

1. Disable PCH validation when minimizing. (Is that less robust than your current workaround? If so, can you explain why?)
2. Use the original PCH header in the scanning -cc1s (translate -include-pch to -include) and switch back in the generated -cc1s (back to -include-pch).
3. Embed instructions in the PCH for how to build it, and make a "minimized" version of the PCH.

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.

LGTM.

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.

In D103930#2820725, @bruno wrote:

Thanks for working on this, comments inline. @vsapsai @jansvoboda11 @dexonsmith any headermap related concerns on your side?

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.)

LGTM.

LGTM.

LGTM too.

Okay, LGTM. (Sorry for the delay, I've been out.)

LGTM, with one suggestion inline.

LGTM.

LGTM.

In D102763#2815285, @compnerd wrote:

Given the thread on the mailing list, I think that this is fine as long as there is an accompanying test to validate the bitcode deserialization continues to work. Id wait a few days more to see if @dexonsmith has an opinion.

This looks much cleaner; thanks for the update!

@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?

In D103165#2786618, @t.p.northover wrote:

I guess one option would be to never use std::thread, and repurpose the guts of llvm_execute_on_thread_impl to implement a cross-platform llvm::thread that allows specifying the stack size as an optional constructor parameter (and then change llvm_execute_on_thread to use llvm::thread). WDYT?

The llvm_execute_on_thread family only seems to have three users and is definitely a less appealing interface, I'd be very tempted round this llvm::thread out as you suggest and switch all users to it.

Thanks for your patience! I thought I already looked at this last week.

In D103275#2787268, @aschwaighofer wrote:

I guess that the 'in tail position' restriction cannot really be used by frontends because it has to predict that otherStuff() cannot be optimized away. So it would not buy us much other than a loss of clarity ...

In D102943#2786015, @arames wrote:

This new version is an attempt to have modules not rely on llvm::hash_code, but on a new llvm::stable_hash_code.
I understand modifying ADT/Hashing.h is sensitive, so maybe we need to discuss the high-level approach first.

In D103275#2785679, @dexonsmith wrote:

I guess I'm wondering what the trajectory is - if the goal is for this to eventually be an IR invariant for the calling convention, I think a verifier check makes sense; once the codegen bugs are sorted out, and the bitcode upgrade is done, the flag can be switched to on-by-default and eventually removed. But if this will never be an invariant of the IR, I'm not sure the LLVM IR verifier is really the right place for the check.

In D103275#2785642, @varungandhi-apple wrote:

I used Assert for two reasons: (1) the surrounding code uses Assert, so it seemed like the right thing to use and (2) in case there's a failure, it will display the instruction missing the musttail.

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.

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.

In D102943#2775115, @pcc wrote:

Isn't the bug here that module hashing is using hash_code? So shouldn't the correct fix be to use a specific hashing algorithm for module hashes?

In D102633#2773578, @aganea wrote:

In D102633#2769762, @arphaman wrote:

It might be good for @aganea to take a look as well.

Thanks! I actually work with @saudi, I already took a look at the patch before uploading. However I'm stil torn about running one of the workers on the main thread. I fear that we could have random errors because of the stack size of the "main" thread could be different from the stack size of the "satellite" threads. There's 99.99% chance that this won't happen, but I'd prefer that behavior to be explicit.

We could have:

-j0: use all hardware threads
-j1: don't use multi-threading, run all the tasks on the main thread
-jN: use the specified number of threads

The rationale is that we're using clang-scan-deps as-a-DLL in Fastbuild, to extract the dependencies. Since Fastbuild already has its own thread pool management, we call into clang-scan-deps with -j1 from different Fastbuild threads (but keeping the DependencyScanningService alive between calls). It would great if each call to clang-scan-deps wouldn't create a extra new thread.

Perhaps any of you would like to comment? +@mehdi_amini

In D102726#2774689, @jdoerfert wrote:

In D102726#2774657, @nikic wrote:

FYI, this change causes a non-trivial compile-time regression in LTO builds: https://llvm-compile-time-tracker.com/compare.php?from=136ced498ba84f6b6126051626e319f18ba740f5&to=5b6cae5524905bc43cfc21a515f828528d1f2e68&stat=instructions (Worst regression seems to be the ThinLTO kc.link step, which is up 4%.)

LLVM attributes are optimized for immutable usage, and it's hard to overstate just how slow the mutable APIs on them are...

I suggested not to check for attributes just to remove them. I still think that's the right thing *but* we can check for them in the remove functions.
@nikic you think that will resolve the compile time issue?

In D102943#2774732, @jroelofs wrote:

why do module hashes need to be stable when cross-compiling?

LGTM.

LGTM! I suggest mentioning the original commit (0a8e12fdbbf54e81954b091ac9fb11cd5f5148e1) and the commit that changed the bitcode (2a078c3072043541ee0595aea6c8d7909f94c6f9) in your commit message.

In D102726#2771958, @steven_wu wrote:

I will do that as a followup.

In D102726#2769856, @steven_wu wrote:

In D102726#2769821, @dexonsmith wrote:

Can you include the revert to attributes-3.3.ll.bc as part of this?

I don't think that matters too much

Can you include the revert to attributes-3.3.ll.bc as part of this?

In D102201#2761154, @thakis wrote:

We bisected a +153kB binary size increase for chrome/android to this (https://bugs.chromium.org/p/chromium/issues/detail?id=1208513#c4). Is that expected? (That build config uses -Oz for many TUs.)

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.

LGTM.

In D102292#2760193, @aeubanks wrote:

In D102292#2759011, @theraven wrote:

In D102292#2757941, @dblaikie wrote:

Yeah, that seems out of scope for this document. https://llvm.org/docs/LangRef.html#pointer-type documents that pointers to functions and pointers to other things are all roughly the same sort of thing - and we're changing all of them over to the opaque pointer type. & other wording generally treats all pointers as having the same size, etc. I think that's best left elsewhere than this doc if there's no interesting distinction between function pointers and data pointers currently (which it doesn't seem like there is).

Note that we have support for different code and data representations with the P flag in the DataLayout. We can also differentiate between pointers to globals with G and there's an under-review patch for a specified address space for allocas. I think it's worth highlighting in this document that address spaces are the recommended way of representing different kinds of pointers (with potentially different sizes, access to different types of memory, representations of null, and so on). We retain the distinction between pointer types where the semantics matter for lowering, we lose the distinction when the distinction doesn't convey any useful meaning.

This is good information to know (I didn't know about it before), but IMO address spaces should have their own documentation (that I'd be happy to link to here). This seems orthogonal to opaque pointers.

LGTM too. This is great to have; thanks for writing this up!

In D101704#2751664, @aeubanks wrote:

update test
add to ReleaseNotes
mention that opaque pointer types aren't ready for general use yet

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.

In D100934#2735955, @jansvoboda11 wrote:

In D100934#2733857, @dexonsmith wrote:

Given that there are four different things being done in this commit, it sounds naively like it should be four separate commits. If the logic is too intertwined to land each of the four pieces separately (certainly possible), can you quickly explain why, to motivate landing them together? (Or maybe there's an NFC refactoring that could be separated as a prep, to make the functional changes more isolated / easy to reason about?)

I think that landing everything together provides more context for people going through git log in the future.

If you really think splitting this up would be better, how about this?

1. Fix AsWritten for umbrella headers/directories.
2. Add tests to clang/test/Modules.

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.