- User Since
- Jan 18 2013, 11:30 AM (312 w, 4 d)
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?
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.
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.
Mon, Jan 14
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.
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.
Thu, Jan 10
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.
Wed, Jan 9
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.
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.
Mon, Jan 7
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.
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. :)
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.
Sun, Jan 6
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?
Fri, Jan 4
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.
Thu, Jan 3
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.
Wed, Jan 2
...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.
Mon, Dec 31
Fri, Dec 21
It sounds like it's fine.
Thu, Dec 20
Okay. That's also presumably true for open-source runtimes that support ARC; tagging David Chisnall and Jonathan Schleifer to get their input.
Wed, Dec 19
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.
Tue, Dec 18
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?
Thanks, looks great.
Mon, Dec 17
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.
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.