[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/include/clang/Basic/Builtins.h:65 enum ID { +#define INTERESTING_IDENTIER(ID, TYPE, ATTRS) ID, NotBuiltin = 0, // This is not a builtin function. You shouldn't muddle this into `Builtin::ID`. =

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/lib/Basic/Builtins.cpp:33 static constexpr Builtin::Info BuiltinInfo[] = { -{"not a builtin function", nullptr, nullptr, nullptr, HeaderDesc::NO_HEADER, +#define INTERESTING_IDENTIFIER(ID)

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/include/clang/Basic/IdentifierTable.h:85 +/// (including not_interesting). +/// - The rest of the values represent builtin IDs (including not_builtin). +static constexpr int FirstObjCKeywordID = 1; The code

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/include/clang/Basic/IdentifierTable.h:325 void setBuiltinID(unsigned ID) { -ObjCOrBuiltinID = ID + tok::NUM_OBJC_KEYWORDS; -assert(ObjCOrBuiltinID - unsigned(tok::NUM_OBJC_KEYWORDS) == ID - && "ID too large fo

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/include/clang/Basic/IdentifierTable.h:341 + void setInterestingIdentifierID(unsigned ID) { +assert(ID != FirstBuiltinID); +ObjCOrBuiltinID = FirstInterestingIdentifierID + (ID - 1); Similarly, this assert

[PATCH] D138511: [CodeGen][AArch64] Fix AArch64ABIInfo::EmitAAPCSVAArg crash with empty record type in variadic arg

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Please don't ping every day. We haven't lost track of your patch, we're just busy reviewing other things. This seems reasonable to me. Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D138316: [ASTContext] Avoid duplicating address space map. NFCI

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. This looks fine to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138316/new/ https://reviews.llvm.org/D138316 _

[PATCH] D136176: Implement support for option 'fexcess-precision'.

rjmccall added inline comments. Comment at: clang/test/CodeGen/X86/fexcess-precision.c:89 +// CHECK-EXT-NEXT:[[EXT1:%.*]] = fpext half [[TMP1]] to float +// CHECK-EXT-NEXT:[[MUL:%.*]] = fmul float [[EXT]], [[EXT1]] +// CHECK-EXT-NEXT:[[TMP2:%.*]] = load half, ptr [[C_

[PATCH] D136639: [CodeGen][ObjC] Fix a memory leak that occurs when a non-trivial C struct property is set using dot notation

rjmccall added inline comments. Comment at: clang/test/CodeGenObjC/nontrivial-c-struct-property.m:89 + +// CHECK: call void @__destructor_8_s0(ptr %[[AGG_TMP_ENSURED]]) + ahatanak wrote: > rjmccall wrote: > > It looks like we're copying `a` into a temporary and t

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

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: > Quuxpl

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

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

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 http://lists.llvm.org/cgi-bin/mailman/l

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

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 http://lists.llvm.org/cgi-bin/m

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

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 http://lists.llvm.org/cgi-bin/mailman/li

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

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; +llvm:

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

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 ther

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

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 the

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

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 m

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

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

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 http://lists.llvm.org/cgi-bin/

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

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 http://lists.llvm.org/cgi-bin/mailm

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

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 whe

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

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 p

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

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 i

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

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 modes,

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

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 cfe-commit

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

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. Comment

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

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 li

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

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 &Context = getLLVMContext()

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

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] D31004: [Objective-C] Fix __weak type traits with -fobjc-weak

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 Qualifiers::OCL_W

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

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 wr

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

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 hasNonTrivialObjCLifetim

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

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 http://lists.llvm.org/cgi-bin/mailman/listi

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

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 http://lists.llvm.org/cgi-bin/ma

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

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

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 http://lists.llvm.org/cgi-bin/mailman/l

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

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 patches

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

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 http://lists.llvm.org/cgi-bin/mailman/l

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

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 http://lists.llvm.org/cgi-bin/ma

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

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 f

[PATCH] D31044: Update for alloca construction changes

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 http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D29599: Clang Changes for alloc_align

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 indic

[PATCH] D29599: Clang Changes for alloc_align

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 list

[PATCH] D29599: Clang Changes for alloc_align

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 t

[PATCH] D29599: Clang Changes for alloc_align

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 http://lists.llvm.org/cgi-b

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

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] D31168: Set FMF for -ffp-contract=fast

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 http://lists.llvm.org/c

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

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. Re

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

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 o

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

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] D31043: Update for lifetime intrinsic signature change

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 http://lists.llvm.org/cgi-bin/mailman/listinfo/

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

rjmccall added inline comments. Comment at: lib/Sema/SemaExprObjC.cpp:3358 var && - var->getStorageClass() == SC_Extern && + !var->isThisDeclarationADefinition() && var->getType().isConstQualified()) { ahatanak wrote: > rjm

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

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 http://lists.llvm.org/cgi-bin/mailman/l

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

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 http://

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

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] D31717: CodeGen: Let lifetime intrinsic use alloca address space

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] D31830: Emit invariant.group.barrier when using union field

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()), Checki

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()), Prazek

