[PATCH] D63975: Warn when ScopeDepthOrObjCQuals overflows

2019-07-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. Thanks, LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63975/new/ https://reviews.llvm.org/D63975 ___ cfe-commits mailing list

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

2019-07-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! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63753/new/ https://reviews.llvm.org/D63753 ___

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-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. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64656/new/ https://reviews.llvm.org/D64656

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

2019-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks, just a few minor comment requests now. Comment at: include/clang/AST/DeclBase.h:1453 +/// copy. +uint64_t HasNonTrivialToPrimitiveCopyCUnion : 1; + Please include in these comments that these imply the associated basic

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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. Repository: rG LLVM Github Monorepo

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/test/CodeGenCXX/pr40771-ctad-with-lambda-copy-capture.cpp:2 +// RUN: %clang_cc1 -emit-obj --std=c++17 -fcxx-exceptions -fexceptions -O1 %s +// CHECK no crash + oydale wrote: > lebedev.ri wrote: > > Either add

[PATCH] D64656: Ensure placeholder instruction for cleanup is created

2019-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprAgg.cpp:1627 +llvm::Constant::getNullValue(CGF.Int8PtrTy), +CharUnits::One()); // placeholder + Please declare a lambda to add a cleanup which implicitly creates this dominator

[PATCH] D64569: [OpenCL] Improve destructor support in C++ for OpenCL

2019-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGClass.cpp:2016 CGF.EmitCXXDestructorCall(dtor, Dtor_Complete, /*for vbase*/ false, -/*Delegating=*/false, addr); +/*Delegating=*/false, addr, type); }

[PATCH] D62960: Add SVE opaque built-in types

2019-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Seems reasonable to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62960/new/ https://reviews.llvm.org/D62960 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D64400: [OpenCL][PR42390] Deduce addr space for templ specialization

2019-07-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. Okay. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64400/new/ https://reviews.llvm.org/D64400 ___ cfe-commits mailing list

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

2019-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Oh, yes, it definitely can't be done to class types. I suppose we should just forget about it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62584/new/ https://reviews.llvm.org/D62584 ___ cfe-commits mailing

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

2019-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D64083#1583109 , @rjmccall wrote: > 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

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

2019-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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

