[PATCH] D47694: [CUDA][HIP] Do not emit type info when compiling for device

2018-06-04 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. https://reviews.llvm.org/D47694 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47694: [CUDA][HIP] Do not emit type info when compiling for device

2018-06-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D47694#1120375, @yaxunl wrote: > In https://reviews.llvm.org/D47694#1120367, @rjmccall wrote: > > > Why not just have the driver disable RTTI in the frontend invocation? > > > CUDA/HIP uses single source for device and host. The host code may

[PATCH] D47627: [ASTContext] Make getAddrSpaceQualType replace address spaces.

2018-06-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Well, Sema should always be diagnosing conflicts. If you don't have a specific reason to allow replacement, I would prefer just fixing the documentation to state this as a precondition. Repository: rC Clang https://reviews.llvm.org/D47627

[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types

2018-06-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:1954 + llvm::APInt getFixedPointMin(QualType Ty) const; + llvm::APInt getFixedPointOne(QualType Ty) const; ebevhan wrote: > rjmccall wrote: > > Are these opaque bit-patterns? I think

[PATCH] D48589: [WIP] [CodeGen] Allow specifying Extend to CoerceAndExpand

2018-06-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/CodeGen/CGFunctionInfo.h:90 union { -unsigned DirectOffset; // isDirect() || isExtend() -unsigned IndirectAlign;// isIndirect() -unsigned AllocaFieldIndex; // isInAlloca() +llvm::StructType

[PATCH] D48589: [WIP] [CodeGen] Allow specifying Extend to CoerceAndExpand

2018-06-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D48589#1144976, @rogfer01 wrote: > @rjmccall because we do not want to impact the clients of `ABIArgInfo` I > thought of two possible approaches > > 1. extend `CGFunctionInfo` with a third trailing array (now it has two), with > as many

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

2018-06-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/FixedPoint.h:11 +/// \file +/// Defines the fixed point number interface. +// Referencing the standard here would be good, and maybe even excerpting the key parts. Comment at:

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

2018-06-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:90 + // by default. + bool SameFBits; + Sorry for the extremely late review, but this really needs to be renamed. Please remember that other compiler maintainers are not experts in

[PATCH] D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types

2018-06-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGObjCMac.cpp:7457-7460 CGObjCNonFragileABIMac::GetEHType(QualType T) { // There's a particular fixed type info for 'id'. if (T->isObjCIdType() || T->isObjCQualifiedIdType()) { +if

[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types

2018-06-24 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:1954 + llvm::APInt getFixedPointMin(QualType Ty) const; + llvm::APInt getFixedPointOne(QualType Ty) const; Are these opaque bit-patterns? I think there should be a type which

[PATCH] D48531: [CodeGen] Provide source locations for UBSan type checks when emitting constructor calls.

2018-06-24 Thread John McCall via Phabricator via cfe-commits
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/D48531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D48589: [WIP] [CodeGen] Allow specifying Extend to CoerceAndExpand

2018-06-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/CodeGen/CGFunctionInfo.h:90 union { -unsigned DirectOffset; // isDirect() || isExtend() -unsigned IndirectAlign;// isIndirect() -unsigned AllocaFieldIndex; // isInAlloca() +llvm::StructType

[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types

2018-06-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:768 +if (CGF.getContext().getTargetInfo().unsignedFixedPointTypesHavePadding() && +Ty->isUnsignedFixedPointType()) { + unsigned NumBits = CGF.getContext().getTypeSize(Ty);

[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers

2018-07-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647 + } +} + Prazek wrote: > rsmith wrote: > > Prazek wrote: > > > rjmccall wrote: > > > > Prazek wrote: > > > > > rjmccall wrote: > > > > > > Incidentally, how do you

[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

2018-04-30 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Parse/ParseStmtAsm.cpp:696 +return StmtError(); + } + svenvh wrote: > rjmccall wrote: > > svenvh wrote: > > > rjmccall wrote: > > > > svenvh wrote: > > > > > rjmccall wrote: > > > > > > You might consider

[PATCH] D46241: [CodeGen] Recognize more cases of zero initialization

2018-04-30 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExprConstant.cpp:1395 +static bool isZeroInitializer(ConstantEmitter , const Expr *Init) { + QualType InitTy = Init->getType().getCanonicalType(); You should have a comment here clarifying that this is

[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

2018-05-01 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Alrght, LGTM. https://reviews.llvm.org/D46022 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46241: [CodeGen] Recognize more cases of zero initialization

2018-05-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExprConstant.cpp:1403 + if (auto *IL = dyn_cast_or_null(Init)) { +if (InitTy->isConstantArrayType()) { + for (auto I : IL->inits()) Do you actually care if it's an array initialization instead

[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

2018-04-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Parse/ParseStmtAsm.cpp:696 +return StmtError(); + } + svenvh wrote: > rjmccall wrote: > > svenvh wrote: > > > rjmccall wrote: > > > > You might consider parsing the statement normally and then just > > > >

[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

2018-04-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/TokenKinds.def:255 +// KEYNOOPENCL - This is a keyword that is not supported in OpenCL C +// nor in OpenCL C++. // KEYALTIVEC - This is a keyword in AltiVec svenvh wrote: >

[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

2018-04-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Parse/ParseStmtAsm.cpp:696 +return StmtError(); + } + svenvh wrote: > rjmccall wrote: > > You might consider parsing the statement normally and then just diagnosing > > after the fact, maybe in Sema. You'd

[PATCH] D46052: GNUstep Objective-C ABI version 2

2018-04-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGObjCGNU.cpp:961 + GV->setSection(Section); +return GV; + } I'd encourage you to use ConstantBuilder whenever you would want to use this. Comment at:

[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

2018-04-26 Thread John McCall via Phabricator via cfe-commits
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/D45382 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45900: CodeGen: Fix invalid bitcast for lifetime.start/end

2018-04-30 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Oh, I see, it's not that the lifetime intrinsics don't handle pointers in the alloca address space, it's that we might have already promoted them into `DefaultAS`. Do the LLVM uses of lifetime intrinsics actually look through these address space casts? I'm wondering

[PATCH] D46042: Cap vector alignment at 16 for all Darwin platforms

2018-05-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I think we should seriously consider making alignment attributes on typedefs (and maybe some other attributes like may_alias) actual type qualifiers that are preserved in the canonical type, mangled, and so on. It would be an ABI break, but it'd also solve a lot of

[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang

2018-05-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Given that these are overloaded builtins, why are there separate builtins for `min` and `umin`? If this is a Clang extension, it needs to be documented in our extensions documentation. The documentation should describe the exact ordering semantics, to the extent we

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

2018-05-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:8931 + /*ConstArg*/ true, false, false, false, false); + auto *CopyCtor = cast_or_null(SMI.getMethod()); + Sorry, I didn't realize you'd go off and write this code

[PATCH] D39305: Simplify codegen and debug info generation for block context parameters.

2017-10-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. The original code doesn't make any sense; it looks like the indirection is backwards. We just built a debug variable corresponding to the block descriptor pointer parameter, so LocalAddr should be dbg.declare'd to be that variable and Arg should be dbg.value'd. It

[PATCH] D38774: [CodeGen] Add support for IncompleteArrayType in Obj-C ivars.

2017-10-26 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. https://reviews.llvm.org/D38774 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D39177: [CodeGen] Generate TBAA info for reference loads

2017-10-26 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D39177 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D39008: [CodeGen] Propagate may-alias'ness of lvalues with TBAA info

2017-10-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Sure, that makes sense to me. John. Repository: rL LLVM https://reviews.llvm.org/D39008 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38774: [CodeGen] Add support for IncompleteArrayType in Obj-C ivars.

2017-10-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjCMac.cpp:5095 +fieldType = fieldType->getAsArrayTypeUnsafe()->getElementType(); + } + You can't just use isa<> here; there can be typedefs of incomplete array type.

[PATCH] D39184: CodeGen: Fix invalid bitcast in partial initialization of automatic arrary variable

2017-10-23 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D39184 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D38773: [Sema] Add support for flexible array members in Obj-C.

2017-10-23 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D38773 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D38857: [OpenCL] Improve printing and semantic check related to implicit addr space

2017-10-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/Type.h:337 +Mask = (Mask & ~ImplicitAddrSpaceMask) | + (((uint32_t)Value) << ImplicitAddrSpaceShift); + } This is probably cleaner as: Mask = (Value ? (Mask | ImplicitAddrSpaceMask)

[PATCH] D39008: [CodeGen] Propagate may-alias'ness of lvalues with TBAA info

2017-10-23 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Well, I've been agreeing so far that it makes sense to propagate TBAA information separately from LValueBaseInfo, and this seems like a logical part of that, so okay. Repository: rL

[PATCH] D38126: Make TBAA information to be part of LValueBaseInfo

2017-10-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Hmm. I'm not sure I like the design of merging TBAAAccessInfo into LValueBaseInfo. LValueBaseInfo is currently the set of information that's generally preserved across l-value manipulations. It was extracted from LValue specifically to create an encapsulated entity

[PATCH] D39069: CodeGen: Fix missing debug loc due to alloca

2017-10-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. If this is something we generally need to be doing in all the places we temporarily save and restore the insertion point, we should fix the basic behavior of saveIP instead of adding explicit code to a bunch of separate places. Can we just override saveIP() on

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

2018-07-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks, much appreciated. A couple more style changes that I noticed and this will be LGTM, although you should also make sure you have Bevin Hansson's approval. Comment at: include/clang/AST/ASTContext.h:33 #include "clang/Basic/AddressSpaces.h"

[PATCH] D41039: Add support for attribute "trivial_abi"

2018-01-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D41039#951807, @rjmccall wrote: > In https://reviews.llvm.org/D41039#951648, @ahatanak wrote: > > > I had a discussion with Duncan today and he pointed out that perhaps we > > shouldn't allow users to annotate a struct with "trivial_abi" if

[PATCH] D41039: Add support for attribute "trivial_abi"

2018-01-05 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I'll trust Richard on the tricky Sema/AST bits. The functionality of the patch looks basically acceptable to me, although I'm still not thrilled about the idea of actually removing the attribute from the AST rather than just letting it not have effect. But we could

[PATCH] D41433: Unit tests for TBAA metadata generation.

2017-12-21 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Interesting. Okay, LGTM. Repository: rC Clang https://reviews.llvm.org/D41433 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

2018-01-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:239 + bit IncludeC = includeC; +} aaron.ballman wrote: > rjmccall wrote: > > aaron.ballman wrote: > > > rjmccall wrote: > > > > I have no objection to allowing ObjC attributes to be

[PATCH] D41039: Add support for attribute "trivial_abi"

2018-01-08 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, looks good to me. https://reviews.llvm.org/D41039 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41039: Add support for attribute "trivial_abi"

2018-01-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/Type.h:811 + bool hasTrivialABIOverride() const; + This should get a comment, even if it's just to refer to the CXXRecordDecl method. Comment at: lib/CodeGen/CGCall.cpp:3498

[PATCH] D41539: [CodeGen] Decorate aggregate accesses with TBAA tags

2018-01-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Yes, I think that's fine. I think unconditionally attaching the tag is probably wrong; you need suppress it in the may_alias cases. I'll grant that the existing code doesn't honor that, but that seems like a bug we should go ahead and fix. You'll need to either

[PATCH] D41539: [CodeGen] Decorate aggregate accesses with TBAA tags

2018-01-24 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, that works, thanks! LGTM. https://reviews.llvm.org/D41539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D42508: AST: support protocol conformances on id/class/interfaces in MS ABI

2018-01-24 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. The basic idea here seems fine to me; I'll leave David to review the details. Repository: rC Clang https://reviews.llvm.org/D42508 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method

2018-01-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/AST/ExprClassification.cpp:652 +if (Ctx.getCanonicalType(ASE->getBase()->getType()).isConstQualified()) + return Cl::CM_ConstQualified; + avt77 wrote: > rjmccall wrote: > > Is there a reason why the places

[PATCH] D42521: [CodeGen] Use the non-virtual alignment when emitting the base class subobject constructor

2018-01-25 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D42521 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method

2018-01-25 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/AST/ExprClassification.cpp:652 +if (Ctx.getCanonicalType(ASE->getBase()->getType()).isConstQualified()) + return Cl::CM_ConstQualified; + Is there a reason why the places that compute the type of these

[PATCH] D40023: [RISCV] Implement ABI lowering

2018-01-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1937 +RetAttrs.addAttribute(llvm::Attribute::ZExt); +} // FALL THROUGH asb wrote: > rjmccall wrote: > > I feel like a better design would be to record whether to do a sext or a

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

2018-01-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/Decl.h:3529 + /// Basic properties of non-trivial C structs. + bool HasStrongObjCPointer : 1; + Is it interesting to track all the individual reasons that a struct is non-trivial at the struct

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

2018-01-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1148 +DK_objc_weak_lifetime, +DK_c_struct_strong_field }; ahatanak wrote: > rjmccall wrote: > > I don't think you want to refer to the fact that the C struct specifically > >

[PATCH] D41999: Refactor handling of signext/zeroext in ABIArgInfo

2018-01-12 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, that looks great. I appreciate you doing this. Repository: rC Clang https://reviews.llvm.org/D41999 ___ cfe-commits mailing list

[PATCH] D40023: [RISCV] Implement ABI lowering

2018-01-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1937 +RetAttrs.addAttribute(llvm::Attribute::ZExt); +} // FALL THROUGH asb wrote: > rjmccall wrote: > > asb wrote: > > > rjmccall wrote: > > > > I feel like a better design would

[PATCH] D40023: [RISCV] Implement ABI lowering

2018-01-12 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks. LGTM, but you should wait for Eli's sign-off, too. https://reviews.llvm.org/D40023 ___ cfe-commits mailing list

[PATCH] D40569: Use default IR alignment for cleanup.dest.slot.

2018-01-12 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. r322406 Repository: rC Clang https://reviews.llvm.org/D40569 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D42660: [PR32482] Fix bitfield layout for -mms-bitfield and pragma pack

2018-01-29 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/AST/RecordLayoutBuilder.cpp:1564 unsigned MaxFieldAlignmentInBits = Context.toBits(MaxFieldAlignment); + unsigned RecordFieldAlign = FieldAlign; if (!MaxFieldAlignment.isZero() && FieldSize) { I think naming

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

2018-01-29 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Parse/ParseObjc.cpp:1434 MaybeParseGNUAttributes(paramAttrs); - ArgInfo.ArgAttrs = paramAttrs.getList(); } aaron.ballman wrote: > rjmccall wrote: > > ObjC parameter syntax is really its own weird

[PATCH] D42614: AST: support ObjC lifetime qualifiers in MS ABI

2018-01-29 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D42614#990808, @compnerd wrote: > Hmm, the only thing that we could fake would be namespaces on the parameter > types. Is that better? I'm not tied to re-using the existing mangling. I'm just worried about re-using the existing mangling

[PATCH] D42660: [PR32482] Fix bitfield layout for -mms-bitfield and pragma pack

2018-01-30 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Oh, that makes much more sense, thanks. Repository: rL LLVM https://reviews.llvm.org/D42660 ___ cfe-commits mailing list

[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method

2018-02-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. If you just want a better diagnostic, there's quite a bit of machinery already set up to give more specific errors about why such-and-such l-value isn't modifiable. There may even be vector-specific diagnostics for that already, just triggered from the wrong place in

[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method

2018-02-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. No, really, in CreateBuiltinArraySubscriptExpr and LookupMemberExpr, in the clauses where they handle vector types, you just need to propagate qualifiers from the base type like is done in BuildFieldReferenceExpr. There's even a FIXME about it in the former.

[PATCH] D42614: AST: support ObjC lifetime qualifiers in MS ABI

2018-01-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. It's not just that functions can't be overloaded on the parameter-variable type qualifier — it's not part of the function type at all, just like making a parameter 'const int' instead of 'int' is not part of the function type. I understand that MSVC mangles some

[PATCH] D41677: Change memcpy/memove/memset to have dest and source alignment attributes.

2018-01-27 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. Still LGTM. Repository: rC Clang https://reviews.llvm.org/D41677 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method

2018-02-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. That's still just const-propagation. The const qualifier on the pointee type of this should propagate to the l-value resulting from the member access, and the vector-specific projection logic should continue to propagate const to the type of the element l-value.

[PATCH] D42811: [CodeGen][va_args] Correct Vector Struct va-arg 'in_reg' code gen

2018-02-01 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. LGTM. Repository: rC Clang https://reviews.llvm.org/D42811 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D42768: AST: add an extension to support SwiftCC on MS ABI

2018-01-31 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Is demangling "correctly" really a more important goal here than not spuriously failing when presented with a swiftcall function type in a non-top-level position? I don't know that there was anything wrong with the previous patch's approach to this if we're just

[PATCH] D42768: AST: add an extension to support SwiftCC on MS ABI

2018-01-31 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. No, I mean things like `void foo(__attribute__((swiftcall)) void (*fnptr)());`. Repository: rC Clang https://reviews.llvm.org/D42768 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43013: ASan+operator new[]: Fix operator new[] cookie poisoning

2018-02-07 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I don't understand why your description of this patch mentions the void* placement new[] operator. There's no cookie to poison for that operator. Repository: rC Clang https://reviews.llvm.org/D43013 ___ cfe-commits

[PATCH] D43013: ASan+operator new[]: Fix operator new[] cookie poisoning

2018-02-08 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, thanks. LGTM. Repository: rC Clang https://reviews.llvm.org/D43013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D42530: Clang permits assignment to vector/extvector elements in a const method

2018-02-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks, that looks a lot better. One minor piece of feedback, but otherwise LGTM. Comment at: lib/Sema/SemaExpr.cpp:4402 +Qualifiers MemberQuals = +Context.getCanonicalType(ResultType).getQualifiers(); +Qualifiers Combined =

[PATCH] D42549: [CodeGen] Use the zero initializer instead of storing an all zero representation.

2018-02-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I'm somewhat surprised LLVM doesn't already canonicalize this, but ok. Should we also do this when building a constant struct? Comment at: lib/CodeGen/CGExprConstant.cpp:873 Elts.push_back(C); + if (!C->isNullValue()) +

[PATCH] D42614: AST: support ObjC lifetime qualifiers in MS ABI

2018-02-07 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D42614#1001279, @compnerd wrote: > @rjmccall, I've updated the approach and no longer abuse the existing > decoration styles. This uses a custom namespace

[PATCH] D42549: [CodeGen] Use the zero initializer instead of storing an all zero representation.

2018-02-08 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. No worries. LGTM! https://reviews.llvm.org/D42549 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D42549: [CodeGen] Use the zero initializer instead of storing an all zero representation.

2018-02-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Oh. It's not a good idea to try to match exact local IR names like this for two reasons: - First, many IR names are different in builds with and without assertions. - Second, it's pretty susceptible to innocuous changes in output. You should use FileCheck patterns

[PATCH] D43181: [CodeGen] Initialize large arrays by copying from a global

2018-02-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:421 + // with explicit initializers should be large enough. + if (NumInitElements > 8 && elementType->isBuiltinType()) { +CodeGen::CodeGenModule = CGF.CGM; Is there a good reason to use

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

2018-02-11 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. No, this LGTM. https://reviews.llvm.org/D41553 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43181: [CodeGen] Initialize large arrays by copying from a global

2018-02-13 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:421 + // with explicit initializers should be large enough. + if (NumInitElements > 8 && elementType->isBuiltinType()) { +CodeGen::CodeGenModule = CGF.CGM; kosarev wrote: > rjmccall

[PATCH] D43187: [AST] Refine the condition for element-dependent array fillers

2018-02-13 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rL LLVM https://reviews.llvm.org/D43187 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

2018-02-13 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1102 +PDK_Struct // non-trivial C struct. + }; + This is unused. Comment at: lib/CodeGen/CGBlocks.cpp:1565 // For all other types, the memcpy is fine. return

[PATCH] D43089: clang: Add ARCTargetInfo

2018-02-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:8185 +unsigned FreeRegs) const { + // TODO: C++ ABI? + unsigned SizeInRegs = (getContext().getTypeSize(Ty) + 31) / 32; Please do go ahead

[PATCH] D42366: [CodeGen] Fix generation of TBAA tags for may-alias accesses

2018-02-19 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I'm fine with that plan. LGTM. https://reviews.llvm.org/D42366 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D43430: Omit nullptr check for sufficiently simple delete-expressions

2018-02-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Discourse nitpick: I would encourage you to just use the ordinary phrase "null pointer", or just "null", when referring to a pointer value that happens to be null and to reserve "nullptr" for *statically* null pointers, especially the `nullptr` expression. If the

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

2018-02-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1121 + /// after it is moved, as opposed to a truely destructive move in which the + /// source object is placed in an uninitialized state. + PrimitiveCopyKind isNonTrivialToPrimitiveDestructiveMove() const;

[PATCH] D34367: CodeGen: Fix address space of indirect function argument

2018-02-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3446 + return LV.asAggregateRValue(); +} + No, I don't think this is right. This method should always return an independent RValue; if the CallArg is storing an LValue, it should copy from it

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

2018-02-24 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks! This looks ready; thank you for your patience in working this out. Comment at: include/clang/AST/Type.h:1108 +PCK_ARCStrong, // objc strong pointer. +

[PATCH] D43430: Omit nullptr check for sufficiently simple delete-expressions

2018-02-24 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. That is an extremely Google-specific analysis, actually; AFAIK almost nobody else uses static linking all the way down to and including the C and C++ standard libraries unless they're literally building an executable for a fully-static environment, like the kernel.

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

2018-02-15 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/Decl.h:3619 +return NonTrivialToPrimitiveDestructiveMove; + } + Please document that this means a C++-style destructive move that leaves the source object in a validly-initialized state.

[PATCH] D43181: [CodeGen] Initialize large arrays by copying from a global

2018-02-16 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks! https://reviews.llvm.org/D43181 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D34367: CodeGen: Fix address space of indirect function argument

2018-02-15 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. That's not really okay; there are some places that make guarantees about call-argument ordering, and in general we want that to be decided at a higher level, rather than having a particular order forced in corner cases just to satisfy a lower-level implementation

[PATCH] D34367: CodeGen: Fix address space of indirect function argument

2018-02-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3505 +if (AS != LangAS::Default && AS != LangAS::opencl_private && +AS != CGM.getASTAllocaAddressSpace()) + Flag = CallArg::ByValArgNeedsCopy; This is an odd condition. What are

[PATCH] D42366: [CodeGen] Fix generation of TBAA tags for may-alias accesses

2018-02-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D42366#1014157, @kosarev wrote: > I think zero would serve better as the unknown-size value. People who are not > aware of TBAA internals would guess that since zero-sized accesses make no > sense, they are likely to have some special

[PATCH] D34367: CodeGen: Fix address space of indirect function argument

2018-02-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3510 + args.add(RValue::getAggregate(Dest.getAddress()), type, L); } return; Please move all this conditionality to the call-emission code; just check whether you have a load of

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

2018-02-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1108 +PCK_ARCStrong, // objc strong pointer. +PCK_Struct // non-trivial C struct. + }; These should all be /// documentation comments, and they mostly shouldn't talk about fields

[PATCH] D43586: CodeGen: handle blocks correctly when inalloca'ed

2018-02-21 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I hate `inalloca` so much. Okay. Repository: rC Clang https://reviews.llvm.org/D43586 ___ cfe-commits mailing list

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

2018-01-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:239 + bit IncludeC = includeC; +} I have no objection to allowing ObjC attributes to be spelled in [[clang::objc_whatever]] style. We can debate giving them a more appropriate standard

[PATCH] D41562: [CodeGen] Generate TBAA info on passing arguments and returning values

2018-01-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Aren't these always loads and stores from allocas? I would expect TBAA to be useless here because it's always strictly less informative than basic-AA, which knows that the access doesn't alias with anything. Repository: rL LLVM https://reviews.llvm.org/D41562

[PATCH] D41539: [CodeGen] Decorate aggregate accesses with TBAA tags

2018-01-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. You're sure you just want a single TBAA tag on memcpy's? I would think that it might be useful to encode separate path information for the two operands. https://reviews.llvm.org/D41539 ___ cfe-commits mailing list

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