[PATCH] D46472: [HIP] Support offloading by linkscript

2018-05-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. The IRGen aspects of this seem reasonable, modulo a comment change. I don't feel qualified to judge the driver change. Comment at: lib/CodeGen/CGCUDANV.cpp:317 + if (GpuBinaryFileName.empty() && !IsHIP) return nullptr; Is

[PATCH] D45174: non-zero-length bit-fields should make a class non-empty

2018-05-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. Okay, LGTM. Repository: rC Clang https://reviews.llvm.org/D45174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[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/CodeGen/CGExprAgg.cpp:964 +RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer(); +break; + case TEK_Complex: It looks like we don't actually support any aggregate types here, which I think is fine

[PATCH] D46487: [HIP] Diagnose unsupported host triple

2018-05-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. LGTM. https://reviews.llvm.org/D46487 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46475: [HIP] Set proper triple and offload kind for the toolchain

2018-05-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Driver/Driver.cpp:554 + }) || + IsHIP) { const ToolChain *HostTC = C.getSingleOffloadToolChain(); It seems to me that it wouldn't be too hard to do your TODO here; it's basically just

[PATCH] D46471: [HIP] Add hip offload kind

2018-05-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Otherwise LGTM. Comment at: lib/Driver/Compilation.cpp:201 + // not compiled again if there are already failures. It is OK to abort the + // CUDA pipeline on errors. + if (A->isOffloading(Action::OFK_Cuda) || A->isOffloading(Action::OFK_HIP))

[PATCH] D46473: [HIP] Let clang-offload-bundler support HIP

2018-05-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. LGTM. https://reviews.llvm.org/D46473 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[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: include/clang/AST/ComparisonCategories.h:71 + /// standard library. The key is a value of ComparisonCategoryResult. + mutable llvm::DenseMap Objects; + EricWF wrote: > rjmccall wrote: > > We expect

[PATCH] D46489: [HIP] Let assembler output bitcode for amdgcn

2018-05-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I think the right solution here is to make a CompileJobAction with type TY_LLVM_BC in the first place. You should get the advice of a driver expert, though. https://reviews.llvm.org/D46489 ___ cfe-commits mailing list

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

2018-05-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D45476#1088189, @rsmith wrote: > This is significantly more complexity than we need. We're talking about > constexpr variables here, so we just need a `VarDecl* -- you can then ask > that declaration for its evaluated value and emit that

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

2018-05-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D45476#1088180, @EricWF wrote: > OK, As I see it, we have two choices: > > (1) Stash the expressions in Sema and import them like > > In https://reviews.llvm.org/D45476#1088047, @rjmccall wrote: > > > I'm sorry, but this is just not true.

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

2018-05-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1998 +``__ATOMIC_CONSUME``, ``__ATOMIC_ACQUIRE``, ``__ATOMIC_RELEASE``, +``__ATOMIC_ACQ_REL``, or ``__ATOMIC_SEQ_CST`` following C++11 memory model semantics. + Thank you for adding this

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

2018-05-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I agree that the format-specifier checker is not intended to be a portability checker. Any attempt to check portability problems has to account for two things: - Not all code is intended to be portable. If you're going to warn about portability problems, you need

[PATCH] D46475: [HIP] Set proper triple and offload kind for the toolchain

2018-05-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, LGTM. https://reviews.llvm.org/D46475 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46643: CodeGen: Emit string literal in constant address space

2018-05-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1373 + llvm::Type *BP = llvm::Type::getInt8Ty(CGM.getLLVMContext()) + ->getPointerTo(Loc.getAddressSpace()); if (Loc.getType() != BP) `CGM.Int8Ty` exists.

[PATCH] D46643: CodeGen: Emit string literal in constant address space

2018-05-14 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/D46643 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

2018-05-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. Fine, that works. https://reviews.llvm.org/D45476 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

2018-05-07 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks, the comments help a lot. Comment at: lib/CodeGen/CGObjCGNU.cpp:439 + ArrayRef IvarOffsets, + ArrayRef IvarAlign, + ArrayRef IvarOwnership);