[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors

2019-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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

[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors

2019-07-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. Current patch LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62413/new/ https://reviews.llvm.org/D62413 ___ cfe-commits mailing

[PATCH] D63975: Warn when ScopeDepthOrObjCQuals overflows

2019-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. (Blocks don't actually allow default arguments, but apparently we still parse them, so we should test that path.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63975/new/ https://reviews.llvm.org/D63975 ___

[PATCH] D63975: Warn when ScopeDepthOrObjCQuals overflows

2019-07-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks. 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: template int foo(T

[PATCH] D63975: Warn when ScopeDepthOrObjCQuals overflows

2019-07-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:6587 +return; + } + Mordante wrote: > rjmccall wrote: > > Comment indentation. > > > > Should we do this when starting to parse a function prototype instead of > > when

[PATCH] D64400: [OpenCL][PR42390] Deduce addr space for templ specialization

2019-07-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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. CHANGES SINCE LAST ACTION

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

2019-07-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/TreeTransform.h:5363 +if (ResultType.getAddressSpace() != LangAS::Default && +(ResultType.getAddressSpace() != LangAS::opencl_private)) { SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

2019-07-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: cfe/trunk/lib/Sema/SemaExprCXX.cpp:4229 +LangAS AddrSpaceR = +RHSType->getAs()->getPointeeType().getAddressSpace(); +CastKind Kind = Anastasia wrote: > rjmccall wrote: > > Anastasia wrote: > > >

[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors

2019-07-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); Anastasia wrote: > rjmccall wrote: > >

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

2019-07-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:12053 +NTCUC_UninitAutoVar); } + ahatanak wrote: > rjmccall wrote: > > Please add a comment explaining why this is specific to local variables. > I was trying to

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

2019-07-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:6097 + // non-trivial to copy or default-initialize. + checkNonTrivialCUnionInInitList(ILE); + } ahatanak wrote: > rjmccall wrote: > > Can we extract a common function that checks the

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

2019-07-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

2019-07-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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

[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors

2019-07-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); rjmccall wrote: > Anastasia wrote: > >

[PATCH] D64400: [OpenCL][PR42390] Deduce addr space for templ specialization

2019-07-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaType.cpp:7421 + // - template specialization as addr space in template argument doesn't + // affect specialization. + (T->isDependentType() && (!T->isPointerType() && !T->isReferenceType() &&

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

2019-07-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: cfe/trunk/lib/Sema/SemaExprCXX.cpp:4229 +LangAS AddrSpaceR = +RHSType->getAs()->getPointeeType().getAddressSpace(); +CastKind Kind = Anastasia wrote: > rjmccall wrote: > > All of this can be much

[PATCH] D64400: [OpenCL][PR42390] Deduce addr space for templ specialization

2019-07-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/Type.h:6512 +inline bool Type::isTemplateSpecializationType() const { + return isa(this); +} This is a sugar type. What are you trying to do? Comment at:

[PATCH] D62584: [OpenCL][PR42033] Deducing addr space with template parameter types

2019-07-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/TreeTransform.h:5363 +if (ResultType.getAddressSpace() != LangAS::Default && +(ResultType.getAddressSpace() != LangAS::opencl_private)) { SemaRef.Diag(TL.getReturnLoc().getBeginLoc(),

[PATCH] D64083: [OpenCL][Sema] Improve address space support for blocks

2019-07-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: cfe/trunk/lib/Sema/SemaExprCXX.cpp:4229 +LangAS AddrSpaceR = +RHSType->getAs()->getPointeeType().getAddressSpace(); +CastKind Kind = All of this can be much simpler: ``` LangAS AddrSpaceL =

[PATCH] D64464: [CodeGen] Emit destructor calls for non-trivial C structs

2019-07-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:4100 + if (E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) +pushDestroy(QualType::DK_nontrivial_c_struct, DeclPtr, E->getType()); + Unfortunately, the lifetime of

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

2019-07-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks, that looks great. A few more requests and then this will be ready to go, I think. Comment at: include/clang/AST/DeclBase.h:1454 /// Number of non-inherited bits in RecordDeclBitfields. enum { NumRecordDeclBits = 11 };

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

2019-07-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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

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

2019-07-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! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63846/new/ https://reviews.llvm.org/D63846 ___ cfe-commits mailing list

[PATCH] D63912: [ObjC] Add a warning for implicit conversions of a constant non-boolean value to BOOL

2019-07-08 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3251 + "the only well defined values for BOOL are YES and NO">, + InGroup;

[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors

2019-07-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.h:274 +return LangAS::Default; + } + This target hook should just return the address space of the `__cxa_atexit` argument, not to claim anything specific about the relation between that

[PATCH] D63912: [ObjC] Add a warning for implicit conversions of a constant non-boolean value to BOOL

2019-07-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3251 + "the only well defined values for BOOL are YES and NO">, + InGroup; The `from` seems awkward here; `%0` is the number, not the type. (I agree that the type

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

2019-07-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D63846#1574311 , @vzakhari wrote: > In D63846#1574302 , @rjmccall wrote: > > > I don't know what I think about widespread use of > > `-fno-discard-value-names` for now; please continue

[PATCH] D63856: [ObjC] Add a -Wtautological-compare warning for BOOL

2019-07-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. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63856/new/ https://reviews.llvm.org/D63856 ___

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

2019-07-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D60456: [RISCV] Hard float ABI support

2019-07-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. Yes, I think you can commit. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60456/new/ https://reviews.llvm.org/D60456 ___

[PATCH] D60456: [RISCV] Hard float ABI support

2019-07-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I'm fine with proceeding with your best guess about what the ABI should be. Comment at: clang/lib/CodeGen/TargetInfo.cpp:9232 +if (IsFloat && Size > FLen) + return false; +// Can't be eligible if an integer type was already found (only

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

2019-07-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

2019-07-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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

[PATCH] D53295: Mark store and load of block invoke function as invariant.group

2019-07-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a reviewer: ahatanak. rjmccall added a comment. Herald added a subscriber: dexonsmith. Okay. Akira, do you have any interest in looking into this as a general block optimization? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53295/new/ https://reviews.llvm.org/D53295

[PATCH] D62960: Add SVE opaque built-in types

2019-07-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/AST/ItaniumMangle.cpp:2680 +break; +#include "clang/Basic/AArch64SVEACLETypes.def" } rsandifo-arm wrote: > rovka wrote: > > erik.pilkington wrote: > > > rsandifo-arm wrote: > > > > erik.pilkington wrote: > >

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

2019-07-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D64128#1569836 , @hfinkel wrote: > In D64128#1569817 , @rjmccall wrote: > > > The pointer/integer conversion is "implementation-defined", but it's not > > totally unconstrained. C

[PATCH] D62645: [Sema] Resolve placeholder types before type deduction to silence spurious `-Warc-repeated-use-of-weak` warnings

2019-07-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:1844 + let Documentation = [Undocumented]; +} + Oh, please add a comment on this explaining what it means, since it's not based on a language feature. Repository: rC Clang CHANGES

[PATCH] D62645: [Sema] Resolve placeholder types before type deduction to silence spurious `-Warc-repeated-use-of-weak` warnings

2019-07-03 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 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62645/new/ https://reviews.llvm.org/D62645 ___

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

2019-07-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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

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

2019-07-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1133 + /// Check if this is or contains a non-trivial C struct/union type. + bool hasNonTrivialPrimitiveCStruct() const; ahatanak wrote: > rjmccall wrote: > > You only want these checks to

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

2019-07-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks, this looks good to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53157/new/ https://reviews.llvm.org/D53157 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D64128: [CodeGen] Generate llvm.ptrmask instead of inttoptr(and(ptrtoint, C)) if possible.

2019-07-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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

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

2019-07-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1133 + /// Check if this is or contains a non-trivial C struct/union type. + bool hasNonTrivialPrimitiveCStruct() const; You only want these checks to trigger on unions with non-trivial

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

2019-07-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:16218 +checkNonTrivialCUnion(E->getType(), E->getExprLoc(), + Sema::NTCUC_LValueToRValueVolatile); + ahatanak wrote: > rjmccall wrote: > > ahatanak wrote: > > >

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

2019-07-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:259 +return DefaultConstrainedRounding.getValue(); + } + kpn wrote: > rjmccall wrote: > > Okay, so what are the invariants here now? It looks like, in order to > > enable constrained

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

2019-07-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:259 +return DefaultConstrainedRounding.getValue(); + } + Okay, so what are the invariants here now? It looks like, in order to enable constrained floating point, I need to also have

[PATCH] D63975: Warn when ScopeDepthOrObjCQuals overflows

2019-07-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:6587 +return; + } + Comment indentation. Should we do this when starting to parse a function prototype instead of when parsing a parameter? Repository: rG LLVM Github

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

2019-06-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:16218 +checkNonTrivialCUnion(E->getType(), E->getExprLoc(), + Sema::NTCUC_LValueToRValueVolatile); + ahatanak wrote: > rjmccall wrote: > > ahatanak wrote: > > >

[PATCH] D58164: Block+lambda: allow reference capture

2019-06-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Okay. In that case, yeah, it'd be good to get Richard's opinion about the right way to get that information here. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58164/new/ https://reviews.llvm.org/D58164

[PATCH] D62645: [Sema] Resolve placeholder types before type deduction to silence spurious `-Warc-repeated-use-of-weak` warnings

2019-06-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks, looks much better. Comment at: include/clang/Sema/Sema.h:2066 bool DeduceVariableDeclarationType(VarDecl *VDecl, bool DirectInit, - Expr *Init); + Expr *Init, bool

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

2019-06-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:16218 +checkNonTrivialCUnion(E->getType(), E->getExprLoc(), + Sema::NTCUC_LValueToRValueVolatile); + ahatanak wrote: > rjmccall wrote: > > It looks like you're

[PATCH] D56366: New warning call-to-virtual-from-ctor-dtor when calling a virtual function from a constructor or a destructor

2019-06-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Oh, sorry, I had completely forgotten about that. My contributions to Clang are primarily code review these days; I am not working on that idea. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56366/new/ https://reviews.llvm.org/D56366

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

2019-06-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Sema/Sema.h:2124 +// Implicitly initialized subobject. +NTCUC_ImplicitInitSubObject, +// Uninialized variable with automatic storage duration. "default-initialized", please.

[PATCH] D62413: [OpenCL][PR41727] Prevent ICE on global dtors

2019-06-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGDeclCXX.cpp:132 + Argument = CGM.getTargetCodeGenInfo().performAddrSpaceCast( + CGM, Addr.getPointer(), SrcAS, LangAS::opencl_global, DestTy); Anastasia wrote: > rjmccall wrote: > >

[PATCH] D56366: New warning call-to-virtual-from-ctor-dtor when calling a virtual function from a constructor or a destructor

2019-06-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaOverload.cpp:13075 + if ((isa(CurContext) || + isa(CurContext)) && + isa(MemExpr->getBase()->IgnoreParenCasts()) && Intentionally suppressed in lambdas? I think that might be reasonable, but

[PATCH] D63260: [Attr] Support _attribute__ ((fallthrough))

2019-06-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:1279 continue; - if (S.getLangOpts().CPlusPlus11) { + if (S.getLangOpts().CPlusPlus11 || S.getLangOpts().C99) { const Stmt *Term = B->getTerminatorStmt();

[PATCH] D62645: [Sema] Resolve placeholder types before type deduction to silence spurious `-Warc-repeated-use-of-weak` warnings

2019-06-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11101 + if (auto *placeholderType = Init->getType()->getAsPlaceholderType()) +if (placeholderType->getKind() == BuiltinType::PseudoObject) { + Res = CheckPlaceholderExpr(Init).get();

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

2019-06-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Are the `CaseStmt`s 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63139/new/

[PATCH] D53295: Mark store and load of block invoke function as invariant.group

2019-06-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Great, thank you. Yaxun, are you planning to pick this back up? I know it's been a long time. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53295/new/ https://reviews.llvm.org/D53295 ___ cfe-commits mailing

[PATCH] D58164: Block+lambda: allow reference capture

2019-06-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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? Repository: rC Clang CHANGES

[PATCH] D63856: [ObjC] Add a -Wtautological-compare warning for BOOL

2019-06-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D63856#1561132 , @erik.pilkington wrote: > In D63856#1561112 , @rjmccall wrote: > > > In D63856#1560213 , > > @erik.pilkington wrote: > > > >

[PATCH] D63856: [ObjC] Add a -Wtautological-compare warning for BOOL

2019-06-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D63856#1560213 , @erik.pilkington wrote: > In D63856#1560180 , @rjmccall wrote: > > > This only applies to relational operators, right? I'm a little > > uncomfortable with calling

[PATCH] D63856: [ObjC] Add a -Wtautological-compare warning for BOOL

2019-06-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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

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

2019-06-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11085 +if (isa(I)) + continue; +if (auto E = dyn_cast(I)) ahatanak wrote: > rjmccall wrote: > > Why is this okay? Don't we need to check default-initialization for these? > I didn't

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

2019-06-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:633 + "capture a variable of type %1}3 " + "since it %select{contains a union that |}2is non-trivial to " + "%select{default-initialize|destruct|copy}0">; Should this be

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

2019-06-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/llvm/IR/IntrinsicInst.h:235 + ebStrict ///< This corresponds to "fpexcept.strict". }; kpn wrote: > rjmccall wrote: > > kpn wrote: > > > rjmccall wrote: > > > > Is it okay that

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

2019-06-20 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D63451#1552609 , @rsmith wrote: > In D63451#1549563 , @rjmccall wrote: > > > Can this attribute not be applied to a base class, or to a type? > > > The standard attribute forbids that

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

2019-06-20 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:326 + // const llvm::Triple = Target.getTriple(); + code CustomCode = [{}]; } Thanks! Comment at: lib/CodeGen/CGExprAgg.cpp:1850

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

2019-06-19 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. Alright, thanks. I agree that per the documentation this should be sufficient. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63277/new/ https://reviews.llvm.org/D63277 ___

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

2019-06-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. This optimization is disabled for functions not in COMDAT sections? Is that documented somewhere? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63277/new/ https://reviews.llvm.org/D63277 ___ cfe-commits mailing

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

2019-06-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/AST/Decl.cpp:3937 + // -- [has] virtual member functions or virtual base classes, or + // -- has subobjects of nonzero size or bit-fields of nonzero length + if (const auto *CXXRD = dyn_cast(RD)) {

[PATCH] D63451: P0840R2: support for [[no_unique_address]] attribute

2019-06-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. 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

[PATCH] D59744: Fix i386 ABI "__m64" type bug

2019-06-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D59744#1549229 , @efriedma wrote: > 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

[PATCH] D59744: Fix i386 ABI "__m64" type bug

2019-06-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thank you. So it sounds like this patch needs to be reverted, and the correct version of it will have to insert these intrinsic calls in four places: - before translating vector arguments to MMX type before calls that pass `__m64` arguments, - after translating MMX

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

2019-06-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/llvm/IR/IntrinsicInst.h:235 + ebStrict ///< This corresponds to "fpexcept.strict". }; kpn wrote: > rjmccall wrote: > > Is it okay that `ebUnspecified` and `ebInvalid` overlap here? > I can

[PATCH] D59744: Fix i386 ABI "__m64" type bug

2019-06-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D59744#1548919 , @efriedma wrote: > > Now, we could theoretically use a different ABI rule for vectors defined > > with Clang-specific extensions, but that seems like it would cause quite a > > few problems of its own. > > I

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

2019-06-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/llvm/IR/IntrinsicInst.h:235 + ebStrict ///< This corresponds to "fpexcept.strict". }; Is it okay that `ebUnspecified` and `ebInvalid` overlap here? Comment at:

[PATCH] D59744: Fix i386 ABI "__m64" type bug

2019-06-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D59744#1548540 , @mtklein wrote: > Hey folks, I'm the Skia point of contact on this, and "luckily" the person > who wrote all the code that got us into this mess. Let me cross post a > couple questions I've had from the

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

2019-06-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Minor requests, then LGTM. Comment at: lib/CodeGen/CGExprConstant.cpp:73 +/// Incremental builder for an llvm::Constant* holding a structure constant. +class ConstantBuilder : private ConstantBuilderUtils { + llvm::SmallVector Elems;

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

2019-06-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:1138 + +return MetadataAsValue::get(Context, RoundingMDS); + } kpn wrote: > rjmccall wrote: > > Huh? You build an `MDNode` that wraps an `MDString` and then immediately > > extract

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

2019-06-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExprConstant.cpp:73 +/// Incremental builder for an llvm::Constant* holding a structure constant. +class ConstantBuilder : private ConstantBuilderUtils { + llvm::SmallVector Elems; This seems like a very

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

2019-06-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D63371#1546587 , @rsmith wrote: > In D63371#1546500 , @rjmccall wrote: > > > Isn't `[[no_unique_address]]` only significant for empty members? I'm not > > sure why they need

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

2019-06-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:113 +CR_ToZero ///< This corresponds to "fpround.tozero". + }; + kpn wrote: > rjmccall wrote: > > Should these have "FP" in the name somewhere? And are they really > >

[PATCH] D63371: Rewrite ConstStructBuilder with a mechanism that can cope with splitting and updating constants.

2019-06-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Isn't `[[no_unique_address]]` only significant for empty members? I'm not sure why they need significant support from constant-building, since they expand to no meaningful initializer. We have some code we'll hopefully be upstreaming soon that relies on being able

[PATCH] D53157: Teach the IRBuilder about constrained fadd and friends

2019-06-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/llvm/IR/IRBuilder.h:228 + /// Enable/Disable use of constrained floating point math + void setIsConstrainedFP(bool IsCon) { IsFPConstrained = IsCon; } + kpn wrote: > erichkeane wrote: > > kpn wrote: > > >

[PATCH] D62831: [CodeGen][ObjC] Add attribute "objc_arc_intert" to ObjC globals that are retain-agnostic

2019-06-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Please rename the test. There's also a typo in the differential title; please make sure that doesn't get into the commit message. Otherwise LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62831/new/

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

2019-06-13 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D62825#1542639 , @rsmith wrote: > In D62825#1542597 , @rjmccall wrote: > > > In D62825#1542374 , @rsmith wrote: > > > > > In D62825#1542309

  1   2   3   4   5   6   7   8   9   10   >