Page MenuHomePhabricator

rjmccall (John McCall)
User

Projects

User does not belong to any projects.

User Details

User Since
Jan 18 2013, 11:30 AM (339 w, 2 d)

Recent Activity

Fri, Jul 19

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

LGTM

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

Thu, Jul 18

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.

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

LGTM

Thu, Jul 18, 10:52 AM · Restricted Project

Wed, Jul 17

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

LGTM with one minor request.

Wed, Jul 17, 12:06 PM · Restricted Project, Restricted Project
rjmccall accepted D62584: [OpenCL][PR42033] Deducing addr space with template parameter types.

Thanks!

Wed, Jul 17, 9:31 AM · Restricted Project

Tue, Jul 16

rjmccall accepted D64804: [OpenCL][Sema] Minor refactoring and constraint checking.

Thanks.

Tue, Jul 16, 12:23 PM · Restricted Project, Restricted Project
rjmccall added a comment to D64780: Disallow most calling convention attributes on PS4..

I'm fine with the idea of targets making unsupported CCs hard errors.

Tue, Jul 16, 12:23 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D64569: [OpenCL] Improve destructor support in C++ for OpenCL.
Tue, Jul 16, 10:45 AM · Restricted Project, Restricted Project
rjmccall added a comment to D62584: [OpenCL][PR42033] Deducing addr space with template parameter types.

Minor comment then LGTM

Tue, Jul 16, 10:25 AM · Restricted Project

Mon, Jul 15

rjmccall added a comment to D64083: [OpenCL][Sema] Improve address space support for blocks.

Sounds good.

Mon, Jul 15, 3:53 PM · Restricted Project, Restricted Project
rjmccall added a comment to D62584: [OpenCL][PR42033] Deducing addr space with template parameter types.

Oh, yes, it definitely can't be done to class types. I suppose we should just forget about it.

Ok, regarding address space qualifiers in template instantiation - is it still ok that we ignore them here in this patch?

Mon, Jul 15, 3:53 PM · Restricted Project

Sat, Jul 13

rjmccall accepted D63975: Warn when ScopeDepthOrObjCQuals overflows.

Thanks, LGTM!

Sat, Jul 13, 11:32 AM · Restricted Project

Fri, Jul 12

rjmccall accepted D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..

Thanks!

Fri, Jul 12, 4:14 PM · Restricted Project, Restricted Project
rjmccall accepted D64656: Ensure placeholder instruction for cleanup is created.

LGTM

Fri, Jul 12, 4:14 PM · Restricted Project
rjmccall added a comment to D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..

Thanks, just a few minor comment requests now.

Fri, Jul 12, 3:15 PM · Restricted Project, Restricted Project
rjmccall added a comment to D64656: Ensure placeholder instruction for cleanup is created.

Oh, I see you've already answered that. I think it's fine to just leave this testing debug output if generated optimized output doesn't affect it; the bulk of our regression testing is with assertions-enabled compilers anyway.

Fri, Jul 12, 3:04 PM · Restricted Project
rjmccall added inline comments to D64656: Ensure placeholder instruction for cleanup is created.
Fri, Jul 12, 3:02 PM · Restricted Project
rjmccall added inline comments to D64656: Ensure placeholder instruction for cleanup is created.
Fri, Jul 12, 12:35 PM · Restricted Project
rjmccall added inline comments to D64569: [OpenCL] Improve destructor support in C++ for OpenCL.
Fri, Jul 12, 12:29 PM · Restricted Project, Restricted Project
rjmccall added a comment to D62960: Add SVE opaque built-in types.

Seems reasonable to me.

Fri, Jul 12, 12:18 PM · Restricted Project
rjmccall accepted D64400: [OpenCL][PR42390] Deduce addr space for templ specialization.

Okay.

Fri, Jul 12, 12:17 PM · Restricted Project
rjmccall added a comment to D62584: [OpenCL][PR42033] Deducing addr space with template parameter types.

Oh, yes, it definitely can't be done to class types. I suppose we should just forget about it.

Fri, Jul 12, 12:15 PM · Restricted Project
rjmccall added a comment to D64083: [OpenCL][Sema] Improve address space support for blocks.

Okay, so it sounds like *from the language perspective* all block pointers are actually pointers into __generic, and the thing with literals is just an implementation detail that's been inappropriately expressed in the AST. The better implementation strategy is to make sure that (1) the AST uses the size/alignment of a __generic pointer for a block pointer and (2) IRGen implicitly converts block literals to __generic pointers when it emits them, and then there's no such thing as a block pointer to a qualified type.