[PATCH] D32133: CodeGen: Let byval parameter use alloca address space

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

[PATCH] D32110: Emit invariant.group loads with empty group md

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. Repository: rL LLVM https://reviews.llvm.org/D32110 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cg

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()), Prazek

[PATCH] D32187: [CodeGen][ObjC]

rjmccall added a comment. I wouldn't mind unconditionally banning this in JumpDiagnostics, actually. https://reviews.llvm.org/D32187 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32187: [CodeGen][ObjC]

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, this looks great. A few tweaks about the diagnostic and LGTM. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:4985 +def note_protected_by_objc_fast_enumer

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

rjmccall added a comment. Thanks for CC'ing me. There are two problems here. The first is that vectors are aggregates which contain values of their element type. (And honestly, we may need to be more permissive than this because people are pretty lax about the element type in a vector until i

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

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 s

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

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: lib/CodeGen/

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

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 structure lik

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

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 tha

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

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 http://lists.llvm.org/cgi

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

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] D29208: Prevent ICE in dllexport class with _Atomic() data member

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] D29208: Prevent ICE in dllexport class with _Atomic() data member

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; wrist

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

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 http://lists.llvm.org/cgi-bin/mai

[PATCH] D34665: [CodeGen] Fix assertion failure in EmitCallArg

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. This seems fine, thanks. https://reviews.llvm.org/D34665 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bi

[PATCH] D34444: Teach codegen to work in incremental processing mode.

rjmccall added a comment. What's the relationship between these llvm::Modules that you want to generate? Are you imagining them as separate translation units, or are the subsequent modules more like addenda to the original? Repository: rL LLVM https://reviews.llvm.org/D3 ___

[PATCH] D34444: Teach codegen to work in incremental processing mode.

rjmccall added a comment. Okay. In that case, I see two problems, one major and one potentially major. The major problem is that, as Richard alludes to, you need to make sure you emit any on-demand definitions that Sema registered with CodeGen during the initial CGM's lifetime but used in late

[PATCH] D34777: CodeGen: Fix invalid bitcast for coerced function argument

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

[PATCH] D34995: [AMDGPU] Fix size and alignment of size_t and pointer types

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

[PATCH] D35056: GCC ABI incompatibility when passing object with trivial copy ctor, trivial dtor, and non-trivial move ctor

rjmccall added a comment. If this is a common algorithm across all ABIs, can we just put Sema / the AST in charge of computing it? It's not a cheap thing to recompute retroactively, especially for an imported type, and it seems like it's easily derived from the things that go into DerivedData.

[PATCH] D33842: CodeGen: Fix address space of global variable

