In D66121#1673233, @aprantl wrote:

I'm afraid I'm going to give up on fixing the AST and return to my debuginfo-only patch.

While I was correct in figuring out that ObjCMethodDecl implementations are not linked up as redeclarations of ObjCMethodDecl decls in the interface, that wasn't the whole story: ObjCMethodDecl::getNextRedeclarationImpl() links them up *implicitly* by finding the implementation for a decl and vice versa by looking up the selector in the interface/implementation (See https://github.com/llvm/llvm-project/blob/244e738485445fa4b72bfef9b9b2f9625cee989e/clang/lib/AST/DeclObjC.cpp#L905).

I can make this mechanism work by adding the method to the ObjCImplementationDecl, so it gets found by getNextRedeclarationImpl(). But if I do that, CodeGen falls over completely, because the new property accessor redeclarations don't actually have any function bodies. CodeGen in several places special-cases property accessors to generate them on-the-fly. I think this may work neatly if we actually created an AST for the function body in SemaObjCProperty too, and remove the functionality from CodeGen, but that is beyond what I'm prepared to do.

LGTM, thank you.

In D67399#1669568, @jfb wrote:

In D67399#1669038, @dnsampaio wrote:

Indeed our main concern is regarding the access widths of loads. As mentioned by @rjmccall, most volatile bitfields are used to perform memory mapped I/O, and some hardware only support them with a specific access width.
The spurious load I am more than glad to leave it disable behind a command flag, so it will only appear if the user requests it. See that volatile accesses might have side effects, and for example, an I/O read counter holding an odd number could define that the data is still being processed.

Are the cases being addressed in the PR actually relevant to real MMIO, or is this patch following the letter of AAPCS which doesn't actually matter?

I have to say that I disagree. The ABI certainly doesn't get to control the language and define what constitutes a volatile access, but when the language has decided that a volatile access is actually performed, I think ABIs absolutely ought to define how they should be compiled. Volatile accesses are quite unusual in that they are used for a purpose — memory-mapped I/O — that is extremely dependent for correctness on the exact instructions used to implement it. IIRC there are architectures that for whatever reason provide two different load instructions where only one of those instructions actually performs memory-mapped I/O correctly. Certainly the exact width of load or store is critical for correctness. So while I have certainly seen ABI overreach before and pushed back on it, for this one case I think ABI involvement is totally appropriate, and in fact I wish more ABIs specified how they expect volatile accesses to be performed.

The exact sequence of volatile accesses is observable behavior, and it's the ABI's right to define correct behavior for compliant implementations, so we do need to do this.

Thanks! Some wording suggestions.

LGTM.

Right, that sounds reasonable. You should be making them link up exactly like a normal method implementation, so if SemaObjC doesn't consider normal method implementations to be redeclarations, then I guess these aren't either. And if we want to change that in the future, then we'll change it for these, too.

Hmm, you know, there are enough different FP options that I think we should probably split them all out into their own section in the manual instead of just listing them under "code generation". That will also give us an obvious place to describe the basic model, i.e. all the stuff about it mostly coming down to different strictness and exception models. Could you prepare a patch that *just* does that reorganization without adding any new features, and then we can add the new options on top of that?

I think this is a step in the right direction, thank you. I'd like @scanon to weigh in on the evolving design here.

Okay, thanks. LGTM.

LGTM

Could you give it a slightly more general name and then use it in the main semantic check in ActOnFields?

Can you prepare an NFC patch with just the changes relating to adding ObjCPropertyImplDecl::get{Getter,Setter}MethodDecl?

Okay, now that I understand the source-compatibility issues a little better, I think this approach is probably okay. If it causes trouble, we can consider special-casing these headers to treat the member as __unsafe_unretained implicitly — the special case isn't great, but it's better than the potential unsoundness.

In D65744#1652355, @Anastasia wrote:

I don't think this is likely to change. Are you suggesting to move the deduction logic for pointee of pointers, references and block pointers into ASTContext helper that creates a pointer/reference/block pointer type?