[PATCH] D46613: _Atomic of empty struct shouldn't assert

2018-05-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. LGTM. Repository: rC Clang https://reviews.llvm.org/D46613 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D42933: [Sema] Avoid -Wformat warning for NSInteger/NSUInteger 'int' values with %zu/%zi long specifiers

2018-05-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D42933#1092077, @smeenai wrote: > Apple's current recommendation for using printf with the NSInteger types is > casting to a long, per >

[PATCH] D46613: _Atomic of empty struct shouldn't assert

2018-05-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/AST/ASTContext.cpp:1965 + Width = Target->getCharWidth(); + Align = Target->getCharWidth(); +} else if (Width <= Target->getMaxAtomicPromoteWidth()) { Alignment, unlike size, is definitely never 0. I

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

2018-05-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D45900#1093154, @yaxunl wrote: > In https://reviews.llvm.org/D45900#1083377, @rjmccall wrote: > > > 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

[PATCH] D46651: [OpenCL] Support placement new/delete in Sema

2018-05-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:2030 + } +} I think a better interpretation of this rule would be to just error on attempts to use the standard non-placement operator new/delete instead of trying to outlaw the

[PATCH] D46643: CodeGen: Emit string literal in constant address space

2018-05-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. The part about string literals looks fine, but: Comment at: lib/CodeGen/CGDecl.cpp:1375 +Loc = Address(EmitCastToVoidPtrInAllocaAddrSpace(Loc.getPointer()), + Loc.getAlignment()); I don't understand why a patch

[PATCH] D46218: PR37275 packed attribute should not apply to base classes

2018-04-27 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 wonder if that was originally just an oversight that got turned into official policy. Anyway, if it's the policy, it's what we have to do; LGTM. Have you checked whether we do the

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

2018-05-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExprConstant.cpp:1413 + } else if (!Init->isEvaluatable(CE.CGM.getContext())) { +return false; + } else if (InitTy->hasPointerRepresentation()) { sepavloff wrote: > rsmith wrote: > > sepavloff

[PATCH] D46665: [Itanium] Emit type info names with external linkage.

2018-05-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/ItaniumCXXABI.cpp:3098 +InfoLinkage = getTypeInfoLinkage(CGM, Ty); +NameLinkage = getTypeInfoLinkage(CGM, Ty, /*ForName*/ true); + } I think we could probably just have the function return both

[PATCH] D46665: [Itanium] Emit type info names with external linkage.

2018-05-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/ItaniumCXXABI.cpp:3098 +InfoLinkage = getTypeInfoLinkage(CGM, Ty); +NameLinkage = getTypeInfoLinkage(CGM, Ty, /*ForName*/ true); + } rjmccall wrote: > I think we could probably just have the

[PATCH] D46665: [Itanium] Emit type info names with external linkage.

2018-05-09 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. Looks really good, thanks. https://reviews.llvm.org/D46665 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

2018-05-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:1998 +``__ATOMIC_CONSUME``, ``__ATOMIC_ACQUIRE``, ``__ATOMIC_RELEASE``, +``__ATOMIC_ACQ_REL``, or ``__ATOMIC_SEQ_CST`` following C++11 memory model semantics. + rjmccall wrote: > Thank you

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

2018-05-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Can we suppress this optimization only when we can't emit an alias? An alias shouldn't degrade debugging experience, and it's good to emit less code at -O0. Repository: rC Clang https://reviews.llvm.org/D46685 ___

[PATCH] D46643: CodeGen: Emit string literal in constant address space

2018-05-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1375 +Loc = Address(EmitCastToVoidPtrInAllocaAddrSpace(Loc.getPointer()), + Loc.getAlignment()); yaxunl wrote: > rjmccall wrote: > > I don't understand why a patch about

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

2018-05-10 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/D46241 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

2018-05-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 works for me. The actual semantic parts of the diff seem to have disappeared from the patch posted to Phabricator, for what it's worth. Repository: rC Clang

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

