[PATCH] D57438: [Sema][ObjC] Allow declaring ObjC pointer members in C++ unions under ARC

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:7033 + // member of an ObjC pointer type, except when it has an in-class initializer, + // it doesn't make the defaulted default constructor defined as deleted. + if (!FieldType.hasNonTrivialObjCLifetime() |

[PATCH] D53925: [modules] Defer emission of inline key functions.

rjmccall added a comment. Seems reasonable to me. Repository: rC Clang https://reviews.llvm.org/D53925 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53725: [CodeGen] Move `emitConstant` from ScalarExprEmitter to CodeGenFunction. NFC.

rjmccall accepted this revision. rjmccall added a comment. Thanks. Still LGTM. :) https://reviews.llvm.org/D53725 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53674: [CodeGen] Fix assertion on referencing constexpr Obj-C object with ARC.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D53674 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added a comment. In https://reviews.llvm.org/D53738#1281894, @ebevhan wrote: > In https://reviews.llvm.org/D53738#1281332, @rjmccall wrote: > > > Well, maybe the cleanest solution would be to actually fold > > `CompoundAssignOperator` back into `BinaryOperator` and just allow > > `Bina

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added a comment. In https://reviews.llvm.org/D53738#1283861, @ebevhan wrote: > In https://reviews.llvm.org/D53738#1283459, @rjmccall wrote: > > > Well, it could be passed around through most code as some sort of > > abstractly-represented intermediate "type" which could be either a Qual

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