rjmccall added a comment. Looks great, thanks! Just a few minor changes. Comment at: lib/CodeGen/CGBlocks.cpp:1144 + ? CGM.getNSConcreteGlobalBlock() + : CGM.getNullPointer(CGM.VoidPtrPtrTy, + QualType(CGM.ge

[PATCH] D33842: CodeGen: Fix address space of global variable

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

[PATCH] D35180: Expose the Clang::QualType to llvm::Type conversion functions

rjmccall added a comment. Exposing these operations seems fine, but I'm not thrilled about the APIs. Of course, the APIs are exactly derived from IRGen's internal APIs, but we'd like to eventually clean those APIs up, and we certainly don't need to emulate them. Comment at:

[PATCH] D35180: Expose the Clang::QualType to llvm::Type conversion functions

rjmccall added a comment. Hmm. Maybe it would make more sense to allow it to return null, and then add a comment explaining that it will do so if it can't lower the function type yet. https://reviews.llvm.org/D35180 ___ cfe-commits mailing list cf

[PATCH] D35056: GCC ABI incompatibility when passing object with trivial copy ctor, trivial dtor, and non-trivial move ctor

rjmccall added a comment. Thank you, I like this approach much better, and the IRGen changes seem fine to me. I'd like to defer to someone else (probably Richard) to review whether the changes to completeDefinition() are correct; I'm not up-to-date with how we handle lazy declarations. https

[PATCH] D35180: Expose the Clang::QualType to llvm::Type conversion functions

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Looks great, thanks! https://reviews.llvm.org/D35180 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/ma

[PATCH] D35180: Expose the Clang::QualType to llvm::Type conversion functions

rjmccall accepted this revision. rjmccall added a comment. Still looks good to me. Do you need help committing? https://reviews.llvm.org/D35180 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/c

[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.

rjmccall added a comment. This is a cute hack, but... I'm not sure I accept the premise that it's a noteworthy obstacle to Clang development to do two greps instead of one. And I don't think I've ever had to debug a diagnostic where __FILE__ and __LINE__ information would've been helpful. Als

[PATCH] D35180: Expose the Clang::QualType to llvm::Type conversion functions

rjmccall closed this revision. rjmccall added a comment. Committed. https://reviews.llvm.org/D35180 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3832 + "indirect-arg-temp"); +IRCallArgs[FirstIRArg] = CastToAllocaAddrSpace(Addr.getPointer()); Isn't the original code here correct? You're basical

[PATCH] D35175: New option that adds the DiagID enum name and index to Diagnostic output.

rjmccall added a comment. To me, features that only serve to help compiler development need to meet a higher bar than this. This just seems really marginal. Like Alex said, you should be able to pretty easily write a debugger script that breaks when it sees a specific diagnostic, or a diagnost

[PATCH] D35268: [ObjC] Ambiguous property synthesis should pick the 'readwrite' property and check for incompatible attributes

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. This all seems reasonable. LGTM. Repository: rL LLVM https://reviews.llvm.org/D35268 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D35438: CodeGen: Ensure there is basic block when performing address space cast

rjmccall added a comment. Hmm. It doesn't seem unreasonable for this to require an insertion point as a precondition. In what situation do we emit addrspace casts after a return? https://reviews.llvm.org/D35438 ___ cfe-commits mailing list cfe-co

[PATCH] D35438: CodeGen: Ensure there is basic block when performing address space cast

rjmccall added a comment. Oh, of course. Sadly, in C/C++ the declaration point of a variable does not necessarily dominate all uses of the variable, so you need to emit the cast immediately after the alloca. Just temporarily move CGF.Builder to that point; hopefully we can assume that this fu

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

rjmccall added a comment. In https://reviews.llvm.org/D32210#810292, @ahatanak wrote: > Address review comments. > > - Allow attaching "noescape" to pointers other than block pointers. Update > the docs accordingly. > - Attach attribute "nocapture" to parameters that are annotated with > "noesc

[PATCH] D35438: CodeGen: Insert addr space cast for automatic/temp var at right position

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:76 auto DestAddrSpace = getContext().getTargetAddressSpace(LangAS::Default); +EnsureInsertPoint(); +auto *CurBB = Builder.GetInsertBlock(); IRBuilder already has saveIP() and restoreI

[PATCH] D35438: CodeGen: Insert addr space cast for automatic/temp var at right position

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

[PATCH] D35479: [CodeGen][mips] Support `long_call/far/near` attributes

rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:1191 +def MipsLongCall : InheritableAttr, TargetSpecificAttr { + let Spellings = [GCC<"long_call">, GCC<"far">, GCC<"near">]; Because this is used for all three attributes, I think you sho

[PATCH] D34444: Teach codegen to work in incremental processing mode.

rjmccall added a comment. In https://reviews.llvm.org/D3#795175, @v.g.vassilev wrote: > @rjmccall, thanks for the prompt and thorough reply. > > In https://reviews.llvm.org/D3#793311, @rjmccall wrote: > > > Okay. In that case, I see two problems, one major and one potentially > > major.

[PATCH] D35479: [CodeGen][mips] Support `long_call/far/near` attributes

rjmccall added a comment. One minor revision, but otherwise looks great, thank you. Comment at: lib/CodeGen/CodeGenModule.cpp:1166 +if (!IsForDefinition) + getTargetCodeGenInfo().setTargetAttributes(FD, F, *this); + } I think you should probably pass

<    24   25   26   27   28   29   30   31   32   33   >