2018-05-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. LGTM, thanks. Repository: rC Clang https://reviews.llvm.org/D46685 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46740: [Clang Tablegen][RFC] Allow Early Textual Substitutions in `Diagnostic` messages.

2018-05-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. This seems like a good idea to me. I wonder if it would be better to take advantage of this to shrink the string tables by preserving the substitution structure until runtime, but that

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

2018-05-07 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:964 +RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer(); +break; + case TEK_Complex: EricWF wrote: > EricWF wrote: > > rjmccall wrote: > > > It looks like we don't actually

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

2018-05-07 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()) sepavloff wrote: > rjmccall wrote: > > Do you actually care if

[PATCH] D46471: [HIP] Add hip offload kind

2018-05-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. Thanks, LGTM. https://reviews.llvm.org/D46471 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46685: [CodeGen] Disable structor optimizations at -O0

2018-05-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Well, internal and external types are important cases. I'm fine with this. It's a pity that we can't express what we want with aliases, though. Repository: rC Clang https://reviews.llvm.org/D46685 ___ cfe-commits

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

2018-05-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I agree that the new-expression case doesn't use the destructor, and all the other cases of list-initialization presumably use the destructor for the initialized type for separate reasons. Ok. Comment at:

[PATCH] D47096: CodeGen: block capture shouldn't ICE

2018-05-18 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/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47099: Disable casting of alloca for ActiveFlag

2018-05-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Maybe there should just be a method that makes a primitive alloca without the casting, and then you can call that in CreateTempAlloca. https://reviews.llvm.org/D47099 ___ cfe-commits mailing list

[PATCH] D47096: CodeGen: block capture shouldn't ICE

2018-05-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Test case should go in test/CodeGenCXX. Also, there already is a `blocks.cpp` there. Repository: rC Clang https://reviews.llvm.org/D47096 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47099: Disable casting of alloca for ActiveFlag

2018-05-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D47099#1105574, @yaxunl wrote: > In https://reviews.llvm.org/D47099#1105493, @rjmccall wrote: > > > Maybe there should just be a method that makes a primitive alloca without > > the casting, and then you can call that in CreateTempAlloca. >

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

2018-05-15 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. That would work. I think it would be reasonable for AutoVarEmission to store that pointer if it makes things easier. https://reviews.llvm.org/D45900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46489: [HIP] Let assembler output bitcode for amdgcn

2018-05-15 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D46489#1099979, @yaxunl wrote: > In https://reviews.llvm.org/D46489#1088940, @rjmccall wrote: > > > I think the right solution here is to make a CompileJobAction with type > > TY_LLVM_BC in the first place. You should get the advice of a

[PATCH] D46665: [Itanium] Emit type info names with external linkage.

2018-05-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I believe static and dynamic linkers — at least on ELF and Mach-O — will always drop weak symbols for strong ones. Now, I think that isn't LLVM's posted semantics for linkonce_odr, but to me that means that LLVM's semantics are inadequate, not that we should decline

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

2018-05-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D44539#1106443, @aaron.ballman wrote: > In https://reviews.llvm.org/D44539#1106152, @rjmccall wrote: > > > This was approved by the Objective-C language group as a default-off > > warning. > > > We usually do not expose new default-off

[PATCH] D47099: Disable casting of alloca for ActiveFlag

2018-05-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:80 auto Alloca = CreateTempAlloca(Ty, Name, ArraySize); Alloca->setAlignment(Align.getQuantity()); if (AllocaAddr) Could you change this to call CreateTempAllocaWithoutCast?

[PATCH] D47108: [CodeGenCXX] Add -fforce-emit-vtables

2018-05-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D47108#1109145, @Prazek wrote: > In https://reviews.llvm.org/D47108#1109014, @rjmccall wrote: > > > I thought we already had places in Sema that marked inline virtual methods > > as used, instantiated templates, etc. for devirtualization

[PATCH] D47108: [CodeGenCXX] Add -fforce-emit-vtables

