[PATCH] D29208: Prevent ICE in dllexport class with _Atomic() data member

2017-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. Thanks, looks good. https://reviews.llvm.org/D29208 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29208: Prevent ICE in dllexport class with _Atomic() data member

2017-02-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGClass.cpp:1135 MemberExpr *ME2 = dyn_cast(RHS); -if (dyn_cast(ME2->getMemberDecl()) != Field) +if (!ME2 || dyn_cast(ME2->getMemberDecl()) != Field) return nullptr;

[PATCH] D29208: Prevent ICE in dllexport class with _Atomic() data member

2017-02-01 Thread John McCall via Phabricator via cfe-commits
rjmccall requested changes to this revision. rjmccall added inline comments. This revision now requires changes to proceed. Comment at: lib/CodeGen/CGClass.cpp:1135 MemberExpr *ME2 = dyn_cast(RHS); -if (dyn_cast(ME2->getMemberDecl()) != Field) +if (!ME2

[PATCH] D27680: [CodeGen] Move lifetime.start of a variable when goto jumps back past its declaration

2017-01-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:217 + /// statements. + llvm::SmallVector LabelSeenStack; + Shouldn't this be maintained by some existing scoping structure like LexicalScope? Comment at:

[PATCH] D27680: [CodeGen] Move lifetime.start of a variable when goto jumps back past its declaration

2017-01-24 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:217 + /// statements. + llvm::SmallVector LabelSeenStack; + ahatanak wrote: > ahatanak wrote: > > rjmccall wrote: > > > Shouldn't this be maintained by some existing scoping

[PATCH] D27680: [CodeGen] Move lifetime.start of a variable when goto jumps back past its declaration

2017-01-25 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D27680#656453, @ahatanak wrote: > Yes, we can make VarBypassDetector detect variables declared after labels too > if we want to put all the logic for disabling lifetime markers in one place. Okay. If that's straightforward, that does seem

[PATCH] D27680: [CodeGen] Move lifetime.start of a variable when goto jumps back past its declaration

2017-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. Thanks. Some minor changes and then LGTM. I'm currently wondering if a better solution might be to just teach the Bypasses analysis about the C lifetime rule. But we don't need to do

[PATCH] D29101: Ignore -f(no)objc-arc-exception when -fno-objc-arc set

2017-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. Yes, that seems reasonable. https://reviews.llvm.org/D29101 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D24812: Lit C++11 Compatibility Patch #11

2017-02-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: test/CodeGenCXX/static-init.cpp:14 +// CHECK98: @_ZZN5test414useStaticLocalEvE3obj = linkonce_odr global %"struct.test4::HasVTable" zeroinitializer, comdat, align 8 +// CHECK11: @_ZZN5test414useStaticLocalEvE3obj = linkonce_odr global

[PATCH] D29986: Fix crash when an incorrect redeclaration only differs in __unaligned type-qualifier

2017-02-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. LGTM. https://reviews.llvm.org/D29986 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29986: Fix crash when an incorrect redeclaration only differs in __unaligned type-qualifier

2017-02-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D29986#684811, @rogfer01 wrote: > Ping? Are you expecting something even more conclusive than "looks good to me"? Because I sent you that feedback a week ago. :) https://reviews.llvm.org/D29986

[PATCH] D29739: Make Lit tests C++11 compatible - Objective-C++

2017-02-13 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D29739#675277, @probinson wrote: > In https://reviews.llvm.org/D29739#674297, @rjmccall wrote: > > > In https://reviews.llvm.org/D29739#674288, @probinson wrote: > > > > > I really think Apple would need to step up here if the default > > >

[PATCH] D24812: Lit C++11 Compatibility Patch #11

2017-02-13 Thread John McCall via Phabricator via cfe-commits
rjmccall added a reviewer: rnk. rjmccall added a comment. Generally looks good to me, thanks. One question for Reid. Comment at: test/CodeGenCXX/static-init.cpp:14 +// CHECK98: @_ZZN5test414useStaticLocalEvE3obj = linkonce_odr global %"struct.test4::HasVTable"

[PATCH] D29739: Make Lit tests C++11 compatible - Objective-C++

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

[PATCH] D29739: Make Lit tests C++11 compatible - Objective-C++

2017-02-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Hmm. Was there discussion about that? I see no reason not to bump ObjC++. https://reviews.llvm.org/D29739 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29739: Make Lit tests C++11 compatible - Objective-C++

2017-02-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D29739#673933, @tigerleapgorge wrote: > Hi John, > > Here is the most recent discussion I can find on cfe-dev. > “I'm guessing that Objective-C/C++ is kind of passe, so nobody is really > interested in modernizing it” >

[PATCH] D29859: Make Lit tests C++11 compatible - nounwind noexcept

2017-02-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. LGTM. https://reviews.llvm.org/D29859 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29739: Make Lit tests C++11 compatible - Objective-C++

2017-02-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D29739#674288, @probinson wrote: > In https://reviews.llvm.org/D29739#673971, @rjmccall wrote: > > > In https://reviews.llvm.org/D29739#673933, @tigerleapgorge wrote: > > > > > Hi John, > > > > > > Here is the most recent discussion I can

[PATCH] D29908: Disallow returning a __block variable via a move

2017-02-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D29908#676601, @ahatanak wrote: > In https://reviews.llvm.org/D29908#676589, @ahatanak wrote: > > > Posting John's review comment, which was sent to the review I abandoned: > > > > "Hmm. The behavior I think we actually want here is that we

[PATCH] D24812: Lit C++11 Compatibility Patch #11

2017-02-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D24812#678931, @tigerleapgorge wrote: > @rjmccall > > Hi John, I've made the changes to volatile.cpp. > I take it this patch is good for commit? Yes, I think I have the information I wanted from Reid. LGTM.

[PATCH] D28691: Support synchronisation scope in Clang atomic builtin functions

2017-01-17 Thread John McCall via Phabricator via cfe-commits
rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. This patch changes the language design of the atomic builtins, which is outside the normal scope of patch review. You need to post an RFC to cfe-dev. I've gone ahead and made

[PATCH] D30345: [CodeGen][Blocks] Refactor capture handling in code that generates block copy/destroy routines

2017-02-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. You're doing this refactor to... support doing another refactor of the same code? Why are these patches separate? Repository: rL LLVM https://reviews.llvm.org/D30345 ___ cfe-commits mailing list

[PATCH] D30430: Make Lit tests C++11 compatible - IR ordering

2017-02-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. The C++98 behavior here is not really vital to test precisely; it's just minor differences in what gets instantiated and when. I think it's fine to just update the run line to -std=c++11 for things like this. But if you really want to test both configurations, this

[PATCH] D25556: [Sema] Add variable captured by a block to the enclosing lambda's potential capture list

2017-02-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Well, I was hoping that Richard would weigh in, but absent that, I don't have any objections. Since it fixes a known bug, let's just go ahead and do it. LGTM. https://reviews.llvm.org/D25556 ___ cfe-commits mailing

[PATCH] D30345: [CodeGen][Blocks] Refactor capture handling in code that generates block copy/destroy routines

2017-03-01 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks, LGTM. Repository: rL LLVM https://reviews.llvm.org/D30345 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30345: [CodeGen][Blocks] Refactor capture handling in code that generates block copy/destroy routines

2017-02-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D30345#688298, @arphaman wrote: > In https://reviews.llvm.org/D30345#688144, @rjmccall wrote: > > > You're doing this refactor to... support doing another refactor of the same > > code? Why are these patches separate? > > > Not quite, by

[PATCH] D27157: IRGen: Remove all uses of CreateDefaultAlignedLoad.

2016-11-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuilder.h:126 // FIXME: these "default-aligned" APIs should be removed, // but I don't feel like fixing all the builtin code right now. llvm::StoreInst *CreateDefaultAlignedStore(llvm::Value *Val,

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

2016-11-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: test/CodeGenCXX/return.cpp:21 + // CHECK-NOSTRICT-NEXT: load + // CHECK-NOSTRICT-NEXT: ret i32 + // CHECK-NOSTRICT-NEXT: } mehdi_amini wrote: > Quuxplusone wrote: > > Can you explain why a load from an uninitialized

[PATCH] D27157: IRGen: Remove all uses of CreateDefaultAlignedLoad.

2016-11-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuilder.h:126 // FIXME: these "default-aligned" APIs should be removed, // but I don't feel like fixing all the builtin code right now. llvm::StoreInst *CreateDefaultAlignedStore(llvm::Value *Val,

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

2016-11-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D27163#607100, @rsmith wrote: > In https://reviews.llvm.org/D27163#607078, @rsmith wrote: > > > A target-specific default for this, simply because there's a lot of code on > > Darwin that happens to violate this language rule, doesn't make

[PATCH] D27157: IRGen: Remove all uses of CreateDefaultAlignedLoad.

2016-11-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks for doing this! A couple minor questions / comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2195 LoadInst *Load = -Builder.CreateDefaultAlignedLoad(IntToPtr, /*isVolatile=*/true); +Builder.CreateAlignedLoad(IntTy,

[PATCH] D26196: Add support for non-zero null pointer for C and OpenCL

2016-12-07 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1532 + return llvm::ConstantInt::get(ConvertType(DestTy), + CGF.getContext().getTargetNullPtrValue(E->getType())); assert(!DestTy->isBooleanType() && "bool should use PointerToBool");

[PATCH] D26196: Add support for non-zero null pointer for C and OpenCL

2016-12-08 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:1532 + return llvm::ConstantInt::get(ConvertType(DestTy), + CGF.getContext().getTargetNullPtrValue(E->getType())); assert(!DestTy->isBooleanType() && "bool should use PointerToBool");

[PATCH] D27627: Allow target to specify default address space for codegen

2016-12-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Ok, I'm not understanding this, then. I declare a local variable: int x; According to you, x should be in the generic address space, i.e. the private address space. And at the LLVM IR level, this will be implemented with an AllocaInst, which always creates memory

[PATCH] D27627: Allow target to specify default address space for codegen

2016-12-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Because, notably, if you do that, then an attempt to pass as an int* will fail, which means this isn't really C++ anymore... and yet that appears to be exactly what you want. https://reviews.llvm.org/D27627 ___

[PATCH] D27627: Allow target to specify default address space for codegen

2016-12-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I suspect that what you actually want to do in order to implement HCC is change SemaType so that 'int*' is implicitly interpreted as 'int __something *'. But I don't know how not having explicit address spaces actually works in the HCC language design, given that

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

2016-12-15 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenFunction.cpp:1078 + QualType T = FD->getReturnType(); + if (T.isTriviallyCopyableType(Context)) { +// Avoid the optimization for functions that return trivially copyable arphaman wrote: >

[PATCH] D22296: CodeGen: New vtable group representation: struct of vtable arrays.

2016-12-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, thanks! https://reviews.llvm.org/D22296 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D27627: Allow target to specify default address space for codegen

2016-12-13 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D27627#619928, @yaxunl wrote: > > Because, notably, if you do that, then an attempt to pass as an int* > > will fail, which means this isn't really C++ anymore... and yet that > > appears to be exactly what you want. > > How about when

[PATCH] D26196: Add support for non-zero null pointer for C and OpenCL

2016-12-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:306 + /// \param AddrSpace address space of pointee in source language. + virtual uint64_t getNullPtrValue(unsigned AddrSpace) const { +return 0; We normally spell out "Pointer",

[PATCH] D26196: Add support for non-zero null pointer for C and OpenCL

2016-12-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. Okay. With that resolved, this LGTM. https://reviews.llvm.org/D26196 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D22296: CodeGen: New vtable group representation: struct of vtable arrays.

2016-12-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. A couple minor suggestions, then LGTM. Comment at: clang/include/clang/AST/VTableBuilder.h:255 +operator ArrayRef() const { return {data(), size()}; }; + }; + Maybe this ought to be in LLVM as OwnedArrayRef? And the more minimal

[PATCH] D27627: Allow target to specify default address space for codegen

2016-12-09 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. What address space should local variables be in? https://reviews.llvm.org/D27627 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27680: [CodeGen] Move lifetime.start of a variable when goto jumps back past its declaration

2017-01-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Interesting. That's a pretty heavyweight solution, and you're still missing switches, which have exactly the same "can jump into an arbitrary variable's scope" behavior. I think maybe an easier solution would be to just remove the begin-lifetime marker completely

[PATCH] D28425: Lit C++11 Compatibility Patch - nonthrowing destructors

2017-01-06 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 good to me. https://reviews.llvm.org/D28425 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D27680: [CodeGen] Move lifetime.start of a variable when goto jumps back past its declaration

2017-01-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:917 + if (!InsertPt) +Builder.SetInsertPoint(BB, BB->begin()); + // If InsertPt is a terminator, insert it before InsertPt. ahatanak wrote: > ahatanak wrote: > > rjmccall wrote:

[PATCH] D28310: Add virtual functions getter

2017-01-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. This is a potentially significant pessimization. Can you turn this into a range iterator of some sort? https://reviews.llvm.org/D28310 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D28148: [Sema] Suppress warnings for C's zero initializer

2016-12-29 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D28148#632541, @Quuxplusone wrote: > Re the language option: In https://reviews.llvm.org/D27163, @rsmith wrote: > > > Perhaps some catch-all "I want defined behavior for things that C defines > > even though I'm compiling in C++" flag would

[PATCH] D27994: Make two vtable tests tolerate C++11

2016-12-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. No, seems fine to me. https://reviews.llvm.org/D27994 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D27936: C++11 test cleanup: nonthrowing destructors

2016-12-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. Nicely done. https://reviews.llvm.org/D27936 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D27680: [CodeGen] Move lifetime.start of a variable when goto jumps back past its declaration

2016-12-20 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Wouldn't it be simpler to just record an insertion point for the beginning of the current lexical scope and insert the lifetime.begin there instead of at the current IP? Comment at: lib/CodeGen/CodeGenFunction.h:351 llvm::Value *Size; +

[PATCH] D27680: [CodeGen] Move lifetime.start of a variable when goto jumps back past its declaration

2016-12-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D27680#629182, @ahatanak wrote: > In https://reviews.llvm.org/D27680#628272, @rjmccall wrote: > > > Wouldn't it be simpler to just record an insertion point for the beginning > > of the current lexical scope and insert the lifetime.begin

[PATCH] D27680: [CodeGen] Move lifetime.start of a variable when goto jumps back past its declaration

2016-12-24 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D27680#630192, @ahatanak wrote: > Ah, good idea. That sounds like a much simpler and less invasive approach. I > agree that the performance impact would probably be small, and if it turns > out to have a significant impact, we can reduce

[PATCH] D27955: Make CodeGenCXX/arm-swiftcall.cpp tolerate C++11

2016-12-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Yes, I think being more specific about the dialect would be better. https://reviews.llvm.org/D27955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27955: Make CodeGenCXX/arm-swiftcall.cpp tolerate C++11

2016-12-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. LGTM, thanks! https://reviews.llvm.org/D27955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D24431: CodeGen: Start using inrange annotations on vtable getelementptr.

2016-12-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. I'm not sure I like this IR design, but this use of it seems fine. LGTM. https://reviews.llvm.org/D24431 ___ cfe-commits mailing list

[PATCH] D22296: CodeGen: New vtable group representation: struct of vtable arrays.

2016-12-07 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. This seems reasonable to me. Just a few representational / API requests. Comment at: clang/include/clang/AST/VTableBuilder.h:222 + typedef llvm::DenseMap> + AddressPointsMapTy;

[PATCH] D26196: Add support for non-zero null pointer for C and OpenCL

2016-12-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/AST/APValue.h:379 void setLValue(LValueBase B, const CharUnits , NoLValuePath, - unsigned CallIndex); + unsigned CallIndex, bool IsNuppPtr); void setLValue(LValueBase B, const

[PATCH] D27627: [WIP] Supporting C++ based kernel languages on AMDGPU Target

2017-03-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Okay. That does seem more sensible than trying to change everything around the other way. https://reviews.llvm.org/D27627 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31007: [Objective-C] Miscellaneous -fobjc-weak Fixes

2017-03-24 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaInit.cpp:6681 // full-expression's cleanups. - if ((S.getLangOpts().ObjCAutoRefCount && - MTE->getType()->isObjCLifetimeType()) || + if (MTE->getType()->isNonTrivialObjCLifetimeType() ||

[PATCH] D31007: [Objective-C] Miscellaneous -fobjc-weak Fixes

2017-03-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, thanks! https://reviews.llvm.org/D31007 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-24 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Just a couple of minor requests. Comment at: lib/Sema/SemaExpr.cpp:708 + if (getLangOpts().ObjCWeak && E->getType().getObjCLifetime() == Qualifiers::OCL_Weak) Cleanup.setExprNeedsCleanups(true); Much like the other

[PATCH] D29599: Clang Changes for alloc_align

2017-03-30 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:4363 +} else if (AllocAlignParam && TargetDecl->hasAttr()) + EmitAlignmentAssumption(Ret.getScalarVal(), AllocAlignParam); } rjmccall wrote: > Your old code was fine, you just needed

