Page MenuHomePhabricator

rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (347 w, 4 d)

Recent Activity

Today

rjmccall added a comment to D66121: Debug Info: Nest Objective-C property function decls inside their container..

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.

Tue, Sep 17, 5:51 PM · debug-info

Yesterday

rjmccall accepted D67517: Create UsersManual section entitled 'Controlling Floating Point Behavior'.

LGTM, thank you.

Mon, Sep 16, 12:52 PM · Restricted Project
rjmccall added inline comments to D67517: Create UsersManual section entitled 'Controlling Floating Point Behavior'.
Mon, Sep 16, 12:30 PM · Restricted Project

Fri, Sep 13

rjmccall added inline comments to D67517: Create UsersManual section entitled 'Controlling Floating Point Behavior'.
Fri, Sep 13, 2:11 PM · Restricted Project
rjmccall added a comment to D67399: [ARM] Follow AACPS standard for volatile bitfields.
In D67399#1669568, @jfb 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?

Fri, Sep 13, 12:53 PM · Restricted Project
rjmccall added inline comments to D67517: Create UsersManual section entitled 'Controlling Floating Point Behavior'.
Fri, Sep 13, 12:51 PM · Restricted Project

Thu, Sep 12

rjmccall added a comment to D67399: [ARM] Follow AACPS standard for volatile bitfields.

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.

Thu, Sep 12, 9:56 PM · Restricted Project
rjmccall added a comment to D67399: [ARM] Follow AACPS standard for volatile bitfields.

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.

Thu, Sep 12, 5:12 PM · Restricted Project
rjmccall added a comment to D67517: Create UsersManual section entitled 'Controlling Floating Point Behavior'.

Thanks! Some wording suggestions.

Thu, Sep 12, 3:24 PM · Restricted Project
rjmccall added a reviewer for D67517: Create UsersManual section entitled 'Controlling Floating Point Behavior': scanon.
Thu, Sep 12, 2:03 PM · Restricted Project

Wed, Sep 11

rjmccall accepted D67429: Improve code generation for thread_local variables:.

LGTM.

Wed, Sep 11, 12:03 PM · Restricted Project, Restricted Project
rjmccall added a comment to D66121: Debug Info: Nest Objective-C property function decls inside their container..

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.

Wed, Sep 11, 12:02 PM · debug-info

Mon, Sep 9

rjmccall added a comment to D62731: [RFC] Add support for options -frounding-math, -fp-model=, and -fp-exception-behavior=, : Specify floating point behavior.

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?

Mon, Sep 9, 1:40 PM · Restricted Project
rjmccall updated subscribers of D62731: [RFC] Add support for options -frounding-math, -fp-model=, and -fp-exception-behavior=, : Specify floating point behavior.

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

Mon, Sep 9, 10:15 AM · Restricted Project

Fri, Sep 6

rjmccall accepted D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header.

Okay, thanks. LGTM.

Fri, Sep 6, 4:12 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header.
Fri, Sep 6, 11:15 AM · Restricted Project, Restricted Project
rjmccall added a comment to D67291: Teach the IRBuilder about constrained FPToSI and FPToUI.

LGTM

Fri, Sep 6, 10:57 AM · Restricted Project

Thu, Sep 5

rjmccall added a comment to D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header.

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

Thu, Sep 5, 10:06 AM · Restricted Project, Restricted Project

Wed, Sep 4

rjmccall added inline comments to D65744: [PR42707][OpenCL] Fix addr space deduction for auto.
Wed, Sep 4, 1:56 PM
rjmccall added inline comments to D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header.
Wed, Sep 4, 12:57 PM · Restricted Project, Restricted Project

Tue, Sep 3

rjmccall added a comment to D66121: Debug Info: Nest Objective-C property function decls inside their container..

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

Tue, Sep 3, 2:54 PM · debug-info
rjmccall added a comment to D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header.

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.

Tue, Sep 3, 2:35 PM · Restricted Project, Restricted Project

Fri, Aug 30

rjmccall added a comment to D65744: [PR42707][OpenCL] Fix addr space deduction for auto.

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?

Fri, Aug 30, 12:37 PM

Tue, Aug 27

rjmccall added inline comments to D62731: [RFC] Add support for options -frounding-math, -fp-model=, and -fp-exception-behavior=, : Specify floating point behavior.
Tue, Aug 27, 11:34 AM · Restricted Project

Mon, Aug 26