In D65744#1629055, @Anastasia wrote:

In D65744#1627589, @rjmccall wrote:

I see. Is the deduction rule recursive or something, where a pointer to pointer is inferred to point to the same address space as the pointee?

It is recursive indeed and we currently deduce all pointees to generic AS.

In D66230#1633348, @GorNishanov wrote:

In D66230#1632625, @rjmccall wrote:

aspects of frame layout being exposed in the ABI is a property of the switch lowering, not coroutines in general.

You may be right. Given that now we have two frontends targeting LLVM Coroutines, some refactoring may be in order. I need to study more the Swift approach before I can form an opinion.

Marking it as NoDuplicate in CoroEarly helps simplify the CoroSplit logic.

Does it? Why? There already has to be a single coro.id in the coroutine, and that's the intrinsic that takes useful information. coro.begin just has a bunch of requirements and no clear purpose except to return the frame, which could just as well be done with a duplicable intrinsic.

Here is how it looks to me: C++ frontends emits the following structure (simplified):

%id = coro.id(stuff)
%mem = SomeAllocCodeCouldBeAnything(stuff)
%frame = coro.begin(%mem)

coro.begin "blesses" the memory as the one to be used in the coroutine and therefore should dominate any possible uses of data that go into the coroutine frame and gives a convenient place to dump spills, copies, etc.

If we did not allow arbitrary allocation logic in c++, there would be no need for coro.begin at all.

In D66230#1632621, @GorNishanov wrote:

In D66230#1632510, @rjmccall wrote:

Is there a case you're imagining where the adjustment would have side-effects? Because I can see a reason to have an intrinsic that returns a frame pointer, but I don't see why that intrinsic would have any of the restrictions of @llvm.coro.begin.

Which restrictions are you thinking about?

Is there a case you're imagining where the adjustment would have side-effects? Because I can see a reason to have an intrinsic that returns a frame pointer, but I don't see why that intrinsic would have any of the restrictions of @llvm.coro.begin.

Seems reasonable to me. If we're no longer imposing any *restrictions* for @llvm.coro.begin, is it serving any real purpose? Is it just a way of getting a handle to the coroutine frame?

In rG038d604f4f8c0f02ef8b6e8740f1a6fc1538c193#679526, @xbolva00 wrote:

As a frontend author, I would like to be able to conveniently copy memory for a struct assignment in a single line of IR.

memmove? :) this needs new benchmarks

One minor request (which will be dead code in your patch), but otherwise LGTM.

I don't think it's generally the frontend's responsibility to decide things like whether to inline memcpy.

I agree with just special-casing this for now.

I see. Is the deduction rule recursive or something, where a pointer to pointer is inferred to point to the same address space as the pointee? I still don't understand why pointers and references are handled differently here, instead of having the rule be "don't infer if the type is opaquely dependent or an auto placeholder".

Isn't the general rule for template argument deduction (which this devolves to) just to ignore top-level qualifiers? And then you can substitute in the substituted type and end up with a properly qualified type for the parameter / variable, and you can add extra qualifiers as necessary. Why are special rules for pointers and references required?

Oh, sorry, I confused several patches, then.

Ah, yes, if this isn't just working on intrinsic functions then that makes total sense.

Have you looked through the attributes that can be written on @interfaces and verified that they're all sensible to write on a @class? It's not hard to imagine that *some* of them should be diagnosed when added to a non-definition.

LGTM, although I'm not generally familiar with the upgrader code.

Since this setting changes IR output, you should write a test that emits IR for source code and tests that you get the right IR output.

Looks good to me; I express no opinion about Kevin's question.

LGTM.

Oh, I see this review might be dead in the water; I'm not sure why I was just added as a reviewer to it (by a non-author, no less).

I doubt "CTAD" is going to survive as a term that people recognize and remember, and I don't think -Wclass-template-argument-deduction is outrageously wordy compared to some of our other diagnostic groups. With that change, this would be fine with me.

Hmm, sorry, that's a C++ spec, and it looks like the (abandoned) C attempt at data parallelism (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2081.htm) doesn't introduce vector types.