[PATCH] D29599: Clang Changes for alloc_align

2017-03-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. Thanks. With that, LGTM. https://reviews.llvm.org/D29599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29599: Clang Changes for alloc_align

2017-03-29 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I see that GCC is up to its same parameter-indexing shenanigans again. Comment at: include/clang/Basic/AttrDocs.td:252 +declaration to specify that the return value of the function (which must be a +pointer type) has an alignment specified by the

[PATCH] D29599: Clang Changes for alloc_align

2017-03-29 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Basic/AttrDocs.td:252 +declaration to specify that the return value of the function (which must be a +pointer type) is at least as aligned as the value indicated parameter. The +parameter is given by its index in the

[PATCH] D31044: Update for alloca construction changes

2017-03-28 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/D31044 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31005: [Objective-C] Fix "repeated use of weak" warning with -fobjc-weak

2017-03-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, thanks. https://reviews.llvm.org/D31005 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D30809: [coroutines] Add codegen for await and yield expressions

2017-03-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. Looks great, thanks. https://reviews.llvm.org/D30809 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31003: [Objective-C] C++ Classes with __weak Members non-POD Types when using -fobjc-weak

2017-03-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D31003#711359, @bkelley wrote: > Thank you @rjmccall for the approval. I don't have commit access; would > someone be willing to commit this path for me please? Thanks! You have a lot of patches here. :) I would encourage you to just ask