rjmccall added inline comments to D66360: Expose constructing a virtual method dispatch through the include/clang/CodeGen APIs..
Mon, Aug 26, 10:22 PM · Restricted Project
rjmccall added inline comments to D62731: [RFC] Add support for options -frounding-math, -fp-model=, and -fp-exception-behavior=, : Specify floating point behavior.
Mon, Aug 26, 10:03 PM · Restricted Project
rjmccall added a comment to D65744: [PR42707][OpenCL] Fix addr space deduction for auto.

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.

Mon, Aug 26, 9:48 PM
rjmccall added inline comments to D64811: Warn when NumParams overflows.
Mon, Aug 26, 9:43 PM · Restricted Project

Aug 16 2019

rjmccall added a comment to D66230: [coroutine] Fixes "cannot move instruction since its users are not dominated by CoroBegin" problem..

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.

Aug 16 2019, 11:54 PM · Restricted Project

Aug 15 2019

rjmccall added a comment to D66230: [coroutine] Fixes "cannot move instruction since its users are not dominated by CoroBegin" problem..

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?

Aug 15 2019, 10:00 PM · Restricted Project
rjmccall added a comment to D66230: [coroutine] Fixes "cannot move instruction since its users are not dominated by CoroBegin" problem..

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.

Aug 15 2019, 5:41 PM · Restricted Project

Aug 14 2019

rjmccall added a comment to D66230: [coroutine] Fixes "cannot move instruction since its users are not dominated by CoroBegin" problem..

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?

Aug 14 2019, 9:40 PM · Restricted Project
rjmccall added a comment to rG038d604f4f8c: [SimplifyLibCalls] Add noalias from known callsites.

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

Aug 14 2019, 8:27 AM

Aug 13 2019

rjmccall committed rGa318c5507348: Coroutines: adjust for SVN r358739 (authored by rjmccall).
Coroutines: adjust for SVN r358739
Aug 13 2019, 8:57 PM
rjmccall committed rG3bbf207fbc56: Don't run a full verifier pass in coro-splitting's private pipeline. (authored by rjmccall).
Don't run a full verifier pass in coro-splitting's private pipeline.
Aug 13 2019, 8:56 PM
rjmccall committed rG5f60b68c68c0: Remove unreachable blocks before splitting a coroutine. (authored by rjmccall).
Remove unreachable blocks before splitting a coroutine.
Aug 13 2019, 8:56 PM
rjmccall committed rG2133feec933e: Support swifterror in coroutine lowering. (authored by rjmccall).
Support swifterror in coroutine lowering.
Aug 13 2019, 8:56 PM
rjmccall committed rGdc4668e5cf91: Update for optimizer changes. (authored by rjmccall).
Update for optimizer changes.
Aug 13 2019, 8:56 PM
rjmccall committed rGd47801e7182a: In coro.retcon lowering, don't explode if the optimizer messes around with the… (authored by rjmccall).
In coro.retcon lowering, don't explode if the optimizer messes around with the…
Aug 13 2019, 8:56 PM
rjmccall committed rGac404832760a: Fix a use-after-free in the coro.alloca treatment. (authored by rjmccall).
Fix a use-after-free in the coro.alloca treatment.
Aug 13 2019, 8:56 PM
rjmccall committed rG62a5dde0c29f: Add intrinsics for doing frame-bound dynamic allocations within a coroutine. (authored by rjmccall).
Add intrinsics for doing frame-bound dynamic allocations within a coroutine.
Aug 13 2019, 8:56 PM
rjmccall committed rG137b50f0c3b1: Guard dumps in the coro intrinsic validation logic behind NDEBUG checks. dump()… (authored by rjmccall).
Guard dumps in the coro intrinsic validation logic behind NDEBUG checks. dump()…
Aug 13 2019, 8:56 PM
rjmccall committed rG382921418554: Generalize llvm.coro.suspend.retcon to allow an arbitrary number of arguments… (authored by rjmccall).
Generalize llvm.coro.suspend.retcon to allow an arbitrary number of arguments…
Aug 13 2019, 8:56 PM
rjmccall committed rG94010b2b7f47: Extend coroutines to support a "returned continuation" lowering. (authored by rjmccall).
Extend coroutines to support a "returned continuation" lowering.
Aug 13 2019, 8:56 PM
rjmccall committed rL368798: Coroutines: adjust for SVN r358739.
Coroutines: adjust for SVN r358739
Aug 13 2019, 8:55 PM
rjmccall committed rL368797: Don't run a full verifier pass in coro-splitting's private pipeline..
Don't run a full verifier pass in coro-splitting's private pipeline.
Aug 13 2019, 8:55 PM
rjmccall committed rL368796: Remove unreachable blocks before splitting a coroutine..
Remove unreachable blocks before splitting a coroutine.
Aug 13 2019, 8:55 PM
rjmccall committed rL368795: Support swifterror in coroutine lowering..
Support swifterror in coroutine lowering.
Aug 13 2019, 8:55 PM
rjmccall committed rL368794: Update for optimizer changes..
Update for optimizer changes.
Aug 13 2019, 8:55 PM
rjmccall committed rL368793: In coro.retcon lowering, don't explode if the optimizer messes around with the….
In coro.retcon lowering, don't explode if the optimizer messes around with the…
Aug 13 2019, 8:55 PM
rjmccall committed rL368792: Fix a use-after-free in the coro.alloca treatment..
Fix a use-after-free in the coro.alloca treatment.
Aug 13 2019, 8:55 PM
rjmccall committed rL368791: Add intrinsics for doing frame-bound dynamic allocations within a coroutine..
Add intrinsics for doing frame-bound dynamic allocations within a coroutine.
Aug 13 2019, 8:55 PM
rjmccall committed rL368790: Guard dumps in the coro intrinsic validation logic behind NDEBUG checks. dump()….
Guard dumps in the coro intrinsic validation logic behind NDEBUG checks. dump()…
Aug 13 2019, 8:55 PM
rjmccall committed rL368789: Generalize llvm.coro.suspend.retcon to allow an arbitrary number of arguments….
Generalize llvm.coro.suspend.retcon to allow an arbitrary number of arguments…
Aug 13 2019, 8:55 PM
rjmccall committed rL368788: Extend coroutines to support a "returned continuation" lowering..
Extend coroutines to support a "returned continuation" lowering.
Aug 13 2019, 8:55 PM
rjmccall accepted D27165: Add format_dynamic_key_arg attribute to improve "-Wformat" warnings for functions that load the formatting string dynamically based on a key value.

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

