In D77321#1958052, @arphaman wrote:

LGTM.

In D76594#1957452, @DmitryPolukhin wrote:

@rsmith, @dexonsmith, @jdoerfert could you please take a look to this diff?
If you think that there are other reviewers who might have more context AST persistence, please add them.

In D77058#1950019, @tejohnson wrote:

I think this is a good approach, rather than a per-function attribute, since as mentioned this will be preserved through inlining.
@dexonsmith, does that seem reasonable to you? I missed the original patch and agree with you that we don't want to fix this in LTO by passing the option through to LTO.

@fhahn, please revert, this isn't how we usually pass options in LTO.

In D76290#1930134, @yln wrote:

I don't understand yet what this enables. Please extend your commit message to elaborate a bit more for people (like me) who aren't familiar with libcxx testing.
Does this enable deriving new tests from existing source files or running existing tests (.{pass,fail,sh}.cpp) in a different way without needing to modify those dozens/hundreds of files?

The change itself looks innocent enough and I don't want to block you on adding a test if there are currently none at all. LGTM.

It would also be good to confirm that this isn't expensive in the normal case (one-level substitutions). Have you timed running the libc++ tests with and without this patch?

This looks really cool. Can you add tests for this feature, somewhere inside llvm/utils/lit/tests?

This SGTM. Ideally we'd confirm the compile-time explosion is fixed before landing this though.

In D72068#1906074, @ekatz wrote:

In D72068#1906010, @dexonsmith wrote:

Given the contention around direction, I would suggest that you seek consensus via an RFC on llvm-dev before pinging the patch any further.

It is the first thing that I did, but I was asked to post the patch for review. I agree that there should have been more discussion in the mail, but it seems stranger (and with not much response) to ping a mail thread.
Please refer to http://lists.llvm.org/pipermail/llvm-dev/2020-January/138710.html for further details.

In D72068#1905933, @ekatz wrote:

In D72068#1889825, @rnk wrote:

I'm still basically not in favor of this. If another contributor thinks it's a great idea, I wouldn't go so far as to block this change, but I don't plan to do any further review.

Fair enough, I respect your opinion.
However, I still think it is a very welcoming change (for the many reasons listed in the "summary"), and I hope other reviewers will see the benefits as I see them, and approve this change.

In D75395#1901859, @Bigcheese wrote:

In D75395#1901778, @dexonsmith wrote:

This is used when
converting an implicit build to an explicit build to match the
systemness the implicit build would have had for a given module.

I had another thought. What if for the explicitly built "system" modules:

• If -Wsystem-headers is on, leave them as user modules in the explicit build.
• If -Wsystem-headers is off, turn off all diagnostics in the explicit build.
  
  Does that give the right semantics, or is there something subtly different?

I considered this, but decided against it because I wanted the implicit and explicit builds to be as similar as possible, and reduce the amount of changes made to the original command line. There's a lot of code in Clang dealing with system files, and I'm not 100% sure that -Wno-everything would be equivalent.

This is used when
converting an implicit build to an explicit build to match the
systemness the implicit build would have had for a given module.

Okay, LGTM then, as long as...

In D75213#1898320, @aprantl wrote:

I'm inclined to try to commit this and see if we get any "describes" assertion reports.

If you're worried about LTO, I suggest trying a clang bootstrap with each of -flto=full and -flto=thin. I was using the former as a testcase when I was doing this work, so I think we'd get good signal from that.

LGTM to me too. With @steven_wu and me on board, I don't think you need to wait for @arphaman. Thanks for the fix!

@thegameg , tests are usually done through the llvm-lto or llvm-lto2 tools.

In D74984#1889605, @aprantl wrote:

Is there any kind of "These are the ADT types in LLVM and when to use them" .rst document that this should be added to?

Thanks! LGTM. Don't know how I missed that API...

In D69471#1883912, @rnk wrote:

Everything is off-by-one because the empty bases are not zero sized. The MSVC record layout algorithm is just different in this area. =/

Thanks for working on this, I agree it's a really important problem.

Thanks for working on this! I have a couple of comments inline.

Code change looks correct to me. Thanks for the fix!

In D51664#1877029, @rnk wrote:

I still think we should do this. I think @fhahn is reimplementing DSE using MemorySSA, so presumably the DSE cases won't be an issue soon, but setting all that aside, I still think it would be nice if we could say once and for all that Instruction::dominates(Instruction*) is amortized O(1) if you haven't modified the instruction stream. Otherwise this kind of pathology will pop up again. Putting the ordering on the IR saves clients from ferrying around and maintaining OrderedInstructions / OrderedBasicBlock data structures, and that seems like a win.

In D74341#1867559, @ldionne wrote:

Specifically, I'm looking to understand why Eric's comment at that time (https://reviews.llvm.org/D27429#618296) about using CLOCK_MONOTONIC_RAW wasn't acted upon -- did we simply overlook it or is there a reason why CLOCK_MONOTONIC_RAW is not a suitable implementation?

LGTM. Thanks for the fix!

LGTM, with one style nitpick.

In D73208#1837909, @MadCoder wrote:

In D73208#1836789, @dexonsmith wrote:

I think we're talking across each other. The idea is check the type when generating the implementation; if it's not correct, you fix it (need to update existing uses to bitcast). So the type used for the implementation would match dynamic methods.

ah I see, I don't know how to do that, I couldn't figure out how to fix the function declaration, if you want to give it a stab I'd love it.

Okay, LGTM.

Thanks, committed in 1e487e4c16821b6de3d651f618274df90bd3fad9.

In D67678#1836922, @rsmith wrote:

In D67678#1836668, @dexonsmith wrote:

In D67678#1836628, @steven_wu wrote:

In D67678#1834957, @rsmith wrote:

In D67678#1834542, @steven_wu wrote:

@rsmith This also breaks macOS SDK. Can we revert this as we discuss a less aggressive option?

Do you have a timeline for how long it would take to fix the MacOS SDK? Is this something we could realistically aim to do in Clang 11 instead? I would really prefer for us to not have different defaults for Darwin versus everywhere else, so if waiting one release cycle is enough, that seems fine.

I am also not sure if we have rules about SDK compatibility for releases and I can't say macOS SDK can definitely be fixed before clang 11 release. @dexonsmith @arphaman might have more info.

We can't comment on future releases.

That said, if this is urgent, let @arphaman know and we can try to qualify internally sometime in the next few months and we can report back what issues we find.

Understood. The current situation is deeply unpalatable (the conversions we allow by default that this patch would have disabled are *evil* and have never been supported by GCC; it looks like Clang only ever allowed them as a bug, and we really need to turn them off by default for safety as much as for GCC compatibility). But this isn't a regression, at least, so I don't think this is especially urgent. (I was working on this because Agner Fog made an impassioned plea that we fix this, and I think our community owes Agner a favor or two...)

In D73208#1836722, @MadCoder wrote:

In D73208#1836704, @dexonsmith wrote:

In D73208#1835264, @MadCoder wrote:

In D73208#1835051, @dexonsmith wrote:

Why isn't a similar dance needed for non-direct methods?

because non direct methods do not need an llvm::Function to be synthesized at the call-site. direct methods do, and they form one with the type of the declaration they see. Then that same llvm::Function is used when you CodeGen the Implementation, so if there's a mismatch, sadness ensues because the LLVM IR verifier will notice the discrepancy between the declared return type of the function and the actual types coming out of the ret codepaths.

Regular obj-C methods use the _implementation_ types for the codegen (the declaration(s) aren't even consulted) and I want to stick at what obj-c does as much as I can.

(as a data point: If you use obj-C types with C functions, the type of the first declaration seen is used instead).

Okay, that makes sense to me.

Another solution would be to change IRGen for the implementation: if the declaration already exists (getFunction), do a bitcast + RAUW dance to fix it up (and update the DirectMethodDefinitions table). WDYT?

I didn't want to do that because that would mean that the type used for the implementation would depart from dynamic Objective-C methods, and it feels that it shouldn't. hence I took this option.

In D73208#1835264, @MadCoder wrote:

In D73208#1835051, @dexonsmith wrote:

Why isn't a similar dance needed for non-direct methods?

because non direct methods do not need an llvm::Function to be synthesized at the call-site. direct methods do, and they form one with the type of the declaration they see. Then that same llvm::Function is used when you CodeGen the Implementation, so if there's a mismatch, sadness ensues because the LLVM IR verifier will notice the discrepancy between the declared return type of the function and the actual types coming out of the ret codepaths.

Regular obj-C methods use the _implementation_ types for the codegen (the declaration(s) aren't even consulted) and I want to stick at what obj-c does as much as I can.

(as a data point: If you use obj-C types with C functions, the type of the first declaration seen is used instead).

In D67678#1836628, @steven_wu wrote:

In D67678#1834957, @rsmith wrote:

In D67678#1834542, @steven_wu wrote:

@rsmith This also breaks macOS SDK. Can we revert this as we discuss a less aggressive option?

Do you have a timeline for how long it would take to fix the MacOS SDK? Is this something we could realistically aim to do in Clang 11 instead? I would really prefer for us to not have different defaults for Darwin versus everywhere else, so if waiting one release cycle is enough, that seems fine.

I am also not sure if we have rules about SDK compatibility for releases and I can't say macOS SDK can definitely be fixed before clang 11 release. @dexonsmith @arphaman might have more info.

LGTM.

Why isn't a similar dance needed for non-direct methods?

This LGTM too, but like John I would prefer a non-_OBJC_ name for the macro since -fblocks also gets used with C/C++.

LGTM.

In D72399#1822902, @rnk wrote:

In D72399#1821475, @russell.gallop wrote:

Honestly, if __thread is available, I wonder if LLVM should prefer that, since it implements the check that the variable is trivially constructible/destructible. But, that has no bearing on which declspec to use.

Is "trivially constructible/destructible" actually a requirement for this? I thought that restriction was due to host platforms which didn't support C++11 thread_local (hence if/when every platform supported C++11 thread_local then the restriction would go away).

Yes, LLVM still supports platforms which do not support C++11 thread_local, so for LLVM, there is still a requirement that things be trivially constructible/destructible. If we used __thread when available, the compiler would enforce that check earlier, rather than letting the contributor commit, push, and find out about this requirement from a buildbot.

In D71219#1776085, @rjmccall wrote:

Yeah, the discussion Jin and I had privately was that we should split the Swift ABI version out of the ObjC GC global metadata and update the backend so that it assembles the image info appropriately from different places. I don't know what happened to that.

That will probably require an IR updater, but that shouldn't be a problem.

In D69825#1771300, @arphaman wrote:

I guess Duncan's comments on https://reviews.llvm.org/D70568 were for this patch. I think it would be good to have:

In D70568#1768138, @dexonsmith wrote:

In D70568#1768129, @dexonsmith wrote:

In D70568#1768129, @dexonsmith wrote:

Can we make this configurable somehow? (E.g., leave behind an -ffork-cc1 -fno-fork-cc1 and a CMake flag to pick? Or just the CMake flag?)

This is really cool; we've wanted this for a long time.

LGTM, although I have a suggested change for one of the comments (and I'm still sad about this).

Pushed as 20d51b2f14ac4488f684f8fc57cb0ba718a6b91d.

Pushed as 20d51b2f14ac4488f684f8fc57cb0ba718a6b91d.

For some reason this revision is locked down and I'm not allowed to "edit" it, which includes adding inline review comments. Can you add me as a reviewer?

In D70285#1752361, @dexonsmith wrote:

In D70285#1752164, @Bigcheese wrote:

lgtm as long as other compilers don't warn on unknown pragmas by default.

godbolt.org says that GCC doesn't have -Wunknown-pragmas by default, but it is in -Wall. I'd better guard this more closely with __clang__.

In D70285#1752164, @Bigcheese wrote:

lgtm as long as other compilers don't warn on unknown pragmas by default.

Pushed as 3279724905c14a8db383ade53af40a0dd49504d8.

@rjmccall gave me an LGTM offline.

In D70370#1750591, @dexonsmith wrote:

In D70370#1750378, @ahatanak wrote:

It wasn't clear to me, but is the pair deleted only if there are no instructions that return false for isSafeBetweenRVCalls in between?

I thought it was okay to skip over all opaque calls, but I've changed the code to check for this.

Respond to Akira's feedback.