[PATCH] D31004: [Objective-C] Fix __weak type traits with -fobjc-weak

2017-03-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. Looks great, thanks! https://reviews.llvm.org/D31004 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D30977: [CodeGen] Emit a CoreFoundation link guard when @available is used

2017-03-17 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. Comment at: lib/CodeGen/CGObjC.cpp:3428 + // CoreFoundation is not used in the code, the linker won't link the + // framework. + auto = getLLVMContext();

[PATCH] D31004: [Objective-C] Fix __weak type traits with -fobjc-weak

2017-03-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I have similar feedback here to the other patch. Please try to see if there's some reasonable way to make this dependent just on the lifetime qualifier without paying attention to the language options. If we, say, decide to start supporting __strong in non-ARC

[PATCH] D31006: [Objective-C] Fix "weak-unavailable" warning with -fobjc-weak

2017-03-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaCast.cpp:125 + assert(Self.getLangOpts().ObjCAutoRefCount || + Self.getLangOpts().ObjCWeak); Unlike the other patches, we do clearly need to be checking the language options in places

[PATCH] D31007: [Objective-C] Miscellaneous -fobjc-weak Fixes

2017-03-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Same thing about parts of this patch — please try to just check T.hasNonTrivialObjCLifetime() when reasonable. Thank you for all this, by the way. :) https://reviews.llvm.org/D31007 ___ cfe-commits mailing list