rjmccall added inline comments. Comment at: lib/AST/Expr.cpp:1609 case CK_AddressSpaceConversion: -assert(getType()->isPointerType() || getType()->isBlockPointerType()); -assert(getSubExpr()->getType()->isPointerType() || - getSubExpr()->getType()->isBlockPoi

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added a comment. >> 1. You would have an inconsistency in either case, since e.g. numeric + >> otherwise always returns the same type as its operands, but this would not. > > True, but the CompoundAssignOperator already has that inconsistency with > ComputationResultType. Right, but i

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

rjmccall added inline comments. Comment at: docs/ClangCommandLineReference.rst:805 -Enable poisoning array cookies when using class member operator new\[\] in AddressSanitizer +Enable poisoning array cookies when using custom operator new\[\] in AddressSanitizer ---

[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic

rjmccall added a comment. In https://reviews.llvm.org/D53705#1284203, @keryell wrote: > I was really meaning "run on the CPU" and not "run on the GPU", it was not a > typo from me. :-) > If there are no visible language extensions besides C++ classes, it is > easier to run the kernel code on a

[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic

rjmccall added a comment. In https://reviews.llvm.org/D53705#1284734, @keryell wrote: > In https://reviews.llvm.org/D53705#1284268, @rjmccall wrote: > > > > > > > > > Okay. So the purpose of OpenCL C++ is to provide a non-unified model that > > allows you to easily run C++ code on the CPU. > >

[PATCH] D54010: [CodeGen] Fix a crash when updating a zeroinitialize designated initializer

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprConstant.cpp:1142 + llvm_unreachable("Unexpected Base constant type!"); +} Is `getAggregateElement` not good enough here? Repository: rC Clang https://reviews.llvm.org/D54010 _

[PATCH] D54010: [CodeGen] Fix a crash when updating a zeroinitialize designated initializer

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D54010 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Repository: rC Clang https://reviews.llvm.org/D52615 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

[PATCH] D54055: CXXNameMangler::mangleFunctionParam: fix assertion

rjmccall added a comment. That sounds more like this use of the mangler isn't manipulating the function type context correctly. But actually I think the problem is that it's ridiculous to assume that arbitrary local declarations have meaningful manglings. Why are we calling `getStaticDeclName

[PATCH] D54055: CXXNameMangler::mangleFunctionParam: fix assertion

rjmccall added a comment. In https://reviews.llvm.org/D54055#1286397, @jfb wrote: > In https://reviews.llvm.org/D54055#1286396, @rjmccall wrote: > > > That sounds more like this use of the mangler isn't manipulating the > > function type context correctly. But actually I think the problem is th

[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic

rjmccall added a comment. I'm just concerned about adding a great deal of complexity to mainline Clang in order to support a language that might still be in major flux and which I feel is likely to be forced to re-embrace qualifiers in the language model. Maybe this work should happen in a bra

[PATCH] D54048: [AST] Get aliased type info from an aliased TemplateSpecialization.

rjmccall added a comment. Changing how `TemplateSpecializationType` is declared in `TypeNodes.def` has way more implications than you're giving it credit for. `FooType::desugar()` should always perform a single-step desugar, but on `TemplateSpecializationType` it's calling `getCanonicalTypeInte

[PATCH] D54055: CXXNameMangler::mangleFunctionParam: fix assertion

rjmccall added a comment. Pfft, if I'm going to be held responsible for my own work, I'm in a lot of trouble. :) Function name + local variable name WFM. Repository: rC Clang https://reviews.llvm.org/D54055 ___ cfe-commits mailing list cfe-comm

[PATCH] D53780: Fix bitcast to address space cast for coerced load/stores

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1257 + Address Casted = CGF.Builder.CreateBitCast(Tmp, CGF.Int8Ty->getPointerTo(Tmp.getAddressSpace())); + Address SrcCasted = CGF.Builder.CreateBitCast(Src, CGF.Int8Ty->getPointerTo(Src.getAddressSpace()));

[PATCH] D54048: [AST] Get aliased type info from an aliased TemplateSpecialization.

rjmccall added a comment. That looks a lot better, thanks. Did you check whether any other tests needed adjustment? That's not uncommon from this kind of desugaring change. If not, LGTM. https://reviews.llvm.org/D54048 ___ cfe-commits mailing li

[PATCH] D54048: [AST] Get aliased type info from an aliased TemplateSpecialization.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Great! https://reviews.llvm.org/D54048 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D53780: Fix bitcast to address space cast for coerced load/stores

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks! LGTM. Repository: rC Clang https://reviews.llvm.org/D53780 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.l

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. I agree with Richard that I'm not sure what the point of supporting frontend visibility settings in OpenCL is. If you want the "everything is internal to the image" optimization, presumably you can just infer visibility on everything in a pass over the IR.

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. In https://reviews.llvm.org/D53153#1288083, @arsenm wrote: > In https://reviews.llvm.org/D53153#1288059, @rjmccall wrote: > > > I agree with Richard that I'm not sure what the point of supporting > > frontend visibility settings in OpenCL is. If you want the "everythin

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. In https://reviews.llvm.org/D53153#1288112, @rjmccall wrote: > But do you want to support *dynamically* linking object files? Because > that's what visibility is about. To be specific, if you don't have multiple levels of linking — doing a slower and relatively more

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. Okay, that's interesting. And that dynamic linking step includes fairly unrestricted linking of OpenCL code to other OpenCL code, rather than just e.g. loading a single block of OpenCL code that exports a small, fixed interface? If so, then I accept that you need sym

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added a comment. In https://reviews.llvm.org/D53738#1287123, @ebevhan wrote: > In https://reviews.llvm.org/D53738#1284213, @rjmccall wrote: > > > Not out of line with other features that significantly break with what's > > expressible. But the easy alternative to storing the intermedia

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added a comment. In https://reviews.llvm.org/D53738#1288372, @ebevhan wrote: > In https://reviews.llvm.org/D53738#1288333, @rjmccall wrote: > > > In https://reviews.llvm.org/D53738#1287123, @ebevhan wrote: > > > > > In https://reviews.llvm.org/D53738#1284213, @rjmccall wrote: > > > > > >

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. In https://reviews.llvm.org/D53153#1289380, @scott.linder wrote: > I don't believe that is currently the case (the unrestricted linking of OCL > code to OCL code via a dynamic linker), but we do have the notion of a static > link step, followed by dynamic linking at ru

[PATCH] D53263: Fix places where the return type of a FunctionDecl was being used in place of the function type

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D53263 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

rjmccall added inline comments. Comment at: lib/AST/Expr.cpp:1609 case CK_AddressSpaceConversion: -assert(getType()->isPointerType() || getType()->isBlockPointerType()); -assert(getSubExpr()->getType()->isPointerType() || - getSubExpr()->getType()->isBlockPoi

[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

rjmccall added a comment. In https://reviews.llvm.org/D52674#1291284, @smeenai wrote: > In https://reviews.llvm.org/D52674#1281747, @rjmccall wrote: > > > In https://reviews.llvm.org/D52674#1271814, @smeenai wrote: > > > > > @rjmccall I prototyped the ForRTTI parameter approach in > > > https://

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:4289 + /*BasePath=*/nullptr, CCK) + .get(); Okay. But if `ToType` *isn't* a reference type, this will never be an address-space conversion. I feel

[PATCH] D53949: [clang][CodeGen] Implicit Conversion Sanitizer: discover the world of CompoundAssign operators

rjmccall added a comment. Seems reasonable to me. Repository: rC Clang https://reviews.llvm.org/D53949 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

rjmccall added a comment. In https://reviews.llvm.org/D50119#1297070, @dblaikie wrote: > Hey @rjmccall - I'm trying to remember, but can't get it quite clear in my > head. I seem to recall some discussion about trivial_abi not implying/being > strong enough for all the cases that trivial_reloca

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:4289 + /*BasePath=*/nullptr, CCK) + .get(); Anastasia wrote: > rjmccall wrote: > > Okay. But if `ToType` *isn't* a reference type, this will never be

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1096 + equivalent to copying the underlying bytes and then dropping the source object + on the floor. * ``__is_destructible`` (MSVC 2013) Quuxplusone wrote: > @rjmccall wrote: > > trivial

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1096 + equivalent to copying the underlying bytes and then dropping the source object + on the floor. * ``__is_destructible`` (MSVC 2013) Quuxplusone wrote: > rjmccall wrote: > > Quuxplus

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1096 + equivalent to copying the underlying bytes and then dropping the source object + on the floor. * ``__is_destructible`` (MSVC 2013) Quuxplusone wrote: > rjmccall wrote: > > Quuxplus

[PATCH] D54055: CGDecl::emitStoresForConstant fix synthesized constant's name

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Seems reasonable to me. Repository: rC Clang https://reviews.llvm.org/D54055 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:

[PATCH] D54489: Implement -frecord-gcc-switches

rjmccall added a comment. In https://reviews.llvm.org/D54489#1297509, @scott.linder wrote: > In https://reviews.llvm.org/D54489#1297504, @troyj wrote: > > > I realize that you're probably striving for option compatibility with gcc, > > but continuing to name it -frecord-gcc-switches when it actu

[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

rjmccall added a comment. I'm not worried about the mangler being re-used for multiple declarations, I'm worried about a global flag changing how we mangle all components of a type when we only mean to change it at the top level. Repository: rC Clang https://reviews.llvm.org/D52674 _

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1096 + equivalent to copying the underlying bytes and then dropping the source object + on the floor. * ``__is_destructible`` (MSVC 2013) Quuxplusone wrote: > rjmccall wrote: > > Quuxplus

[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

rjmccall added a comment. In https://reviews.llvm.org/D52674#1297893, @smeenai wrote: > In https://reviews.llvm.org/D52674#1297879, @rjmccall wrote: > > > I'm not worried about the mangler being re-used for multiple declarations, > > I'm worried about a global flag changing how we mangle all com

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:4289 + /*BasePath=*/nullptr, CCK) + .get(); Anastasia wrote: > rjmccall wrote: > > Anastasia wrote: > > > rjmccall wrote: > > > > Okay. But if `ToTyp

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:4289 + /*BasePath=*/nullptr, CCK) + .get(); Anastasia wrote: > rjmccall wrote: > > Anastasia wrote: > > > rjmccall wrote: > > > > Anastasia wrote: > >

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1096 + equivalent to copying the underlying bytes and then dropping the source object + on the floor. * ``__is_destructible`` (MSVC 2013) Quuxplusone wrote: > dblaikie wrote: > > Quuxplus

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

rjmccall added a comment. IIRC, abbreviations just silently don't take effect if the record doesn't conform; so things will appear to work, but the size on disk will be bigger. Comment at: include/clang/AST/Expr.h:1615 + } + + /// Build a string literal. ric

[PATCH] D54539: [CodeGen] Replace '@' characters in block descriptors' symbol names with '\1' on ELF targets.

rjmccall added a comment. This should not be changing the actual `@`-encoding value. If we want to use the `@`-encoding in the symbol name, we can change it when naming the symbol, provided that \1 is not legal in the `@`encoding. Repository: rC Clang https://reviews.llvm.org/D54539 ___

[PATCH] D52440: Emit lifetime markers for temporary function parameter aggregates

rjmccall added a comment. In https://reviews.llvm.org/D52440#1298816, @itessier wrote: > > This is problematic because we're not necessarily in a scope that usefully > > limits the duration of cleanups — we don't push full-expression scopes when > > emitting an arbitrary statement. Probably we

[PATCH] D54489: Implement -frecord-command-line (-frecord-gcc-switches)

rjmccall added inline comments. Comment at: docs/ClangCommandLineReference.rst:797 + +Generate a section .LLVM.command.line containing the clang driver command line. + 1. Is this section always called `.LLVM.command.line`, or does it differ by target object-file

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

rjmccall added a comment. In https://reviews.llvm.org/D54166#1299149, @riccibruno wrote: > In https://reviews.llvm.org/D54166#1298889, @rjmccall wrote: > > > IIRC, abbreviations just silently don't take effect if the record doesn't > > conform; so things will appear to work, but the size on disk

[PATCH] D54166: [AST] Store the string data in StringLiteral in a trailing array of chars

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D54166 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D54539: [CodeGen] Replace '@' characters in block descriptors' symbol names with '\1' on ELF targets.

rjmccall added a comment. Oh, I see, sorry. Please rename these functions to make their purpose clear. Also, you could reasonably do this substitution on all targets. Repository: rC Clang https://reviews.llvm.org/D54539 ___ cfe-commits mailing

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. https://reviews.llvm.org/D53764 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/l

[PATCH] D54489: Implement -frecord-command-line (-frecord-gcc-switches)

rjmccall added inline comments. Comment at: docs/ClangCommandLineReference.rst:797 + +Generate a section .LLVM.command.line containing the clang driver command line. + scott.linder wrote: > rjmccall wrote: > > 1. Is this section always called `.LLVM.command.line`

[PATCH] D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang

rjmccall added inline comments. Comment at: include/clang/Basic/AttrDocs.td:1749-1750 +On 64-bit ARM targets, this argument causes the function to obey the vector +procedural call standard (VPCS) rules as described in the Vector ABI for +AArch64. In particular, the register calle

[PATCH] D54634: [OpenCL] Fix address space deduction in template args

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Makes sense. https://reviews.llvm.org/D54634 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/li

[PATCH] D54489: Implement -frecord-command-line (-frecord-gcc-switches)

rjmccall added inline comments. Comment at: docs/ClangCommandLineReference.rst:799 +command-line. Command-lines in the section are surrounded and separated by null +bytes. Spaces and backslashes in the command-line are escaped with backslashes. +(ELF Only) How ab

[PATCH] D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang

rjmccall added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:1101 case CC_AAPCS: + case CC_AArch64VectorCall: return llvm::dwarf::DW_CC_LLVM_AAPCS; rnk wrote: > sdesmalen wrote: > > I wasn't really sure whether this requires a corresponding >

[PATCH] D54379: Add Hurd toolchain support to Clang

rjmccall added inline comments. Comment at: lib/Driver/ToolChains/Hurd.cpp:84 + const Driver &D = getDriver(); + std::string SysRoot = computeSysRoot(); + kristina wrote: > Move semantics? Or is this guaranteed elision (I'm not sure)? If you're talking about th

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

rjmccall added a reviewer: ahatanak. rjmccall added inline comments. Comment at: include/clang/AST/DeclCXX.h:482 +/// and a defaulted destructor. +unsigned IsNaturallyTriviallyRelocatable : 1; + Quuxplusone wrote: > erichkeane wrote: > > Quuxplusone wrote

[PATCH] D54675: [AST] Store the expressions in ParenListExpr in a trailing array

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D54675 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D54676: [AST] Pack CallExpr

rjmccall added a comment. I don't think we should be reducing the number of call arguments we can support, sorry, even if 16K is a fairly absurd number that would probably trip stack overflow protections if you actually executed it. Let's try to keep it at least 64K-ish. Repository: rC Cla

[PATCH] D53949: [clang][CodeGen] Implicit Conversion Sanitizer: discover the world of CompoundAssign operators

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Heh, okay. Repository: rC Clang https://reviews.llvm.org/D53949 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.

[PATCH] D54676: [AST] Pack CallExpr

rjmccall added a comment. You can have more arguments than parameters because of varargs. Even putting that aside, no, I think we generally shouldn't go backwards on these limits. Anyway, packing right up to the limits imposed by `NumExprBits` probably isn't a great idea. Repository: rC C

[PATCH] D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang

rjmccall added a comment. Implementation LGTM. Comment at: include/clang/Basic/AttrDocs.td:1753 +This means it is more efficient to call such functions from code that performs +extensive floating-point and vector calculations, because fewer live SIMD&FP +registers need to be sa

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

rjmccall added a comment. In https://reviews.llvm.org/D50119#1303423, @Quuxplusone wrote: > In the `unordered_set [[maybe_trivially_relocatable]]` patch, I must wrap the > attribute in a macro `_LIBCPP_MAYBE_TRIVIALLY_RELOCATABLE_UNLESS_DEBUG`. > Without this caveat, I would have ended up with

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

rjmccall added a comment. In https://reviews.llvm.org/D50119#1303662, @Quuxplusone wrote: > In https://reviews.llvm.org/D50119#1303577, @rjmccall wrote: > > > In https://reviews.llvm.org/D50119#1303423, @Quuxplusone wrote: > > > > > In the `unordered_set [[maybe_trivially_relocatable]]` patch, I

[PATCH] D54768: Don't speculatively emit VTTs for classes unless we are able to correctly emit references to all the functions they will (directly or indirectly) reference.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D54768 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:80 +// used with the same version of generated operators. +RecTy = Context.getAddrSpaceQualType(RecTy, LangAS::opencl_generic); + I would suggest taking this opportunity to set up the AST

[PATCH] D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54425/new/ https://reviews.llvm.org/D54425 ___ cfe-commits mailing list

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:80 +// used with the same version of generated operators. +RecTy = Context.getAddrSpaceQualType(RecTy, LangAS::opencl_generic); + Anastasia wrote: > rjmccall wrote: > > I would suggest tak

[PATCH] D54858: [OpenCL] Improve diagnostics for address spaces in template instantiation

rjmccall added inline comments. Comment at: lib/Sema/TreeTransform.h:4241 + if (SemaRef.getLangOpts().OpenCL && T.getType()->isTemplateTypeParmType()) +Quals.removeAddressSpace(); + When do you actually add the qualifier back? Also, I don't think this is sp

[PATCH] D54589: [clang][UBSan] Sanitization for alignment assumptions.

rjmccall added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:75 - ``-fsanitize=alignment``: Use of a misaligned pointer or creation - of a misaligned reference. + of a misaligned reference. Also sanitizes assume-aligned-like attributes. - ``-fsa

[PATCH] D54589: [clang][UBSan] Sanitization for alignment assumptions.

rjmccall added inline comments. Comment at: docs/UndefinedBehaviorSanitizer.rst:198 +assume-aligned-like attributes), `object-size``, and ``vptr`` checks do not +apply to pointers to types with the ``volatile`` qualifier lebedev.ri wrote: > rjmccall wrote: > >

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

rjmccall added a comment. In D50119#1305503 , @Quuxplusone wrote: > In D50119#1303720 , @rjmccall wrote: > > > In D50119#1303662 , @Quuxplusone > > wrote: > > > > > >> I sti

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:4035 + V = Builder.CreatePointerBitCastOrAddrSpaceCast(V, DestTy); +} mikael wrote: > rjmccall wrote: > > Always use the `performAddrSpaceConversion` target hook if there's a >

[PATCH] D54589: [clang][UBSan] Sanitization for alignment assumptions.

rjmccall added inline comments. Comment at: docs/ReleaseNotes.rst:310 + char **passthrough(__attribute__((align_value(1024))) char **x) { +return x; // <- check the + } unfinished Comment at: docs/UndefinedBehaviorSanitizer.rs

[PATCH] D54589: [clang][UBSan] Sanitization for alignment assumptions.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54589/new/ https://reviews.llvm.org/D54589 ___ cfe-commi

[PATCH] D54489: Implement -frecord-command-line (-frecord-gcc-switches)

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54489/new/ https://reviews.llvm.org/D54489 ___ cfe-commits mailing list

[PATCH] D54986: Make CodeGen choose when to emit vtables.

rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:3991-3992 + if (auto *MD = dyn_cast(D)) { +// FIXME: There's no reason to do this if the key function is inline. +// Formally, the ABI requires it, but the difference is not observable. +if (de

[PATCH] D54986: Make CodeGen choose when to emit vtables.

rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:3991-3992 + if (auto *MD = dyn_cast(D)) { +// FIXME: There's no reason to do this if the key function is inline. +// Formally, the ABI requires it, but the difference is not observable. +if (de

[PATCH] D54986: Make CodeGen choose when to emit vtables.

rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenModule.cpp:3991-3992 + if (auto *MD = dyn_cast(D)) { +// FIXME: There's no reason to do this if the key function is inline. +// Formally, the ABI requires it, but the difference is not observable. +if (de

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

rjmccall added inline comments. Comment at: lib/AST/DeclCXX.cpp:2187 ClassTy = C.getQualifiedType(ClassTy, Qualifiers::fromCVRUMask(getTypeQualifiers())); + I feel like the right design now is for `getTypeQualifiers()` to return

[PATCH] D54858: [OpenCL] Improve diagnostics for address spaces in template instantiation

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54858/new/ https://reviews.llvm.org/D54858 ___ cfe-commits mailing list cfe-comm

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

rjmccall added inline comments. Comment at: cfe/trunk/lib/CodeGen/CGExpr.cpp:4268 +DestTy.getAddressSpace(), ConvertType(DestTy)); +return MakeNaturalAlignPointeeAddrLValue(V, DestTy); + } romanovvlad wrote: > Hi, > > It seems this code doesn't work

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:2638 + // type. + QualType getCorrespondingSignedFixedPointType(QualType Ty) const; + Please include in the comment here that, unlike `getCorrespondingUnsignedType`, this has to b

[PATCH] D54986: Make CodeGen choose when to emit vtables.

rjmccall added inline comments. Comment at: lib/AST/ASTContext.cpp:9801 + RD->getTemplateSpecializationKind() == + TSK_ExplicitInstantiationDefinition; else Does it matter if it's not this particular declaration that's the explicit ins

[PATCH] D55067: [HIP] Fix offset of kernel argument for AMDGPU target

rjmccall added a comment. This seems backwards. Clang knows what the actual ABI alignment of the C type is, and it doesn't have to match the alignment of the IR type that IRGen produces. It's the actual C ABI alignment that's supposed to affect the calling convention, so there needs to be som

[PATCH] D55067: [HIP] Fix offset of kernel argument for AMDGPU target

rjmccall added a comment. I understand that it's copied into a properly-aligned local variable, but if it affects how the function is called, that's also part of the ABI, and it should be taken from the C alignment, not any vagaries of how struct types are translated into IR. The other reason

[PATCH] D54986: Make CodeGen choose when to emit vtables.

rjmccall added inline comments. Comment at: lib/AST/RecordLayoutBuilder.cpp:3069 const Decl *Result = Entry ? Entry.get(getExternalSource()) : computeKeyFunction(*this, RD); rsmith wrote: > rjmccall wrote: > > Why not just change `computeKeyFunction`

[PATCH] D55067: [HIP] Fix offset of kernel argument for AMDGPU target

rjmccall added a comment. In D55067#1313480 , @arsenm wrote: > I don't understand why there's a discrepancy in this case. Why does anything > think the alignment of a packed struct is anything other than 1? Why is the C > alignment claiming something hig

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added inline comments. Comment at: clang/include/clang/Basic/FixedPoint.h:67 + FixedPointSemantics + getCommonSemantics(const FixedPointSemantics &Other) const; + leonardchan wrote: > rjmccall wrote: > > Actually representing the fully-precise value is

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1304 +RHSTy = ResultTy; + } + ebevhan wrote: > rjmccall wrote: > > Hmm. So adding a signed integer to an unsigned fixed-point type always > > converts the integer to unsigned? That's

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. In D53153#1314777 , @scott.linder wrote: > In D53153#1291348 , @rjmccall wrote: > > > In D53153#1289380 , @scott.linder > > wrote: > > > > > I don

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

rjmccall added inline comments. Comment at: include/clang/AST/DeclCXX.h:2189 +C.addCVRUQualifiers(CVRU); +C.addQualifiers(getType().getQualifiers()); +return C; The address-space qualifier should get stored in the same way on the `FunctionProtoType`

[PATCH] D55127: [OpenCL] Diagnose conflicting address spaces between template definition and its instantiation

rjmccall added inline comments. Comment at: lib/Sema/SemaType.cpp:7232 + if (D.getContext() == DeclaratorContext::TemplateArgContext) +// Do not deduce address space for non-pointee type in template arg. +; I don't understand what you're tryi

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. In D53153#1315107 , @yaxunl wrote: > In D53153#1315039 , @scott.linder > wrote: > > > In D53153#1314798 , @rjmccall > > wrote: > > > > > You still

<    1   2   3   4   5   6   7   8   9   10   >