2018-05-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I thought we already had places in Sema that marked inline virtual methods as used, instantiated templates, etc. for devirtualization purposes when optimization was enabled. Did we rip that out? The problem we've had over and over with devirtualization is that we

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

2018-05-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. There are at least three good reasons to make sure this can enabled/disabled by a flag: - We have to anticipate that introducing new keywords will cause some compatibility problems. New language standards that introduce new keywords can be disabled using `-std=`.

[PATCH] D47103: Implement strip.invariant.group

2018-05-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. The changes to Clang generally seem reasonable, but I think you should split them into a separate commit from the commit that adds the intrinsic itself. Comment at: clang/lib/CodeGen/CGExpr.cpp:3858 +} + } + Please add a comment

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

2018-05-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added a reviewer: akyrtzi. rjmccall added a comment. CC: Argyrios for the USR question. Repository: rC Clang https://reviews.llvm.org/D46084 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46084: [Fixed Point Arithmetic] Addition of the Fixed Point _Accum type

2018-05-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/DiagnosticCommonKinds.td:172 +def err_fixed_point_only_allowed_in_c : Error< + "Fixed point types are only allowed in C">; leonardchan wrote: > leonardchan wrote: > > leonardchan wrote: > > >

[PATCH] D47103: Implement strip.invariant.group