[PATCH] D30977: [CodeGen] Emit a CoreFoundation link guard when @available is used

2017-03-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGObjC.cpp:3423 +return; + if (!IsOSVersionAtLeastFn) +return; Reverse these checks, please; IsOSVersionAtLeastFn is much cheaper to check and will predominantly be null.

[PATCH] D31003: [Objective-C] C++ Classes with __weak Members non-POD Types when using -fobjc-weak

2017-03-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/AST/DeclCXX.cpp:727 + !(Context.getLangOpts().ObjCWeak && +T.getObjCLifetime() == Qualifiers::OCL_Weak)) { setHasObjectMember(true); Similarly, I think the best way of expressing this

[PATCH] D31003: [Objective-C] C++ Classes with __weak Members non-POD Types when using -fobjc-weak

2017-03-20 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks, LGTM. https://reviews.llvm.org/D31003 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31007: [Objective-C] Miscellaneous -fobjc-weak Fixes

2017-03-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/AST/Type.cpp:3773 +/// lifetime semantics. +bool Type::isNonTrivialObjCLifetimeType() const { + return CanonicalType.hasNonTrivialObjCLifetime(); Is this method not identical in behavior to

[PATCH] D31006: [Objective-C] Fix "weak-unavailable" warning with -fobjc-weak

2017-03-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. LGTM. Comment at: lib/Sema/SemaCast.cpp:125 + assert(Self.getLangOpts().ObjCAutoRefCount || + Self.getLangOpts().ObjCWeak); bkelley

[PATCH] D31004: [Objective-C] Fix __weak type traits with -fobjc-weak

2017-03-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/AST/Type.cpp:2026 - if (Context.getLangOpts().ObjCAutoRefCount) { -switch (getObjCLifetime()) { -case Qualifiers::OCL_ExplicitNone: - return true; - -case Qualifiers::OCL_Strong: -case

[PATCH] D30977: [CodeGen] Emit a CoreFoundation link guard when @available is used

2017-03-15 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGObjC.cpp:3435 + CheckFTy, "__clang_at_available_requires_core_foundation_framework")); + CFLinkCheckFunc->setLinkage(llvm::GlobalValue::LinkOnceAnyLinkage); + CodeGenFunction CGF(*this); Is this a

[PATCH] D31673: Allow casting C pointers declared using extern "C" to ObjC pointer types

2017-04-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/Sema/SemaExprObjC.cpp:3358 var && - var->getStorageClass() == SC_Extern && + !var->isThisDeclarationADefinition() && var->getType().isConstQualified()) { Hmm. Come to think

[PATCH] D31673: Allow casting C pointers declared using extern "C" to ObjC pointer types

2017-04-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. https://reviews.llvm.org/D31673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31168: Set FMF for -ffp-contract=fast

2017-04-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. Yes, thank you, that's great. https://reviews.llvm.org/D31168 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31669: Fix lambda to block conversion in C++17 by avoiding copy elision for the lambda capture used by the created block

2017-04-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: include/clang/Sema/Initialization.h:124 +/// is a lambda that's captured by a block it's converted to. +bool IsLambdaToBlockConversionEntity; }; It seems less invasive to just give this a new EntityKind.

[PATCH] D31168: Set FMF for -ffp-contract=fast

2017-04-02 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I may have missed earlier steps in this patch series. Why is this being done statefully and contextually in the IRBuilder instead of just applying the flag from the BinaryOperator to the instruction when building it? It's not like ScalarExprEmitter doesn't know that

[PATCH] D31043: Update for lifetime intrinsic signature change

2017-04-06 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/D31043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31669: Fix lambda to block conversion in C++17 by avoiding copy elision for the lambda capture used by the created block

2017-04-05 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Looks good, thanks. Repository: rL LLVM https://reviews.llvm.org/D31669 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31830: Emit invariant.group.barrier when using union field

2017-04-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),