You might want to look into ISO/IEC TS 19570 - 2018 to see what they're thinking. If it looks like there's a potential conflict, then maybe these should have an OpenCL-specific prefix on them.

Hmm. I think there are two reasonable concerns here, both arising from the fact that these names occupy (the builtin namespace parallel to) a namespace that the committee might want to occupy in the future:

• Is a v2fN suffix sufficiently likely to avoid interfering with this namespace? If there's an *exact* collision, that might be okay if the builtin actually matches the standard behavior; my concern is that they might conflict. (Different orderings of element count and element type? Unrelated functions ending in v?)
• Is it a good idea to introduce builtins in this namespace that traffic in our current vector types instead of a potential future standard-blessed vector type?

LGTM

In N2405 (http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2405.pdf), these are called truncf16, cosf16, etc., which I think is appropriate.

LGTM

LGTM

LGTM, although I don't think it would be ridiculous to make this an inline function definition like isAnnotation. (I like your generated implementation much better than the sequential ifs used there, though.)

LGTM

In D65286#1606441, @Anastasia wrote:

In D65286#1606071, @mantognini wrote:

In vector_literals_nested.cl, we have tests for (global) constants. Do you think it would be worth testing those as well in C++ mode? Maybe the two files (vector_literals_nested.cl and vector_literals_valid.cl) should also be merged as most of their content seems duplicated.

Thanks for pointing out. Indeed there was lots of repetition. I only added one missing test case (line 37 of vector_literals_valid.cl) and also removed another similar file test/SemaOpenCL/vector_literals_const.cl.

In C++, we have the comma operator and therefore the AST is significantly different. Running the content of your test file with -x c++ shows that it is rejected as desired. It could be worth having a negative test to ensure we always reject this in vanilla C++.

Added test case in test/SemaCXX/vector.cpp. However I am now confused what syntax we shouldn't accept exactly. @rjmccall do you think there should be an error on line 341?

Okay. It seems reasonable to me to allow OpenCL vector initialization syntax in OpenCL C++, since we're treating OpenCL C++ as an ObjC++-like merge of the language extensions, but we shouldn't go further than that and start changing behavior in the non-OpenCL languages.

In D65256#1601510, @jordan_rose wrote:

In D65256#1601509, @rjmccall wrote:

Sorry, am I missing something? Such a union would've been either ill-formed or unavailable in ARC (depending on where it was declared) before this recent work.

Apparently that was not the case if it was in a system header. Instead, Clang marked the member unavailable rather than the entire union.

In D65256#1601415, @jordan_rose wrote:

In D65256#1601410, @rjmccall wrote:

These were unavailable in system headers before because otherwise we would've had to make them invalid. Since these unions are no longer otherwise invalid, there shouldn't be a problem with allowing them in system headers, and in fact making the semantics vary that way seems quite problematic. Now, specific *uses* in system headers might still appear to be invalid — e.g. an ObjC ivar of type union { __strong id x; } — and the right behavior is definitely that those use sites should be marked as invalid instead of refusing to compile the system header.

That's the right answer on paper, but it's source-breaking in practice, for both Swift and Objective-C. On the Objective-C side, someone could have been using, say, glob_t in a copyable way in their ARC code, never touching the block field, and it would have been working fine. On the Swift side, we won't be able to import such a union at all when previously we would have.

These were unavailable in system headers before because otherwise we would've had to make them invalid. Since these unions are no longer otherwise invalid, there shouldn't be a problem with allowing them in system headers, and in fact making the semantics vary that way seems quite problematic. Now, specific *uses* in system headers might still appear to be invalid — e.g. an ObjC ivar of type union { __strong id x; } — and the right behavior is definitely that those use sites should be marked as invalid instead of refusing to compile the system header.

Wait, plain C++? Do we currently allow this syntax outside of OpenCL?

LGTM

Yes, that's the right fix, although you might also consider adding a getObjectType() to CXXMemberCallExpr.

LGTM

LGTM with one minor request.