Fri, Jul 12, 10:50 AM · Restricted Project, Restricted Project
rjmccall added a comment to D64083: [OpenCL][Sema] Improve address space support for blocks.

A qualifier "outside" the block-pointer type is telling you what address space the object which holds the block pointer is in, which is a completely different thing. Instead of __global int (^block4)(void) = ^{ return 0; };, you need to write int (^__global block4)(void) = ^{ return 0; };.

Fri, Jul 12, 10:50 AM · Restricted Project, Restricted Project
rjmccall added a comment to D62413: [OpenCL][PR41727] Prevent ICE on global dtors.

If you're interested in working on this, great. I actually think there's zero reason to emit a non-null argument here on any target unless we're going to use the destructor as the function pointer — but we can do that on every Itanium target, so we should. So where we want to be is probably:

  • use the complete-object destructor and the object pointer as its argument when destroying a (non-array) object of C++ class type
  • otherwise use a custom function that materializes the object pointer on its own and uses a null pointer as the argument
Fri, Jul 12, 10:39 AM · Restricted Project
rjmccall accepted D62413: [OpenCL][PR41727] Prevent ICE on global dtors.

Current patch LGTM

Fri, Jul 12, 10:39 AM · Restricted Project
rjmccall added a comment to D63975: Warn when ScopeDepthOrObjCQuals overflows.