[PATCH] D31717: CodeGen: Let lifetime intrinsic use alloca address space

2017-04-17 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. LGTM. https://reviews.llvm.org/D31717 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32029: [ObjC] Fix lifetime markers of loop variable in EmitObjCForCollectionStmt

2017-04-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. Yes, looks good to me. Repository: rL LLVM https://reviews.llvm.org/D32029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D32029: [ObjC] Fix lifetime markers of loop variable in EmitObjCForCollectionStmt

2017-04-14 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Those test changes are smaller than I thought they might be; great. https://reviews.llvm.org/D32029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31885: Remove TBAA information from LValues representing union members

2017-04-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D31885#730539, @dberlin wrote: > In https://reviews.llvm.org/D31885#730072, @rjmccall wrote: > > > Thanks for CC'ing me. There are two problems here. > > > > The second is that our alias-analysis philosophy says that the fact that > > two

[PATCH] D31885: Remove TBAA information from LValues representing union members

2017-04-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D31885#730853, @hfinkel wrote: > > There was a deliberate decision to make TBAA conservative about type > > punning in LLVM because, in practice, the strict form of TBAA you think we > > should follow has proven to be a failed optimization

[PATCH] D31885: Remove TBAA information from LValues representing union members

2017-04-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D31885#730920, @dberlin wrote: > Just so i understand: Ignoring everything else (we can't actually make > likelyalias work, i think the code in the bugs makes that very clear), None of the code in the bugs I've seen makes that very clear,