2018-05-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3858 +} + } + Prazek wrote: > rjmccall wrote: > > Please add a comment explaining why this is necessary. (I'm actually not > > sure why it is, because surely the invariant groups we

[PATCH] D47166: use zeroinitializer for (trailing zero portion of) large array initializers more reliably

2018-05-22 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. LGTM, thanks! Repository: rC Clang https://reviews.llvm.org/D47166 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47354: [CodeGen][Darwin] Set the calling-convention of a thread-local variable initialization function to fix calling-convention mismatch

2018-05-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/D47354 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

2018-05-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. It's waiting on a discussion that's happening pretty slowly, sorry. I know this is frustrating. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47096: CodeGen: block capture shouldn't ICE

2018-05-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. RecursiveASTVisitor instantiations are huge. Can you just make the function take a Stmt and then do the first few checks if it happens to be an Expr? Repository: rC Clang https://reviews.llvm.org/D47096 ___

[PATCH] D46665: [Itanium] Emit type info names with external linkage.

2018-05-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D46665#1102348, @rsmith wrote: > In https://reviews.llvm.org/D46665#1102290, @rjmccall wrote: > > > I believe static and dynamic linkers — at least on ELF and Mach-O — will > > always drop weak symbols for strong ones. Now, I think that

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

2018-05-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. https://reviews.llvm.org/D45900 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D47166: use zeroinitializer for (trailing zero portion of) large array initializers more reliably

2018-05-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I like this approach a lot. Comment at: lib/CodeGen/CGExprConstant.cpp:675 + // We have mixed types. Use a packed struct. + std::vector Types; + Types.reserve(Elements.size()); Why std::vector? Repository: rC Clang

[PATCH] D47092: downgrade strong type info names to weak_odr linkage

2018-05-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: test/CodeGenCXX/arm64.cpp:48 void A::foo() {} - // CHECK-GLOBALS-DAG: @_ZTSN5test21AE = constant [11 x i8] + // CHECK-GLOBALS-DAG: @_ZTSN5test21AE = weak_odr constant [11 x i8] // CHECK-GLOBALS-DAG: @_ZTIN5test21AE = constant {

[PATCH] D47096: CodeGen: block capture shouldn't ICE

2018-05-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D47096#1105374, @jfb wrote: > In https://reviews.llvm.org/D47096#1105368, @rjmccall wrote: > > > RecursiveASTVisitor instantiations are huge. Can you just make the > > function take a Stmt and then do the first few checks if it happens to

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

2018-05-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. This was approved by the Objective-C language group as a default-off warning. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:1018 +def warn_objc_property_assign_on_object : Warning< + "'assign' attribute must not be of object type, use

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

2018-05-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. This isn't really an Objective-C user forum, but I'll try to summarize briefly. `unsafe_unretained` is an unsafe version of `weak` — it's unsafe because it can be left dangling if the object is deallocated. It's necessary for a small (and getting smaller every year)

[PATCH] D47092: downgrade strong type info names to weak_odr linkage

2018-05-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Incomplete classes are a curse. I don't suppose we can just modify the language specification to make it illegal to use `typeid(Incomplete*)`? Or make equality/hashing undefined in these cases? Comment at: test/CodeGenCXX/arm64.cpp:48 void

[PATCH] D46740: [Clang Tablegen][RFC] Allow Early Textual Substitutions in `Diagnostic` messages.

2018-05-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: utils/TableGen/ClangDiagnosticsEmitter.cpp:514 + std::vector Diags = Records.getAllDerivedDefinitions("Diagnostic"); + llvm::for_each(Diags, [&](Record *R) { substituteDiagText(R, SubsMap); }); EricWF wrote: >

[PATCH] D47092: downgrade strong type info names to weak_odr linkage

2018-05-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D47092#1105317, @rjmccall wrote: > Incomplete classes are a curse. I don't suppose we can just modify the > language specification to make it illegal to use `typeid(Incomplete*)`? Or > make equality/hashing undefined in these cases?

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

2018-05-20 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, my comments seem to all be addressed. Repository: rC Clang https://reviews.llvm.org/D46052 ___ cfe-commits mailing list

[PATCH] D47099: Call CreateTempAllocaWithoutCast for ActiveFlag

2018-05-22 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/D47099 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

2018-06-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaObjCProperty.cpp:2554 + PropertyTy->isObjCRetainableType() && + !PropertyTy->isObjCClassType()) { +Diag(Loc, diag::warn_objc_property_assign_on_object); Please use

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

2018-06-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D47627#1121970, @ebevhan wrote: > There's actually a bit more to it than in these two patches. The complete > diff to this function in our downstream Clang looks like this: > >QualType >ASTContext::getAddrSpaceQualType(QualType T,

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

2018-06-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a reviewer: t.p.northover. rjmccall added a comment. Okay, as a code change this seems more reasonable to me. I haven't really thought through the ABI-compatibility issues, though. CC'ing Tim. https://reviews.llvm.org/D46013 ___

[PATCH] D46651: [OpenCL] Support new/delete in Sema

2018-06-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:13010 + if (SemaRef.getLangOpts().OpenCLCPlusPlus) { +if (auto *PtrTy = ResultType.getTypePtr()->getAs()) { + ResultType = RemoveAddressSpaceFromPtr(SemaRef, PtrTy); `getTypePtr()` is

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

2018-06-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D47233#1129110, @smeenai wrote: > Wrapping an Objective-C exception inside a C++ exception means dynamically > constructing a C++ exception object and traversing the class hierarchy of the > thrown Obj-C object to populate the catchable

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

2018-06-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. The non-fragile Objective-C path — i.e. interoperation with C++ exceptions instead of using `setjmp`/`longjmp` in an utterly-incompatible style — is absolutely the right direction going forward. How does "wrapping an Objective-C exception inside a C++ exception" work?

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

2018-06-12 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:784 +return !hasDefinition() || !isDynamicClass() || hasAnyDependentBases(); + } + Both of these new methods deserve doc comments explaining that they're conservative checks

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

2018-06-18 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] D47988: [CodeGen] Emit MSVC funclet IR for Obj-C exceptions

2018-06-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In general, it's unfortunate that this has to leave so many C++-runtime-specific tendrils in the ObjC code. Unlike the EH type patch, though, I'm not sure I can see a great alternative here, especially because of the semantic restrictions required by outlining.

[PATCH] D47988: [CodeGen] Emit MSVC funclet IR for Obj-C exceptions

2018-06-18 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D47988#1135929, @rnk wrote: > In https://reviews.llvm.org/D47988#1135533, @rjmccall wrote: > > > In general, it's unfortunate that this has to leave so many > > C++-runtime-specific tendrils in the ObjC code. Unlike the EH type patch, > >

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

2018-06-15 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1623 + const CXXRecordDecl *SourceClassDecl = + E->getType().getTypePtr()->getPointeeCXXRecordDecl(); + const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl();

[PATCH] D48040: Implement constexpr __builtin_*_overflow

2018-06-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/AST/ExprConstant.cpp:8260 + // It won't GROW, since that isn't possible, so use this to allow + // TruncOrSelf. + APSInt Temp = Result.extOrTrunc(Info.Ctx.getTypeSize(ResultType)); The comment should

[PATCH] D47733: [CUDA][HIP] Set kernel calling convention before arrange function

2018-06-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. Ok. https://reviews.llvm.org/D47733 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D46651: [OpenCL] Support new/delete in Sema

2018-06-11 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. LGTM. Comment at: lib/Sema/SemaExprCXX.cpp:2030 + } +} svenvh wrote: > rjmccall wrote: > > Anastasia wrote: > > > svenvh wrote: > > > > rjmccall wrote: > > > > > I think a better

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

