This looks reasonable to me in the sense that it treats distinct metadata as being different but bypasses distinct debug info. CC @dexonsmith

This commit seems to cause some regression for "-save-temps" as there is no new line before the pragma. See the below test case, -E will output
int test();#pragma clang assume_nonnull
It will fail the compilation on the preprocessed output with
error: expected unqualified-id
int test();#pragma clang assume_nonnull end

^

@dexonsmith @bruno: are you okay with this change? It looks good to me :]

This looks good to me in general. Since it should not change functionality, it may not be possible to write a test case.

In D57463#2823600, @thakis wrote:

is there any documentation on how to use this? I see that clang has a -forder-file-instrumentation switch to enable this. But what do I do after I enabled it? Run the instrumented program and then…? From the name, I'm guessing maybe the idea is that this will produce an order file?

In D86049#2255738, @rjmccall wrote:

We just talk about it. I agree with Nathan that we shouldn't just add this as a short-term hack; we should design the ABI right and then do what we want.

I think these are basically all the ABI questions:

• Is there a good reason to preserve signature compatibility with normal ObjC methods, or should we just drop the selector argument on direct methods?
• Should we do the null test on the caller side or the callee side?
• In direct class methods, is the caller's responsibility or the callee's to ensure that self is initialized?

For the first, I think dropping the argument is fine.

Dropping the argument sounds good to me! We are already passing the argument as undefined.

For the second, I'm less certain.

It feels cleaner to do the null test on the callee side (i.e inside the wrapper). It may have binary size advantage compared to checking at each callsite.

For the third, making it the callee's responsibility is generally better for code size but harder to optimize. But we only have the code-size cost on the caller side when materializing from a global, and that's actually something we can also eliminate as redundant. So maybe the best approach is to introduce a weak+hidden thunk that we use when making a call to a direct class method on a specific class, and we have that thunk do the entire materialization sequence.

Completely agree. For direct class method, making sure 'self' is initialized on the callee side is cleaner and better for code size. But caller has the context to know whether the class is already initialized.
Maybe we can start with doing it on callee's side and make an incremental improvement by implementing the approach that can get both benefits.

In D75574#1925808, @rjmccall wrote:

This might also be interesting to @manmanren.

There are a few concerns about the approach:
1> Compile time: dumping AST as string then hashing the string. Alex measured it on a synthetic benchmark, it shows insignificant impact.
2> Stability: from my understanding, the main goal of this patch is to increase stability of the symbol name so it will not change if the relevant code for the block is not changing. It may not be as stable when the compiler version changes.
Using per-function ID improves the stability compared to per-module ID. @alexbdv do you have rough ideas on how much better the proposed approach is in terms of stability, comparing to per-function ID?
3> Using a compiler flag may slow down the adoption.

We should add a new module flag (with type Error) that contains Major and Minor and make sure "Garbage Collection" value is the same for Swift as for ObjC.

In the backend, we will reconstruct IMAGE_INFO from the new module flags and make sure the value stays the same with and without this patch.

IMAGE_INFO will be different for Swift vs ObjC.

Should be question mark! Meant for clarification from @steven_wu and @rjmccall. Should IMAGE_INFO include the value from the new module flag?

Just to clarify :]

Thanks!

r355333

Mark windows as unsupported in the testing case.

Thanks!

Address review comments!

Rebase

Add a testing case to make sure the instrumentation works correctly!

Hi David,

I think I have addressed most of the comments. There is one comment which I am not sure.

Small Nits

Address some review comments

Add support for the new pass manager

I am not sure if this is the best approach, but the implementation looks okay to me. @bruno @rsmith What do you think?

Please update the patch with context: http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Is it possible to add a testing case?

LGTM. Sorry for the delay,

Ping :]
Hoping to wrap this up this week.
Cheers,
Manman

Ping :]
I am hoping to wrap this up this week. Thanks,
Manman

LGTM

LGTM

Add CC1 option to control hashing of the module file content.

Bruno helped on collecting performance data for this patch:
$ cat cocoa-test.h
#import <Cocoa/Cocoa.h>

Addressing review comments.

Rebase on top of r291686.

Add testing case.

I forgot to upload the testing case, I will add it now.

LGTM.

Addressing review comments!

Thanks for working on this, Sanjoy!

Thanks,
Manman

LGTM.

Thanks,

Thanks for the work!

LGTM

LGTM .

LGTM

The rest looks pretty reasonable.

In particular, I think once D26438 is checked in (I'm also waiting for you to take a look at that one, btw :) )

I will take a look at that soon :)

Cheers,

Cheers,
Manman

In D25916#594844, @benlangmuir wrote:

Does it mean that a system module should only import system modules? If a system module is allowed to import non-system modules, for a non-system module, we will validate diagnostic options differently depending on whether a system module or a non-system module imports it. This will cause a non-system module that was validated earlier to be invalidated by a child thread.

It seems like we should validate the options the same way regardless of what the importer is, but I'm guessing this was done for a reason... What's the behaviour of a user-header imported by a system header (without modules)? If the user header warnings show up even without -Wsystem-headers, then we should be okay validating, right?

We are hitting this issue when building large projects, but the reproducibility is quite low.

In D26438#593369, @sanjoy wrote:

Hi all (esp. @manmanren ),

Are you happy with the general direction here? If so, I'll start writing some FileCheck unit tests (I don't want to sink time into tests if there we are likely going to see major corrections here).