[PATCH] D31885: Remove TBAA information from LValues representing union members

2017-04-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D31885#730799, @dberlin wrote: > In https://reviews.llvm.org/D31885#730766, @rjmccall wrote: > > > In https://reviews.llvm.org/D31885#730539, @dberlin wrote: > > > > > In https://reviews.llvm.org/D31885#730072, @rjmccall wrote: > > > > > > >

[PATCH] D32199: [TBAASan] A TBAA Sanitizer (Clang)

2017-04-20 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. If you're going to try to enforce the declared type of memory, you'll also need something like the C effective type rule to handle char buffers in C++. As far as I can tell, it's not actually legal under the spec to cast an array of chars to an arbitrary type and

[PATCH] D32210: [Sema][ObjC] Add support for attribute "noescape"

2017-04-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. The rule we actually want is that no reference to the block derived from the parameter value will survive after the function returns. You can include examples of things that would cause potential escapes, but trying to actually lock down the list seems ill-advised.

[PATCH] D31885: Remove TBAA information from LValues representing union members

2017-04-19 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In https://reviews.llvm.org/D31885#730958, @dberlin wrote: > In https://reviews.llvm.org/D31885#730936, @rjmccall wrote: > > > In https://reviews.llvm.org/D31885#730920, @dberlin wrote: > > > > > Just so i understand: Ignoring everything else (we can't actually make >

  1   2   3   4   5   6   7   8   9   10   >