Ping..

Ping.....

In D73852#1872013, @lebedev.ri wrote:

This patch also omitted cfe-commits lists.

Ping again. Is there still something more to do here?

Ping.

Updated according to comments.

Fix possible crash, cache regex.

(Removed unintended debug output in tests.)

I've found a corner case, as shown by clang/test/PCH/delayed-pch-instantiate.cpp, where it really is necessary to perform an instantiation in the TU. I've updated the patch to detect this and repeat these instantiations the way they would be without the patch.

Handled all tests which failed because the change moved templates in -emit-llvm output.
Added a test for specialization of a template instantiated in a PCH.

In D69585#1825133, @ABataev wrote:

I thought you were going to add an option or a flag to control the behavior? If so, just provide an option in tests to avoid triggering of the new behavior (except for declare_target... test and those 2 you modified already) and that's it.

Could you please still provide input in https://reviews.llvm.org/D69585? The patch still fails in 21 OpenMP tests simply because of compiler output getting reordered, which seems to require extensive reorganizing of the tests, and I don't want to do extensive (and tedious) changes there without your input.

In D69585#1821831, @aganea wrote:

What is the error?

In D72759#1823062, @ABataev wrote:

Must be fixed in b841b9e96e605bed5a1f9b846a07aae88c65ce02

In D72759#1821759, @ABataev wrote:

Fails in what way?

In order to simplify this, I've updated the patch to remove any reference to OpenMP and I've moved that part to https://reviews.llvm.org/D72759 .

This version uses a module based on the code posted above.

I've updated the test as requested.

In D69585#1818205, @rnk wrote:

I'm interested in making clang do this, but I think this needs significantly more work until this is ready to land. It needs in-tree tests.

Do you need some more information about the patch? It'd be nice if this could make it into 10.0.

Ping.

In D69779#1801771, @dblaikie wrote:

Ah, if I mark the standalone function template 'inline' (the implicit linkage of member functions) then I get the same failure for both. Haven't tested whether the fix is the same fix for both yet.

Closing, handled in D71654.

I'm back at using gdb for the time being, so I'd have to build code with debuginfo suitable for lldb, which would take some time. But given that this is more or less the same as I did before, I don't see why this shouldn't work similarly. Thank you for getting these changes in.

In D69778#1776472, @dblaikie wrote:

I guess one aspect is that -building-pch-with-obj seems like it duplicates the fmodules-codegen concept (both basically are a flag passed during pcm/pch build time that says "I promise to build an object file from this pcm/pch, so rely on that assumption when building other things that depend on the pcm/pch) - if I'd noticed the -building-pch-with-obj going in I would've made the point then.

In D69778#1771799, @rsmith wrote:

It's a bit weird for this to be controlled by a -fmodules flag, but it's only a -cc1 flag, so I'm OK with that; we can rename it if/when we expose it from the driver.

Ping.

Ping.

Ping.

It turns out that this patch may introduce unwanted changes, specifically it can cause err_specialization_after_instantiation if the specialization is done in a source file but needed already by code in the PCH. But this seems to be rare (I've encountered it only after building the Skia library with put-everything-in-the-PCH), is easy to avoid (PCHs often need little tweaks anyway) and the performance gain is very much worth it.

In D68909#1736931, @kwk wrote:

Are there no tests for this code?

Let's go a different route. This patch fully passes all tests and so should be ready to be committed.

In D68541#1710489, @llunak wrote:

Can you please ACK also the other patches linked above that this one depends on? Or can I take the discussion here as being enough?

Ping?

Handled by https://reviews.llvm.org/D42303 .

Due to some technical problems with this approach and lack of feedback, I'm scratching this one. A new approach is at https://reviews.llvm.org/D69585 .

In D68549#1710986, @teemperor wrote:

We could also just let the allocator take a parameter so that is increases the growth size to do it every Nth slab (N=1 for us) and set a slightly larger starting size.

Thinking more about this, maybe this is really not the right place and the change should be done in BumpPtrAllocator? To me it seems rather unreasonable that it would double the allocation size only after 128 allocations. Surely by the time it has already allocated 128*4096=0.5MiB memory it could have decided to raise the allocation size much sooner?

Oh, I somehow completely missed the fact that there are 256 of those :-/. In that case my numbers are way too much. Can you easily benchmark with different numbers? I think something like 131072 for BumpPtrAllocator (to still be large enough for the mmap threshold) could be reasonable, the size for StringPool can go down to 256 or maybe can be simply dropped.

In D68541#1710031, @clayborg wrote:

Looks good!

In D68910#1710147, @hhb wrote:

This is fine for me. Actually it doesn't make any difference for all platforms I tried... Out of curiosity, is it possible to share the implementation of get_python_lib() in openSUSE? Thanks.

Adjusted the testcase.

How about this one?

In D68910#1709832, @hhb wrote:

Can you sync to the latest code and try again? Your problem is likely to be fixed in a previous change...

Removed the explicit test timeout.

Updated misleading description.

Added a unittest.

In D68541#1700359, @clayborg wrote:

It would be fine to remove the 'd' shortcut for detach and resume if we need to by moving this functionality up into the menu bar. Right now the Process menu has "Detach" only, so it might be good to first change the "Process" menu to have "Detach" or "Detach suspended". I agree that accidentally hitting 'd' would be bad.

In D68541#1698127, @JDevlieghere wrote:

Would it be possible to add a test for this? Maybe you can extend TestGuiBasic.py.

In D68549#1700019, @aprantl wrote:

This makes sense to me. We may need to make this configurable for low-memory hosts at some point.

Increased default number of buckets for StringMap too.

In D63508#1671776, @torarnv wrote:

So, this will make -frewrite-includes do more work, to ensure that it not only covers the "top level" #include or __has_include case, but also __has_include in one or more levels of macros?

Does that effectively means it needs to do full preprocessing?

I've noticed that the Developer Policy says that it's actually allowed to commit patches without approval for parts "that you have contributed or maintain". Given that I'm the author of -rewrite-includes I take it that it's ok if I commit this if there are no further comments.

• updated to apply to current trunk
• changed misleading condition in a test