In D56411#1352602, @yaxunl wrote:

In D56411#1352332, @rjmccall wrote:

This patch still doesn't make any sense. You don't need to do any special validation when passing a function as a template argument. When Sema instantiates the template definition, it'll rebuild the expressions that refer to the template parameter, which will trigger the normal checking for whether those expressions are illegally referencing a host function from the device, etc. All you need to do is suppress that checking (whether it happens in a template definition or not) for references from non-potentially-evaluated contexts.

If you look at line 6583 of lib/Sema/SemaTemplate.cpp, you will see clang does the check if the function needs overloading resolution. However, clang missed the check if the function does not need overloading resolution. That's why I need to add the check at line 6593. All the other stuff is just to help make this check.

why clang does not do the reference check when there is no overloading resolution?

It's an interesting idea at least, and it does seem like there ought to be some way to express it, at least internally. A -cc1 option is fine for your purposes, right?

Thanks, LGTM.

I think that's the right direction, yeah.

Okay, so is this ready to re-review independently?

One nit-pick, then this is good to go.

In D53738#1333372, @rjmccall wrote:

In D53738#1333276, @leonardchan wrote:

In D53738#1326071, @rjmccall wrote:

I'm fine with making this change under the assumption that we've gotten the language rule right. Even if that weren't abstractly reasonable for general language work — and I do think it's reasonable when we have a good-faith question about the right semantics — this is clearly still an experimental implementation and will be for several months yet, and hopefully it won't take that long for us to get a response.

@rjmccall Have you received a response yet? If not, do you think you have an estimate on the response time, or also mind sharing the contact information if that's ok?

I just have a coworker who's part of the committee. I think you might be over-opimistic about how quickly things get answered with the C committee, though. We should not allow our work to be blocked waiting for a response.

Is there a reason not to just do T1.getQualifiers() == T2.getQualifiers()?

Yeah, I would recommend splitting the APFixedPoint in APValue changes into a separate patch.

If I'm debugging a clang bug and calling D->dump(), I think it'll be just as clear as it used to be what the ObjCTypeParamDecl lines mean.

This is the AST dumper. This is not a user feature.

I don't really have an opinion about this, sorry.

Mostly, we don't really want the abstract visibility calculation to change whenever we see a definition.

In D53153#1353256, @scott.linder wrote:

In D53153#1317977, @rjmccall wrote:

I think -fvisibility=hidden isn't good enough because you want to infer hidden visibility even for external symbol references, and that's just not how global visibility works. But after this discussion, I'm prepared to accept that (1) we should have some sort of single-image compiler mode that implies that all symbols are defined within the image and (2) you can make your toolchain imply that together with -fvisibility=hidden and then have specific symbols opt in to non-hidden visibility if they need to be accessible to whatever loader / runtime you have.

It seems that explicit visibility attributes on external symbol references (e.g. __attribute__((visibility("hidden"))) extern int foo;) are respected in Clang, so I don't understand the rationale for global visibility controls not applying to them as well. Can you describe why this is the case?

It's pretty unfortunate that all these fields have to be individually called out like this. Can you move all these basic layout fields into a separate struct (which can be a secondary base class of TargetInfo) which can then just be normally copied? Anything that needs special copy semantics, like the LLVM DataLayout (do you need to copy this?) doesn't need to go into that struct, just the basic POD things that determine fundamental type layouts and semantics.

Great, LGTM.

In D56318#1353116, @yaxunl wrote:

In D56318#1353106, @rjmccall wrote:

No, I understand that things like the function-call ABI should be different from the associated host ABI, but things like the size of long and the bit-field layout algorithm presumably shouldn't be, and that's the sort of thing that's configured by TargetInfo.

How about create a ForwardingTargegInfo which will has a pointer to AuxTarget and forward to that target if it is not null. Then let AMDGPUTargetInfo inherit from that.

No, I understand that things like the function-call ABI should be different from the associated host ABI, but things like the size of long and the bit-field layout algorithm presumably shouldn't be, and that's the sort of thing that's configured by TargetInfo.

If I was only concerned about size_t, your current solution would be fine. My concern is that you really need to match *all* of the associated CPU target's ABI choices, so your target really ought to be forwarding everything to that target by default and only selectively overriding it in order to support GPU-specific features. Probably the easiest way to do that is via inheritance.