In D26438#591415, @sanjoy wrote:

In D26438#591414, @manmanren wrote:

The tag for the load instruction is !2 whose access type is !7 == !{!"float", !5, i64 0}.

!7 is a scalar type node, it is a float, right?

Perhaps we are talking past each other, but are you concluding that it is scalar by looking at its name? I thought the names themselves were not supposed to be significant within the TBAA type system.

Structurally, it is exactly a struct that with a single char field. If I compile

struct S {
     int i;
     float f;
};

struct S2 {
     char c;
};

float f(struct S* s, struct S2* s2) {
     return s->f + s2->c;
}

then I get:

!0 = !{i32 1, !"PIC Level", i32 2}
!1 = !{!"clang version 4.0.0 (http://llvm.org/git/clang.git b74377b0a34a33cb3c7b565f0e543cd3e56b3f6b) (llvm/trunk 286280)"}
!2 = !{!3, !7, i64 4}
!3 = !{!"S", !4, i64 0, !7, i64 4}
!4 = !{!"int", !5, i64 0}
!5 = !{!"omnipotent char", !6, i64 0}
!6 = !{!"Simple C/C++ TBAA"}
!7 = !{!"float", !5, i64 0}
!8 = !{!9, !5, i64 0}
!9 = !{!"S2", !5, i64 0}

with !2 as the access tag for s->f and !8 as the access tag for s2->c. !2 has !7 as its access type (which means it must be a scalar type), which is indistinguishable from the struct type node !9, except from its name. Without a priori knowledge of what "float" and "S2" mean, we can't conclude one way or the other.

In D26438#591404, @sanjoy wrote:

In D26438#591399, @manmanren wrote:

There is this bit of language in the TBAA implementation:

// The second field is the access type node, it
// must be a scalar type node.

but in the TBAA metadata generated by clang it seems like we have access types like !{!"int", !N, i64 0} which look like struct nodes.

Can you give an example where clang generates a struct path tag node where the 2nd field is not a scalar type node?

That seems to be the case everywhere. E.g.

struct S {
     int i;
     float f;
};

float f(struct S* s) {
     return s->f;
}

compiles to

; ModuleID = 'x.c'
source_filename = "x.c"
target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-apple-macosx10.12.0"

%struct.S = type { i32, float }

; Function Attrs: norecurse nounwind readonly ssp uwtable
define float @f(%struct.S* nocapture readonly %s) local_unnamed_addr #0 {
entry:
  %f = getelementptr inbounds %struct.S, %struct.S* %s, i64 0, i32 1
  %0 = load float, float* %f, align 4, !tbaa !2
  ret float %0
}

attributes #0 = { ... }

!llvm.module.flags = !{!0}
!llvm.ident = !{!1}

!0 = !{i32 1, !"PIC Level", i32 2}
!1 = !{!"clang version 4.0.0 (http://llvm.org/git/clang.git b74377b0a34a33cb3c7b565f0e543cd3e56b3f6b) (llvm/trunk 286280)"}
!2 = !{!3, !7, i64 4}
!3 = !{!"S", !4, i64 0, !7, i64 4}
!4 = !{!"int", !5, i64 0}
!5 = !{!"omnipotent char", !6, i64 0}
!6 = !{!"Simple C/C++ TBAA"}
!7 = !{!"float", !5, i64 0}

The tag for the load instruction is !2 whose access type is !7 == !{!"float", !5, i64 0}.

In D26438#591405, @sanjoy wrote:

In D26438#591400, @manmanren wrote:

In D26438#591227, @sanjoy wrote:

In D26438#591194, @mehdi_amini wrote:

When did we stop using the "old" form? We don't need to preserve an auto-upgrade forever, as long as we can simply drop metadata for old bitcodes this is as valid alternative.

We've had the auto upgrade logic for what looks like approximately 3 years. I think it is reasonable to deprecate the functionality and eventually remove it, though I know (from off-list emails) that it will break (in terms of performance, not correctness) downstream users.

Can you explain more on why it will break downstream users in terms of performance?

I meant if we go by Mehdi's strategy of dropping old style tbaa metadata instead of auto-upgrading them then downstream users who generate the old style tbaa metadata will no longer get the benefit of TBAA. Their code will be correctly compiled, but will run slower.

In D26438#591227, @sanjoy wrote:

In D26438#591194, @mehdi_amini wrote:

When did we stop using the "old" form? We don't need to preserve an auto-upgrade forever, as long as we can simply drop metadata for old bitcodes this is as valid alternative.

We've had the auto upgrade logic for what looks like approximately 3 years. I think it is reasonable to deprecate the functionality and eventually remove it, though I know (from off-list emails) that it will break (in terms of performance, not correctness) downstream users.

In D26438#591021, @sanjoy wrote:

Hi Manman,

In D26438#590834, @sanjoy wrote:

I may have misunderstood your point, but for struct-path TBAA, the last integer for a type node is the offset. Only for old scalar TBAA, the last optional integer specifies whether it is immutable. There is no ambiguity in struct-path TBAA.

Thanks, that clears it up for me.

There is this bit of language in the TBAA implementation:

// The second field is the access type node, it
// must be a scalar type node.

but in the TBAA metadata generated by clang it seems like we have access types like !{!"int", !N, i64 0} which look like struct nodes.

Can you give an example where clang generates a struct path tag node where the 2nd field is not a scalar type node?