Aug 13 2019, 4:58 PM · Restricted Project
rjmccall added inline comments to D66094: [CodeGen] Emit destructor calls for non-trivial C structs returned by function calls and loaded from volatile objects.
Aug 13 2019, 4:13 PM · Restricted Project, Restricted Project
rjmccall added a comment to rG038d604f4f8c: [SimplifyLibCalls] Add noalias from known callsites.

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

Aug 13 2019, 3:21 PM
rjmccall added a comment to D27165: Add format_dynamic_key_arg attribute to improve "-Wformat" warnings for functions that load the formatting string dynamically based on a key value.

I agree with just special-casing this for now.

Aug 13 2019, 2:54 PM · Restricted Project
rjmccall added inline comments to D66121: Debug Info: Nest Objective-C property function decls inside their container..
Aug 13 2019, 2:29 PM · debug-info
rjmccall added inline comments to D66121: Debug Info: Nest Objective-C property function decls inside their container..
Aug 13 2019, 11:26 AM · debug-info
rjmccall added a comment to D65744: [PR42707][OpenCL] Fix addr space deduction for auto.

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".

Aug 13 2019, 11:23 AM

Aug 12 2019

rjmccall added inline comments to D66121: Debug Info: Nest Objective-C property function decls inside their container..
Aug 12 2019, 11:30 PM · debug-info
rjmccall added a comment to D65744: [PR42707][OpenCL] Fix addr space deduction for auto.

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?

Aug 12 2019, 11:26 PM
rjmccall added a comment to D65994: Extended FPOptions with new attributes.

Oh, sorry, I confused several patches, then.

Aug 12 2019, 11:14 PM · Restricted Project
rjmccall added a comment to D66047: Do not call replaceAllUsesWith to upgrade calls to ARC runtime functions to intrinsic calls.

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

Aug 12 2019, 11:42 AM · Restricted Project
rjmccall added a comment to D65665: Support for attributes on @class declarations.

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.

Aug 12 2019, 10:30 AM · Restricted Project
rjmccall accepted D47419: [SemaDeclCXX] Allow inheriting constructor declaration that specify a cv-qualified type.
Aug 12 2019, 10:30 AM · Restricted Project, Restricted Project
rjmccall accepted D66047: Do not call replaceAllUsesWith to upgrade calls to ARC runtime functions to intrinsic calls.

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

Aug 12 2019, 10:27 AM · Restricted Project
rjmccall added a comment to D65994: Extended FPOptions with new attributes.

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.

Aug 12 2019, 10:24 AM · Restricted Project
rjmccall added a comment to D66078: Added RAII object for authomatic restore of fp state.

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

Aug 12 2019, 10:22 AM · Restricted Project

Aug 8 2019

rjmccall accepted D62960: Add SVE opaque built-in types.

LGTM.

Aug 8 2019, 9:17 AM · Restricted Project, Restricted Project