This patch still doesn't make any sense. You don't need to do any special validation when passing a function as a template argument. When Sema instantiates the template definition, it'll rebuild the expressions that refer to the template parameter, which will trigger the normal checking for whether those expressions are illegally referencing a host function from the device, etc. All you need to do is suppress that checking (whether it happens in a template definition or not) for references from non-potentially-evaluated contexts.

Sounds good.

Don't worry, that's a familiar mistake. :)

But why? Why do you want to limit this to just template arguments instead of all sorts of similar contexts?

I really dislike this particular idiom.

Okay. In my opinion, then, we should just do this unconditionally.

Sema won't necessarily have resolved a template decl when parsing a template argument list, so trying to propagate that decl down to indicate that we're resolving a template argument is not a good approach.

Oh, and please update the commit message to primarily talk about the changes to placeholder checking. You can discuss the impact on the repeated-use-of-weak warning in a follow-up paragraph.

Yeah, LGTM.

Thanks! LGTM.

If it's at all reasonable to just avoid doing the work multiple times, let's do that. It should also avoid duplicate diagnostics if e.g. an overload is resolved to an unavailable function.

Okay. That comment seems reasonable. Glad to hear you're on top of it. :)

LGTM.

Does the regression disappear if you make the scope specifier the first trailing object? Later trailing objects are more expensive to access, and I would imagine that the scope specifier and template arguments are more important than the other fields.

Can we find a way to preserve the rewritten expression out of DeduceAutoType so that the initialization code just happens to never see a placeholder along this path?

Okay. Is there a reasonable way to make your targets delegate to a different TargetInfo implementation for most things so that you can generally match the host target for things like type sizes and alignments?

No, no, I understand that you're not changing pointer sizes, but this is one example of trying to match the ABI of the target environment, and I'm trying to understand how far that goes. What does it mean to be in the "MSVC" environment when you're actually just compiling for the GPU? Why are you using OS headers in the first place? Do you need struct layout to match MSVC (presumably on x86-64)? What should happen with the C++ ABI?

What's the general idea here, that you're going to pretend to be the environment's "standard" CPU target of the right pointer width and try to match the ABI exactly? This seems like a pretty treacherous road to go down.

In D55662#1346059, @ahatanak wrote:

In D55662#1346055, @rjmccall wrote:

I think you could just disable the diagnostic in an unevaluated context.

The call to isUnevaluatedContext in ObjCPropertyOpBuilder::complete returns false when the type of auto __weak wp in testAuto is being deduced because the ExpressionEvaluationContextRecord on the stack isn't an unevaluated context. I can silence the warning if I can push an unevaluated context at the entry of Sema::DeduceAutoType, but it's not clear to me that it's safe to do so.

I think you could just disable the diagnostic in an unevaluated context.

Sorry, I keep coming up with small tweaks to the documentation. These should be the last of them, so if Aaron's also satisfied, feel free to commit with those modifications.

I don't mind you committing it like this.

Thanks, LGTM.

...although it might be reasonable to extract the method implementations on DefaultABIInfo as helper functions so that the code can be reused without requiring a particular inheritance hierarchy.

LGTM.

LGTM.

LGTM.

It sounds like it's fine.

In D55662#1338766, @ahatanak wrote:

In D55662#1337141, @rjmccall wrote:

In D55662#1336835, @ahatanak wrote:

In D55662#1335773, @rjmccall wrote:

Okay. You may need to push an unevaluated context when doing that.

Since I'm just moving the call to CheckPlaceholderExpr to the call site, I don't think I have to push an unevaluated context there?

Hmm. Right, for the auto inference specifically it's fine because the expression is in fact evaluated: we're not just eliminating placeholders in order to resolve decltype, we're eliminating placeholders to actually figure out what's going on with the initialization.

clang currently diagnose the repeated use of weak in the following case (with or without this patch):

auto __weak wp = b.weakProp;

I find this counterintuitive, but I guess this is the expected behavior?

In D55869#1338003, @theraven wrote:

This should be fine for the GNUstep runtime (the GCC runtime doesn't support ARC at all). My main concern is that it will break already-released versions of the runtime built with a newer version of clang. I can easily enable a new flag in the next release, but doing so for older ones is more problematic.

In D55869#1337733, @pete wrote:

In D55869#1337723, @js wrote:

In D55869#1337711, @dexonsmith wrote:

In D55869#1337706, @js wrote:

The ObjFW runtime itself does not contain anything about release, retain or autorelease - these are all part of ObjFW (as in the framework). As ObjFW also supports the Apple runtime, as well as mixing with Foundation code, it so far only provides objc_retain and objc_release when they are missing, to make ARC work. They just call into the corresponding methods on the object and do nothing else.

How will this change work from the Apple runtime? Will objc_retain/objc_release call the retain/release on the object if it implements its own? Should I drop my own retain/release implementation from my root object if I am compiling against the Apple runtime?

Yes, the idea is that the specialized runtime functions are fast paths for the common case, but they still fall back if the object has overridden them.

I am guessing not just overridden, but also a root class providing one? IOW, the fast path is used when the class does not have the retain or release method at all? In that case, LGTM as is.

The Apple runtime is using a bit on each realized class to track the overriding. The root class defaults to not having a custom RR, ie, it appears that its version of RR is considered equivalent to objc_retain.

Presumably that would apply to other root classes too, although i'm not 100% sure. I did see some special handling of NSObject in there too so perhaps only its RR is equivalent to objc_retain/objc_release.

In D55869#1337706, @js wrote:

Thanks for tagging me!

The ObjFW runtime itself does not contain anything about release, retain or autorelease - these are all part of ObjFW (as in the framework). As ObjFW also supports the Apple runtime, as well as mixing with Foundation code, it so far only provides objc_retain and objc_release when they are missing, to make ARC work. They just call into the corresponding methods on the object and do nothing else.

How will this change work from the Apple runtime? Will objc_retain/objc_release call the retain/release on the object if it implements its own? Should I drop my own retain/release implementation from my root object if I am compiling against the Apple runtime?

Okay. That's also presumably true for open-source runtimes that support ARC; tagging David Chisnall and Jonathan Schleifer to get their input.

In D55662#1336835, @ahatanak wrote:

In D55662#1335773, @rjmccall wrote:

Okay. You may need to push an unevaluated context when doing that.

Since I'm just moving the call to CheckPlaceholderExpr to the call site, I don't think I have to push an unevaluated context there?

Thanks, LGTM. Somewhat surprised that we don't have an implicit copy of the Strong argument, but it's not a bad thing that we don't.

You're gating on OpenCL C++ in several places in this patch, but I don't think you need to. This extension should be available to anyone using address spaces and C++.

So, once upon a time this was a problem because we were rewriting the method calls in the runtime itself. Can you explain how that's not a problem now? Do we just expect the runtime to be compiled with the right switch?

In D55662#1335402, @ahatanak wrote:

Sorry, please ignore my previous comment. I was looking at the wrong place.

The following code reaches Sema::BuildDecltypeType without going through ActOnDecltypeExpression:

template <typename T>
void overloaded_fn(T);
decltype(auto) v5 = &overloaded_fn<int>;

Sema::BuildDecltypeType is called from Sema::DeduceAutoType, so calling CheckPlaceholderExpr there should fix the assert when the test case above is compiled.

In D55662#1335146, @ahatanak wrote:

Here are a couple of examples I found running the regression tests:

int f0(int);
float f0(float);
decltype(f0) f0_a; // this is not valid.

template <class... Ts>
int var_expr(Ts... ts);

template <class... Ts>
auto a_function(Ts... ts) -> decltype(var_expr(ts...));

template <class T>
using partial = decltype(a_function<int, T>);

int use_partial() { partial<char> n; }

template<class T> void oneT() { }
int main()
{
  decltype(oneT<int>)* fun = 0;
}

If the call to CheckPlaceholderExpr in Sema::BuildDecltypeType is moved to TreeTransform<Derived>::TransformDecltypeType and Parser::ParseDecltypeSpecifier, we can assert at the beginning of Sema::BuildDecltypeType.

Thanks, looks great.

You're making intrinsics with weak_external linkage? I feel like that's going to be unnecessarily awkward in the future, but okay.

I agree with not packing the argument count in.

I think the core question is whether there are any vendors who care about ABI stability for these attributes who don't care about the bugfix. Speaking for Apple, we would rather have the bugfix. Sony might have different thoughts; CC'ing Paul.

In D54862#1333700, @ebevhan wrote:

I'm also a bit confused about the semantics that this patch is applying to function types. It mostly seems to concern the extra trailing Qualifiers on CXXMethodDecl to store the addrspace quals, but in some places (SemaType:4842, SemaDecl:3198) it seems to be applying the address space to the function type itself, which certainly seems like something else to me. A function with an address space qualified type would (to me, at least) be a function *located* in that address space, not one qualified to take a this from that address space.

Thanks! I think the only real question here is whether the entry should be *skipped* if it isn't directly a global variable or if you should stripPointerCasts() to try to find the global.