2018-06-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D47627#1127716, @ebevhan wrote: > > Well, the documentation mismatch is worth fixing even if the code isn't. > > But I think at best your use-case calls for weakening the assertion to be > > that any existing address space isn't

[PATCH] D48040: Implement constexpr __builtin_*_overflow

2018-06-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks, comment change looks good. LGTM. https://reviews.llvm.org/D48040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-06-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Why not just have the driver disable RTTI in the frontend invocation? https://reviews.llvm.org/D47694 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-06-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D46042#1121648, @rnk wrote: > It's the typedef alignment changes that are causing problems for us, not the > MaxVectorAlign changes. That makes more sense. The new alignment attribute > breaks our implementation of `_mm256_loadu_ps`,

[PATCH] D47564: [Parse] Use CapturedStmt for @finally on MSVC

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. Okay, We can try this, then. Repository: rC Clang https://reviews.llvm.org/D47564 ___ cfe-commits mailing list

[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. Oh, I see, because you're worried that the host code might contain `dynamic_cast` or similar features which would complain if RTTI were disabled. `getLangOpts().CUDAIsDevice` implies `getLangOpts().CUDA`, so I think you can just check the former. Otherwise LGTM.

[PATCH] D47564: [Parse] Use CapturedStmt for @finally on MSVC

2018-05-31 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D47564#1118423, @smeenai wrote: > In https://reviews.llvm.org/D47564#1118381, @rjmccall wrote: > > > That's an interesting idea. I don't see any particular reason not to do it > > this way if you're willing to accept that it's never going

[PATCH] D47564: [Parse] Use CapturedStmt for @finally on MSVC

2018-05-31 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. That's an interesting idea. I don't see any particular reason not to do it this way if you're willing to accept that it's never going to support the full control-flow possibilities of @finally. You will need to add JumpDiagnostics logic to prevent branches out of

[PATCH] D47564: [Parse] Use CapturedStmt for @finally on MSVC

2018-06-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D47564#1119874, @smeenai wrote: > In https://reviews.llvm.org/D47564#1118424, @rjmccall wrote: > > > No, it was just a general question. Have you gotten this to a point where > > it's testable? > > > Yup, it's been working fine in my local

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

2018-06-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I'm not sure it's appropriate to impose this as an overhead on all targets. https://reviews.llvm.org/D46013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2018-06-01 Thread John McCall via Phabricator via cfe-commits
rjmccall closed this revision. rjmccall added a comment. Landed as r333791. Repository: rC Clang https://reviews.llvm.org/D46042 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46013: [ARM] Conform to AAPCS when passing overaligned composites as arguments

2018-06-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D46013#1119513, @efriedma wrote: > > I'm not sure it's appropriate to impose this as an overhead on all targets. > > You mean the overhead of increasing the size of TypeInfo? That's fair. Yeah. It seems like something that could be pretty

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

2018-06-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Is there a specific reason to change the implementation instead of changing the documentation? Repository: rC Clang https://reviews.llvm.org/D47627 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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