(Blocks don't actually allow default arguments, but apparently we still parse them, so we should test that path.)

Fri, Jul 12, 10:25 AM · Restricted Project
rjmccall added a comment to D63975: Warn when ScopeDepthOrObjCQuals overflows.

It's good to have a lambda test, but that one isn't actually testing the lambda path — the place the diagnostic will trigger is just the normal function-prototype path, just originally within a lambda. You can do something like this:

Fri, Jul 12, 10:24 AM · Restricted Project

Thu, Jul 11

rjmccall added inline comments to D63975: Warn when ScopeDepthOrObjCQuals overflows.
Thu, Jul 11, 8:03 PM · Restricted Project
rjmccall added a comment to D64400: [OpenCL][PR42390] Deduce addr space for templ specialization.

There are some code paths that I think are common between the parser and template instantiation, like BuildPointerType and BuildReferenceType, but if you want to do context-sensitive inference that might not be good enough.

Thu, Jul 11, 7:47 PM · Restricted Project
rjmccall added inline comments to D62584: [OpenCL][PR42033] Deducing addr space with template parameter types.
Thu, Jul 11, 7:41 PM · Restricted Project
rjmccall added inline comments to D64083: [OpenCL][Sema] Improve address space support for blocks.
Thu, Jul 11, 11:15 AM · Restricted Project, Restricted Project
rjmccall added inline comments to D62413: [OpenCL][PR41727] Prevent ICE on global dtors.
Thu, Jul 11, 10:49 AM · Restricted Project

Wed, Jul 10

rjmccall added inline comments to D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..
Wed, Jul 10, 11:30 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..
Wed, Jul 10, 4:18 PM · Restricted Project, Restricted Project
rjmccall added a comment to D63139: [Diagnostics] Implement -Wswitch-unreachable.

I agree that we really shouldn't use most of those — we shouldn't have null types or null child expressions anywhere in the AST. (Accessors might return null for *optional* children, of course, but that's different.) And yeah, a big part of that would be having an error type that could be used in various places (instead of e.g. defaulting to int).

Wed, Jul 10, 2:18 PM · Restricted Project
rjmccall added a comment to D63139: [Diagnostics] Implement -Wswitch-unreachable.

I agree that tools shouldn't be forced to deal with invalid AST that looks like valid AST. To me that means finding ways to preserve information that (1) don't badly violate invariants and (2) are easily discoverable as invalid. For case, which has external requirements (e.g. not being a duplicate value) not entirely dissimilar to a declaration, I think that means having an "invalid" flag; arbitrary tools already can't rely on the expression being constant-evaluable because of templates, although granted many tools might never look at template patterns. For other things (e.g. a binary operator) maybe that means using different classes (e.g. InvalidBinaryOperator) so that tools looking at an apparently well-typed expression don't need to consider totally invalid possibilities.

Wed, Jul 10, 1:42 PM · Restricted Project
rjmccall added inline comments to D62413: [OpenCL][PR41727] Prevent ICE on global dtors.
Wed, Jul 10, 11:05 AM · Restricted Project
rjmccall added inline comments to D64400: [OpenCL][PR42390] Deduce addr space for templ specialization.
Wed, Jul 10, 10:21 AM · Restricted Project
rjmccall added inline comments to D64083: [OpenCL][Sema] Improve address space support for blocks.
Wed, Jul 10, 10:16 AM · Restricted Project, Restricted Project

Tue, Jul 9

rjmccall added inline comments to D64400: [OpenCL][PR42390] Deduce addr space for templ specialization.
Tue, Jul 9, 11:09 PM · Restricted Project
rjmccall added inline comments to D62584: [OpenCL][PR42033] Deducing addr space with template parameter types.
Tue, Jul 9, 11:05 PM · Restricted Project
rjmccall added inline comments to D64083: [OpenCL][Sema] Improve address space support for blocks.
Tue, Jul 9, 10:55 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D64464: [CodeGen] Emit destructor calls for non-trivial C structs.
Tue, Jul 9, 10:49 PM · Restricted Project
rjmccall added a comment to D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..

Thanks, that looks great. A few more requests and then this will be ready to go, I think.

Tue, Jul 9, 10:30 PM · Restricted Project, Restricted Project
rjmccall added a comment to D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible..

I wouldn't favor adding something really obscure that was only useful for clang, but I think builtins to set and clear mask bits while promising to preserve object-reference identity would be more generally useful for libraries. For example, there might be somewhere in libc++ that could take advantage of this.

Tue, Jul 9, 10:13 AM · Restricted Project, Restricted Project

Mon, Jul 8

rjmccall accepted D63846: [clang] Preserve names of addrspacecast'ed values..

Thanks!

Mon, Jul 8, 8:17 PM · Restricted Project, Restricted Project
rjmccall accepted D63912: [ObjC] Add a warning for implicit conversions of a constant non-boolean value to BOOL.
Mon, Jul 8, 5:30 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D62413: [OpenCL][PR41727] Prevent ICE on global dtors.
Mon, Jul 8, 1:49 PM · Restricted Project
rjmccall added inline comments to D63912: [ObjC] Add a warning for implicit conversions of a constant non-boolean value to BOOL.
Mon, Jul 8, 1:41 PM · Restricted Project, Restricted Project
rjmccall added a comment to D63846: [clang] Preserve names of addrspacecast'ed values..

I don't know what I think about widespread use of -fno-discard-value-names for now; please continue to use FileCheck variables, and we can make a holistic decision about that flag later.

Sorry, I have one particular question about clang/test/CodeGenOpenCLCXX/addrspace-of-this.cl:
Test the address space of 'this' when invoking copy-constructor.
COMMON: [[C1GEN:%c1.ascast[0-9]*]] = addrspacecast %class.C* %c1 to %class.C addrspace(4)*

This check seems to rely on %c1 name already. I guess the matching may go off, if we do not use actual names on the right hand side of the assignment. Should I do anything about the right hand side, or just use a generic wildcard on the left hand side?

Mon, Jul 8, 1:41 PM · Restricted Project, Restricted Project
rjmccall accepted D63856: [ObjC] Add a -Wtautological-compare warning for BOOL.

Okay.

Mon, Jul 8, 1:33 PM · Restricted Project, Restricted Project
rjmccall added a comment to D63846: [clang] Preserve names of addrspacecast'ed values..

I don't know what I think about widespread use of -fno-discard-value-names for now; please continue to use FileCheck variables, and we can make a holistic decision about that flag later.

Mon, Jul 8, 1:29 PM · Restricted Project, Restricted Project
rjmccall accepted D60456: [RISCV] Hard float ABI support.

Yes, I think you can commit.

Mon, Jul 8, 12:40 PM · Restricted Project, Restricted Project
rjmccall added a comment to D63846: [clang] Preserve names of addrspacecast'ed values..

Please don't check IR names in test output. That actually includes anonymous names like %2; these should always be tested with FileCheck variables. I suggest using %.* as the pattern; if you're matching the LHS of an LLVM assignment, that shouldn't have problems with accidentally matching too much.

Mon, Jul 8, 12:03 PM · Restricted Project, Restricted Project
rjmccall added a comment to D60456: [RISCV] Hard float ABI support.

I'm fine with proceeding with your best guess about what the ABI should be.

Mon, Jul 8, 11:57 AM · Restricted Project, Restricted Project

Sat, Jul 6

rjmccall added a comment to D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible..

I would be opposed to any addition of a -f flag for this absent any evidence that it's valuable for some actual C code; this patch appears to be purely speculative. I certainly don't think we should be adding it default-on. Your argument about alignment is what I was referring to when I mentioned that this is enforcing alignment restrictions in places we traditionally and intentionally haven't.

Sat, Jul 6, 5:10 PM · Restricted Project, Restricted Project

Thu, Jul 4

rjmccall added a reviewer for D53295: Mark store and load of block invoke function as invariant.group: ahatanak.

Okay. Akira, do you have any interest in looking into this as a general block optimization?

Thu, Jul 4, 12:25 PM
rjmccall added inline comments to D62960: Add SVE opaque built-in types.
Thu, Jul 4, 12:24 PM · Restricted Project
rjmccall added a comment to D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible..

The pointer/integer conversion is "implementation-defined", but it's not totally unconstrained. C notes that "The mapping functions for converting a pointer to an integer or an integer to a pointer are intended to be consistent with the addressing structure of the execution environment.", and we do have to honor that. The standard allows that "the result ... might not point to an entity of the referenced type", but when in fact it's guaranteed to do so (i.e. it's not just a coincidental result of an implementation decision like the exact address of a global variable — no "guessing"), I do think we have an obligation to make it work. And on a practical level, there has to be *some* way of playing clever address tricks in the language in order to implement things like allocators and so forth. So this makes me very antsy.

I don't disagree. But I believe the question is if we have:

int *x = malloc(4);
int *y = malloc(4);
if (x & ~15 == y) {
  *(x & ~15) = 5; // Is this allowed, and if so, must the compiler assume that it might set the value of *y?
}

I certainly agree that we must allow the implementation of allocators, etc. But allocators, I think, have the opposite problem. They actually have some large underlying objects (from mmap or whatever), and we want the rest of the system to treat some subobjects of these larger objects as though they were independent objects of some given types. From the point of view of the allocator, we have x, and we have void *memory_pool, and we need to allow x & N to point into memory_pool, but because, from the allocator's perspective, we never knew that x didn't point into memory_pool (as, in fact, it likely does), that should be fine (*).

There might be more of an issue, for example, if for a given object, I happen to know that there's some interesting structure at the beginning of its page (or some other boundary).

Thu, Jul 4, 11:53 AM · Restricted Project, Restricted Project

Wed, Jul 3

rjmccall added inline comments to D62645: [Sema] Resolve placeholder types before type deduction to silence spurious `-Warc-repeated-use-of-weak` warnings .
Wed, Jul 3, 7:31 PM · Restricted Project, Restricted Project
rjmccall accepted D62645: [Sema] Resolve placeholder types before type deduction to silence spurious `-Warc-repeated-use-of-weak` warnings .

Thanks, LGTM.

Wed, Jul 3, 7:31 PM · Restricted Project, Restricted Project
rjmccall added a comment to D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible..

The pointer/integer conversion is "implementation-defined", but it's not totally unconstrained. C notes that "The mapping functions for converting a pointer to an integer or an integer to a pointer are intended to be consistent with the addressing structure of the execution environment.", and we do have to honor that. The standard allows that "the result ... might not point to an entity of the referenced type", but when in fact it's guaranteed to do so (i.e. it's not just a coincidental result of an implementation decision like the exact address of a global variable — no "guessing"), I do think we have an obligation to make it work. And on a practical level, there has to be *some* way of playing clever address tricks in the language in order to implement things like allocators and so forth. So this makes me very antsy.

Wed, Jul 3, 7:24 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..
Wed, Jul 3, 6:02 PM · Restricted Project, Restricted Project
rjmccall added a comment to D53157: Teach the IRBuilder about constrained fadd and friends.

Thanks, this looks good to me.

Wed, Jul 3, 6:01 PM · Restricted Project
rjmccall added a comment to D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible..

I agree with Eli that this isn't obviously a legal transformation. llvm.ptrmask appears to make semantic guarantees about e.g. the pointer after the mask referring to the same underlying object, which means we can only safely emit it when something about the source program makes that guarantee. It's not at all clear that C does so for an expression like (T*) ((intptr_t) x & N).

Wed, Jul 3, 4:47 PM · Restricted Project, Restricted Project

Tue, Jul 2

rjmccall added inline comments to D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..
Tue, Jul 2, 1:38 PM · Restricted Project, Restricted Project

Mon, Jul 1

rjmccall added inline comments to D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..
Mon, Jul 1, 2:34 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D53157: Teach the IRBuilder about constrained fadd and friends.
Mon, Jul 1, 1:48 PM · Restricted Project
rjmccall added inline comments to D53157: Teach the IRBuilder about constrained fadd and friends.
Mon, Jul 1, 11:26 AM · Restricted Project
rjmccall added inline comments to D63975: Warn when ScopeDepthOrObjCQuals overflows.
Mon, Jul 1, 10:27 AM · Restricted Project

Fri, Jun 28

rjmccall added inline comments to D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..
Fri, Jun 28, 7:39 PM · Restricted Project, Restricted Project
rjmccall added a comment to D58164: Block+lambda: allow reference capture.

Okay. In that case, yeah, it'd be good to get Richard's opinion about the right way to get that information here.

Fri, Jun 28, 7:36 PM · Restricted Project
rjmccall added a comment to D62645: [Sema] Resolve placeholder types before type deduction to silence spurious `-Warc-repeated-use-of-weak` warnings .

Thanks, looks much better.

Fri, Jun 28, 7:36 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..
Fri, Jun 28, 12:10 PM · Restricted Project, Restricted Project
rjmccall added a comment to D56366: New warning call-to-virtual-from-ctor-dtor when calling a virtual function from a constructor or a destructor.

Oh, sorry, I had completely forgotten about that.

Fri, Jun 28, 11:41 AM · Restricted Project

Thu, Jun 27

rjmccall added inline comments to D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..
Thu, Jun 27, 11:46 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D62413: [OpenCL][PR41727] Prevent ICE on global dtors.
Thu, Jun 27, 11:33 PM · Restricted Project
rjmccall added inline comments to D56366: New warning call-to-virtual-from-ctor-dtor when calling a virtual function from a constructor or a destructor.
Thu, Jun 27, 11:25 PM · Restricted Project
rjmccall added inline comments to D63260: [Attr] Support _attribute__ ((fallthrough)).
Thu, Jun 27, 11:11 PM · Restricted Project
rjmccall added inline comments to D62645: [Sema] Resolve placeholder types before type deduction to silence spurious `-Warc-repeated-use-of-weak` warnings .
Thu, Jun 27, 11:06 PM · Restricted Project, Restricted Project
rjmccall added a comment to D63139: [Diagnostics] Implement -Wswitch-unreachable.

Are the CaseStmts being dropped in C++ because the expression overflows? I agree that that's bad AST behavior; we should strive to generate AST whenever we can, even if it's not valid.

Thu, Jun 27, 11:02 PM · Restricted Project
rjmccall added a comment to D53295: Mark store and load of block invoke function as invariant.group.

Great, thank you. Yaxun, are you planning to pick this back up? I know it's been a long time.

Thu, Jun 27, 11:01 PM
rjmccall added a comment to D58164: Block+lambda: allow reference capture.

I'm not sure we've ever written down what the semantics of the block capture are actually supposed to be in this situation. I guess capturing a reference is the right thing to do? Is that what we generally do if this is a POD type?

Thu, Jun 27, 10:55 PM · Restricted Project
rjmccall added a comment to D63856: [ObjC] Add a -Wtautological-compare warning for BOOL.

This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have (BOOL) 2, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-{0,1} value in the BOOL — I think this is fine.

I think the party line is that it is undefined behaviour (in some sense), since UBSan will happily crash if you try to load a non-boolean value from a BOOL.

What? Since when?

https://reviews.llvm.org/D27607

Thu, Jun 27, 11:28 AM · Restricted Project, Restricted Project
rjmccall added a comment to D63856: [ObjC] Add a -Wtautological-compare warning for BOOL.

This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have (BOOL) 2, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-{0,1} value in the BOOL — I think this is fine.

I think the party line is that it is undefined behaviour (in some sense), since UBSan will happily crash if you try to load a non-boolean value from a BOOL.

Thu, Jun 27, 11:01 AM · Restricted Project, Restricted Project

Wed, Jun 26

rjmccall added a comment to D63856: [ObjC] Add a -Wtautological-compare warning for BOOL.

This only applies to relational operators, right? I'm a little uncomfortable with calling this "tautological" since it's not like it's *undefined behavior* to have (BOOL) 2, it's just *unwise*. But as long as we aren't warning about reasonable idioms that are intended to handle unfortunate situations — like other code that might have left a non-{0,1} value in the BOOL — I think this is fine.

Wed, Jun 26, 10:35 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..
Wed, Jun 26, 10:00 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D63753: [Sema] Instead of rejecting C unions with non-trivial fields, detect attempts to destruct/initialize/copy them..
Wed, Jun 26, 1:56 PM · Restricted Project, Restricted Project
rjmccall added inline comments to D53157: Teach the IRBuilder about constrained fadd and friends.
Wed, Jun 26, 11:44 AM · Restricted Project

Mon, Jun 24

rjmccall added a reviewer for D62088: [compiler-rt][builtins] Scaled Integer log10(): t.p.northover.

My understanding is that Tim Northover sometimes reviews patches to compiler-rt, even if he's not an official owner; CC'ing him.

Mon, Jun 24, 3:03 PM · Restricted Project, Restricted Project
rjmccall added a comment to D62088: [compiler-rt][builtins] Scaled Integer log10().

Thanks! Still don't feel qualified to actually approve compiler-rt changes, though.

Mon, Jun 24, 1:01 PM · Restricted Project, Restricted Project

Jun 20 2019

rjmccall added a comment to D63451: P0840R2: support for [[no_unique_address]] attribute.

Can this attribute not be applied to a base class, or to a type?

The standard attribute forbids that right now. We could add a custom attribute that permits it, but we're required to diagnose application of the standard attribute to a type -- though a warning would suffice to satisfy that requirement. (Because this gives "same as a base class" layout, adding it to a base class wouldn't have any effect right now, but we could certainly say that the attribute on a class type also implies the class is not POD for the purpose of layout, to permit tail padding reuse.)

Jun 20 2019, 2:06 PM · Restricted Project, Restricted Project
rjmccall accepted D63451: P0840R2: support for [[no_unique_address]] attribute.
Jun 20 2019, 1:40 PM · Restricted Project, Restricted Project

Jun 19 2019

rjmccall accepted D63277: [CUDA][HIP] Don't set "comdat" attribute for CUDA device stub functions..

Alright, thanks. I agree that per the documentation this should be sufficient.

Jun 19 2019, 6:26 PM · Restricted Project, Restricted Project
rjmccall added a comment to D63277: [CUDA][HIP] Don't set "comdat" attribute for CUDA device stub functions..

This optimization is disabled for functions not in COMDAT sections? Is that documented somewhere?

Jun 19 2019, 12:32 PM · Restricted Project, Restricted Project

Jun 18 2019

rjmccall added inline comments to D63451: P0840R2: support for [[no_unique_address]] attribute.
Jun 18 2019, 8:09 PM · Restricted Project, Restricted Project
rjmccall added a comment to D63451: P0840R2: support for [[no_unique_address]] attribute.

Can this attribute not be applied to a base class, or to a type? I think every time I've seen someone get bitten by the unique-address rule, it was because they had a base class that deleted some constructors (or something like that) and which was a base of a million different types; I'm sure the language model they'd prefer would be to just add an attribute to that one type instead of chasing down every place where they declared a field of the type.

Jun 18 2019, 6:45 PM · Restricted Project, Restricted Project
rjmccall added a comment to D62088: [compiler-rt][builtins] Scaled Integer log10().

Please add test cases for scale=0 and scale=width as I assume those need special handling (UB right now?).
And if scale=0 and scale=width needs special handling, then I guess scale=1 and scale=width-1 are new boundary values so I maybe it would be nice to have tests for those scales as well.

Added. For a scale of 0 we only return the integer result. For widths > 60 or 28, we return INT_MAX to represent an error since we need to represent 10 << scale in 64/32 bits for this to work. So the scale boundaries are [0,60] for the 64 bit version and [0,28] for the 32 bit version. We could technically increase this to go up to 63/31, although to get the precise result would require a bigger buffer (using more 128 bit ints) which would complicate things further.

For the 64 bit function, I realized that in order to get the precise result without a 128 bit int would require implementing division by 10 in 2 64 bit ints. I eventually found a way to do this and get the precise result, but am not sure if it should be included in this patch since it would be adding another large function. Perhaps it could be readded in a followup? For now, I removed the functions we fallback to if 128 bit ints aren't supported and only define the function if 128 bits are enabled.

Jun 18 2019, 4:24 PM · Restricted Project, Restricted Project
rjmccall added a comment to D59744: Fix i386 ABI "__m64" type bug.

If we're going to insert emms instructions automatically, it doesn't really make sense to do it in the frontend; the backend could figure out the most efficient placement itself. (See lib/Target/X86/X86VZeroUpper.cpp, which implements similar logic for AVX.) The part I'd be worried about is the potential performance hit from calling emms in places where other compilers wouldn't, for code using MMX intrinsics.

Jun 18 2019, 4:16 PM · Restricted Project, Restricted Project