Aug 7 2019

rjmccall added inline comments to D62960: Add SVE opaque built-in types.
Aug 7 2019, 2:39 PM · Restricted Project, Restricted Project

Aug 6 2019

rjmccall added a comment to D54565: Introduce `-Wctad` as a subgroup of `-Wc++14-compat`.

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).

Aug 6 2019, 8:05 AM · Restricted Project
rjmccall added a comment to D54565: Introduce `-Wctad` as a subgroup of `-Wc++14-compat`.

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.

Aug 6 2019, 8:03 AM · Restricted Project

Aug 5 2019

rjmccall added a comment to D65753: Builtins: Add some v2f16 variants.

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.

Aug 5 2019, 1:21 PM
rjmccall added a comment to D65753: Builtins: Add some v2f16 variants.

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.

Aug 5 2019, 12:54 PM
rjmccall added a comment to D65753: Builtins: Add some v2f16 variants.

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?
Aug 5 2019, 12:47 PM
rjmccall accepted D65597: WIP: Builtins: Start adding half versions of math builtins.

LGTM

Aug 5 2019, 11:56 AM

Aug 2 2019

rjmccall added a comment to D65597: WIP: Builtins: Start adding half versions of math builtins.

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.

Aug 2 2019, 6:06 PM
rjmccall accepted D65670: Use switch instead of series of comparisons.

LGTM

Aug 2 2019, 11:41 AM · Restricted Project, Restricted Project
rjmccall added inline comments to D64464: [CodeGen] Emit destructor calls for non-trivial C structs.
Aug 2 2019, 8:46 AM · Restricted Project
rjmccall accepted D64932: [Parser] Emit descriptive diagnostic for misplaced pragma.

LGTM

Aug 2 2019, 8:04 AM · Restricted Project, Restricted Project

Aug 1 2019

rjmccall added inline comments to D64811: Warn when NumParams overflows.
Aug 1 2019, 8:06 AM · Restricted Project

Jul 31 2019

rjmccall added a comment to D65405: [Parser] Use special definition for pragma annotations.

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.)

Jul 31 2019, 9:18 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D64932: [Parser] Emit descriptive diagnostic for misplaced pragma.
Jul 31 2019, 9:18 PM · Restricted Project, Restricted Project
rjmccall accepted D65406: [Parser] Change parameter type from int to enum.

LGTM

Jul 31 2019, 9:13 PM · Restricted Project, Restricted Project
rjmccall added a comment to D65286: [OpenCL] Allow OpenCL C style vector initialization in C++.

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?

Jul 31 2019, 9:13 PM · Restricted Project

Jul 26 2019

rjmccall added inline comments to D64811: Warn when NumParams overflows.
Jul 26 2019, 12:01 PM · Restricted Project
rjmccall added inline comments to D64811: Warn when NumParams overflows.
Jul 26 2019, 11:29 AM · Restricted Project
rjmccall added inline comments to D64932: [Parser] Emit descriptive diagnostic for misplaced pragma.
Jul 26 2019, 11:21 AM · Restricted Project, Restricted Project
rjmccall added a comment to D65286: [OpenCL] Allow OpenCL C style vector initialization in C++.

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.

Jul 26 2019, 8:36 AM · Restricted Project

Jul 25 2019

rjmccall added a comment to D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header.

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.

Jul 25 2019, 12:15 PM · Restricted Project, Restricted Project
rjmccall added a comment to D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header.

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.

Jul 25 2019, 11:28 AM · Restricted Project, Restricted Project
rjmccall added a comment to D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header.

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.

Jul 25 2019, 10:09 AM · Restricted Project, Restricted Project
rjmccall added a comment to D65286: [OpenCL] Allow OpenCL C style vector initialization in C++.

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

Jul 25 2019, 10:00 AM · Restricted Project

Jul 19 2019

rjmccall accepted D64569: [OpenCL] Improve destructor support in C++ for OpenCL.

LGTM

Jul 19 2019, 9:34 AM · Restricted Project, Restricted Project

Jul 18 2019

rjmccall added a comment to D64569: [OpenCL] Improve destructor support in C++ for OpenCL.

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

Jul 18 2019, 10:55 AM · Restricted Project, Restricted Project
rjmccall accepted D64934: Teach the IRBuilder about constrained FPTrunc and FPExt.

LGTM

Jul 18 2019, 10:52 AM · Restricted Project

Jul 17 2019

rjmccall added a comment to D64569: [OpenCL] Improve destructor support in C++ for OpenCL.

LGTM with one minor request.

Jul 17 2019, 12:06 PM · Restricted Project, Restricted Project