[PATCH] D32601: [CodeGen][ObjC] Don't retain/release capture objects at block creation that are const-qualified

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I agree, this seems reasonable. LGTM. https://reviews.llvm.org/D32601 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.l

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

rjmccall added a comment. I agree with Aaron, it would be nice if this had some immediate effect. One obvious suggestion: take advantage of it in code generation. LLVM already has a parameter attribute called "nocapture" which conveys exactly the right semantics for noescape pointers and refe

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#748873, @kparzysz wrote: > Ping. > > What's the next step here? Sounds to me like we should just not apply struct-path TBAA data that runs through a union field because either LLVM's representation can't handle it or Clang isn't gen

[PATCH] D32977: [OpenCL] Emit function-scope variable in constant address space as static variable

rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:7129 +NewVD->getType().getAddressSpace() != LangAS::opencl_constant) || NewVD->hasExternalStorage()) { if (!T->isSamplerT() && Seeing criteria like this, and a lot of the ot

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

rjmccall added a comment. The right fix is probably just to make sure that EmitLValueForField doesn't add TBAA information when the base LValue doesn't have it. That will also fix problems with recursive member access where the base is may_alias. Repository: rL LLVM https://reviews.llvm.or

[PATCH] D32248: CodeGen: Cast alloca to expected address space

rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7291 + Address adjustAddrSpaceForAutoVar(Address A, const VarDecl *VD, + CodeGen::CodeGenFunction &CGF) const override; }; How about, instead of introducing a second method, we just cha

[PATCH] D32977: [OpenCL] Emit function-scope variable in constant address space as static variable

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:158 + // variable in constant address space in OpenCL. + if (D.getStorageDuration() != SD_Automatic && !D.hasExternalStorage()) { llvm::GlobalValue::LinkageTypes Linkage = Please rearrange

[PATCH] D32977: [OpenCL] Emit function-scope variable in constant address space as static variable

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

[PATCH] D32248: CodeGen: Cast alloca to expected address space

rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7291 + Address adjustAddrSpaceForAutoVar(Address A, const VarDecl *VD, + CodeGen::CodeGenFunction &CGF) const override; }; yaxunl wrote: > rjmccall wrote: > > How about, instead of intr

[PATCH] D32248: CodeGen: Cast alloca to expected address space

rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7291 + Address adjustAddrSpaceForAutoVar(Address A, const VarDecl *VD, + CodeGen::CodeGenFunction &CGF) const override; }; yaxunl wrote: > rjmccall wrote: > > yaxunl wrote: > > > rjmcca

[PATCH] D32977: [OpenCL] Emit function-scope variable in constant address space as static variable

rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:10286 + // these variables must be a compile time constant. + VDecl->getType().getAddressSpace() == LangAS::opencl_constant) CheckForConstantInitializer(Init, DclT); yaxu

[PATCH] D32248: CodeGen: Cast alloca to expected address space

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1115 + assert(T.getAddressSpace() == LangAS::Default || + T.getQualifiers().hasTargetSpecificAddressSpace()); + auto Addr = getTargetHooks().performAddrSpaceCast(*this, yaxunl wrote: > t

[PATCH] D32977: [OpenCL] Emit function-scope variable in constant address space as static variable

rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:10286 + // these variables must be a compile time constant. + VDecl->getType().getAddressSpace() == LangAS::opencl_constant) CheckForConstantInitializer(Init, DclT); Anas

[PATCH] D32248: CodeGen: Cast alloca to expected address space

rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:7210 + return; +} } err_opencl_function_variable seems like a better diagnostic, at least for opencl_global. You can fall back on this more general diagnostic for other address space

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

rjmccall added a comment. In https://reviews.llvm.org/D31885#755456, @kparzysz wrote: > In https://reviews.llvm.org/D31885#751164, @rjmccall wrote: > > > The right fix is probably just to make sure that EmitLValueForField doesn't > > add TBAA information when the base LValue doesn't have it. Th

[PATCH] D32248: CodeGen: Cast alloca to expected address space

rjmccall added a comment. Looking good. We had some overlapping patches and comments there, so I want to make sure you didn't miss my comment on EmitAutoVarAlloca. Comment at: lib/Sema/SemaDecl.cpp:7205 +} +assert (T.getAddressSpace() != LangAS::opencl_constant); +

[PATCH] D32248: CodeGen: Cast alloca to expected address space

rjmccall added inline comments. Comment at: lib/CodeGen/CGDecl.cpp:1120 +address.getPointer()->getType()->getPointerElementType()->getPointerTo( +getContext().getTargetAddressSpace(T.getAddressSpace())), +/*non-null*/ true); A lot of t

[PATCH] D32248: CodeGen: Cast alloca to expected address space

rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7296 + unsigned getASTAllocaAddressSpace() const override { +return LangAS::Count + getABIInfo().getDataLayout().getAllocaAddrSpace(); + } yaxunl wrote: > rjmccall wrote: > > Can we rena

[PATCH] D32248: CodeGen: Cast alloca to expected address space

rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:7296 + unsigned getASTAllocaAddressSpace() const override { +return LangAS::Count + getABIInfo().getDataLayout().getAllocaAddrSpace(); + } yaxunl wrote: > rjmccall wrote: > > yaxunl wrot

[PATCH] D32248: CodeGen: Cast alloca to expected address space

rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:411 +CodeGen::CodeGenFunction &CGF, llvm::Value *Src, unsigned SrcAddr, +unsigned DestAddr, llvm::Type *DestTy, bool isNonNull) const { // Since target may map different address spaces in AST to the

[PATCH] D32248: CodeGen: Cast alloca to expected address space

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Looks great, thank you for being patient. https://reviews.llvm.org/D32248 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://list

[PATCH] D33284: [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource

rjmccall added a comment. Generally looks great, thanks! Comment at: lib/CodeGen/CGBlocks.cpp:923 + MakeAddrLValue(blockField, type, + LValueBaseInfo(AlignmentSource::Decl, false)), /*captured by init*/ false); -

[PATCH] D33284: [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:2256 Address(CapLVal.getPointer(), getContext().getDeclAlign(VD)), -CapLVal.getType(), AlignmentSource::Decl); +CapLVal.getType(), LValueBaseInfo(AlignmentSource::Decl, MayAli

[PATCH] D33284: [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource

rjmccall added a comment. In https://reviews.llvm.org/D33284#757917, @kparzysz wrote: > Changed the getNatural.*TypeAlignment to produce the entire LValueBaseInfo, > assuming MayAlias to be false. > > Added merging of LValueBaseInfos. The merging assumes that the alignment > source in the para

[PATCH] D33284: [CodeGen] Propagate LValueBaseInfo instead of AlignmentSource

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Yes, of course. Although "looks good to me" is acceptance whether or not I actually push a button in Phabricator. :) Repository: rL LLVM https://reviews.llvm.org/D33284 ___

[PATCH] D32401: [Devirtualization] insert placement new barrier with -O0

rjmccall added a comment. In https://reviews.llvm.org/D32401#759223, @Prazek wrote: > Is everyone ok with sending this patch? As long as this continues to be an opt-in optimization, go ahead and do your research project, I suppose. https://reviews.llvm.org/D32401 _

[PATCH] D33328: [CodeGen] Pessimize aliasing for union members (and may-alias) objects

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:1436 +if (BaseInfo.getMayAlias()) + TBAAInfo = CGM.getTBAAInfo(getContext().CharTy); llvm::MDNode *TBAAPath = CGM.getTBAAStructTagInfo(TBAABaseType, TBAAInfo, Hmm. Should we be cons

[PATCH] D33328: [CodeGen] Pessimize aliasing for union members (and may-alias) objects

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:1436 +if (BaseInfo.getMayAlias()) + TBAAInfo = CGM.getTBAAInfo(getContext().CharTy); llvm::MDNode *TBAAPath = CGM.getTBAAStructTagInfo(TBAABaseType, TBAAInfo, kparzysz wrote: > rjmcc

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

rjmccall added a comment. > In addition to the ABI compatibility concern, IMO, GCC's behavior here is > better and more user friendly: > > - C++ classes have methods > - calling a method on a field takes the address of a field I mean, this argument is hardly limited to non-POD types. I suppose

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

rjmccall added a comment. Yes, I think that's an okay way to check this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117616/new/ https://reviews.llvm.org/D117616 ___ cfe-commits mailing list cfe-commit

[PATCH] D118402: [C2x][ObjC] Do not crash when trying to encode a _BitInt type

rjmccall added a comment. I don't think you really need the FIXME when we don't have it for all the other types that we don't have encodings for. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118402/new/ https://reviews.llvm.org/D118402 _

[PATCH] D118402: [C2x][ObjC] Do not crash when trying to encode a _BitInt type

rjmccall added a comment. But overall this seems fine, yeah. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118402/new/ https://reviews.llvm.org/D118402 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D118552: [clang] [concepts] Correctly(?) handle placeholder types in ExprRequirements.

rjmccall added a comment. It's a type that the debugger integration uses when it doesn't know the type of a symbol. If it's a data symbol, you make a `VarDecl` with `UnknownAny` type; if it's a function symbol, you make a `FunctionDecl` with `UnknownAny`. In either case, if the user fails to

[PATCH] D113107: Support of expression granularity for _Float16.

rjmccall added a comment. So, this is still approach #4, i.e. changing the types of expressions, which is not what GCC appears to be doing: https://godbolt.org/z/67hqeb37h. You need a custom emitter in IRGen for `_Float16`-typed arithmetic expressions. Comment at: clang/lib/

[PATCH] D120566: [OpenCL][AMDGPU]: Do not allow a call to kernel

rjmccall added a comment. Is there something which stops you from taking the address of a kernel and then calling it? If not, are there actually any uses of kernels in the module that shouldn't be rewritten as uses of the clone? I feel like this would be a lot easier to just fix in your LLVM p

[PATCH] D120566: [OpenCL][AMDGPU]: Do not allow a call to kernel

rjmccall added a comment. In D120566#3346533 , @arsenm wrote: > In D120566#3346506 , @rjmccall > wrote: > >> Is there something which stops you from taking the address of a kernel and >> then calling it? If not

[PATCH] D118525: [modules] Merge ObjC interface ivars with anonymous types.

rjmccall added a comment. This is one of those patches that's difficult to review because it's so hard to foresee the consequences of changing the concepts. That said, I think the basic idea here seems reasonable. Comment at: clang/test/Modules/merge-anon-record-definition-i

[PATCH] D120856: [NFC][Clang][OpaquePtr] Remove calls to Address::deprecated in CGBlocks.cpp

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120856/new/ https://reviews.llvm.org/D120856 ___

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

rjmccall added a comment. Test looks good, thanks. Comment at: clang/lib/CodeGen/CGCall.cpp:2060 +case Builtin::BIstrndup: + RetAttrs.addAlignmentAttr(Context.getTargetInfo().getNewAlign() / +Context.getTargetInfo().getCharWi

[PATCH] D101097: [Sema] Don't set BlockDecl's DoesNotEscape bit If the block is being passed to a function taking a reference parameter

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:5917 if (auto *BE = dyn_cast(Arg->IgnoreParenNoopCasts(Context))) BE->getBlockDecl()->setDoesNotEscape(); We need to be checking that the parameter type is a block point

[PATCH] D93377: [Clang] Add __ibm128 type to represent ppc_fp128

rjmccall added subscribers: akyrtzi, benlangmuir. rjmccall added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:131 +Float128, +Ibm128 }; This is necessary because it's possible to derive this type from a mode attribute? ===

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

rjmccall added a comment. What is the purpose of the builtin? Where is it being used? Typically you *can't* change the signature of a builtin because the builtin is itself a language feature that's documented to have a particular signature. If you've made a builtin purely for use in generate

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879 ___ cfe-commits mailing list

[PATCH] D101097: [Sema] Don't set BlockDecl's DoesNotEscape bit If the block is being passed to a function taking a reference parameter

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:5917 if (auto *BE = dyn_cast(Arg->IgnoreParenNoopCasts(Context))) BE->getBlockDecl()->setDoesNotEscape(); ahatanak wrote: > rjmccall wrote: > > We need to be checking th

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

rjmccall added a comment. Yes, if you can dynamically choose to use an aligned allocator, that's clearly just much better. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100739/new/ https://reviews.llvm.org/D100739

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

rjmccall added a comment. In D100739#2717259 , @ychen wrote: > In D100739#2717227 , @rjmccall > wrote: > >> Yes, if you can dynamically choose to use an aligned allocator, that's >> clearly just much better. > >

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

rjmccall added a comment. Here are the options I think the committee might take: 1. Always select an unaligned allocator and force implementors to dynamically align. This seems unlikely to me. 2. Allow an aligned allocator to be selected. The issue here is that we cannot know until coroutine

[PATCH] D101502: [ObjC][ARC] Don't enter the cleanup scope if the initializer expression isn't an ExprWithCleanups

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101502/new/ https://reviews.llvm.org/D101502

[PATCH] D101389: [clang][CodeGen] Fix address space for sret

rjmccall added a comment. I think this is intentional; requiring the indirect-result parameter to be in the alloca address space would prevent direct initialization of non-temporary memory, which is an important optimization in C++. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101389/

[PATCH] D101389: [clang][CodeGen] Fix address space for sret

rjmccall added a comment. In D101389#2724700 , @yaxunl wrote: > In D101389#2724636 , @rjmccall > wrote: > >> I think this is intentional; requiring the indirect-result parameter to be >> in the alloca address sp

[PATCH] D101097: [Sema] Don't set BlockDecl's DoesNotEscape bit If the block is being passed to a function taking a reference parameter

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101097/new/ https://reviews.llvm.org/D101097 ___

[PATCH] D101156: [Clang] Support a user-defined __dso_handle

rjmccall added a comment. What's the crash exactly/ Is IRGen just unhappy about processing the user definition when we've generated a declaration with a different type? Because we're already supposed to be being cautious about this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

rjmccall added a comment. Seems conceptually okay, given that we have an option to control it. Comment at: clang/lib/CodeGen/CGStmt.cpp:796 + bool EmitBoolCondBranch = !C || !C->isOne(); + bool CondIsConst = C; const SourceRange &R = S.getSourceRange(); fh

[PATCH] D102015: [clang CodeGen] Don't crash on large atomic function parameter.

rjmccall added a comment. Objective-C object types also have aggregate evaluation kind. Those can only be used as value types in an old, fragile ObjC ABI, but I believe we haven't yet formally decided to remove support for that. Fortunately, however, there's a much better simplification avail

[PATCH] D102015: [clang CodeGen] Don't crash on large atomic function parameter.

rjmccall added a comment. In D102015#2748441 , @efriedma wrote: > In D102015#2743634 , @rjmccall > wrote: > >> Objective-C object types also have aggregate evaluation kind. Those can >> only be used as value ty

[PATCH] D115790: [Coroutines] Set presplit attribute in Clang and mlir

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:712 + assert(CurFn); + CurFn->addFnAttr("coroutine.presplit", "0"); } The assertion here is not necessary; if it was, we'd need it everywhere. Please add a comment like "LLVM expect

[PATCH] D115790: [Coroutines] Set presplit attribute in Clang and mlir

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Alright, that's fine. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115790/new/ https://reviews.llvm.org/D115790 ___ cfe-commits maili

[PATCH] D111566: [SYCL] Fix function pointer address space

rjmccall added a comment. Block pointers are actually data pointers and should stay in the generic address space. I... don't honestly know why they get lowered as function pointers; they probably shouldn't be. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111566/new/ https://reviews.

[PATCH] D116948: [CodeGen] Treat ObjC `__unsafe_unretained` and class types as trivial when generating copy/dispose helper functions

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBlocks.cpp:41 + // Reserve space now to prevent reallocation. + Captures.reserve(Block->getNumCaptures()); Does this actually work? It's a hashtable, so I'm worried about the possibility of e.g

[PATCH] D115440: Provide __builtin_alloca*_uninitialized variants

rjmccall added a comment. Yes, this LGTM implementation-wise. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115440/new/ https://reviews.llvm.org/D115440 ___ cfe-commits mailing list cfe-commits@lists.llv

[PATCH] D111566: [SYCL] Fix function pointer address space

rjmccall added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:11494 + const Type *TypePtr = T.getTypePtr(); + return TypePtr->isFunctionType() + ? llvm::DataLayout(getTargetInfo().getDataLayoutString()) You can just do `T->isFunctionType(

[PATCH] D111566: [SYCL] Fix function pointer address space

rjmccall added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:11496 + .getProgramAddressSpace() + : getTargetAddressSpace(T.getQualifiers()); +} Oh, I'm sorry I missed this. Parsing the data layout string into an `llvm::

[PATCH] D116948: [CodeGen] Treat ObjC `__unsafe_unretained` and class types as trivial when generating copy/dispose helper functions

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBlocks.cpp:361 /// Order by 1) all __strong together 2) next, all byfref together 3) next, /// all __weak together. Preserve descending alignment in all situations. "byref"

[PATCH] D116948: [CodeGen] Treat ObjC `__unsafe_unretained` and class types as trivial when generating copy/dispose helper functions

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBlocks.cpp:2142 +if (capture.isConstantOrTrivial()) + continue; + ahatanak wrote: > rjmccall wrote: > > Should this condition be specific to whether it's trivial *to destroy*? > > C++ type

[PATCH] D116948: [CodeGen] Treat ObjC `__unsafe_unretained` and class types as trivial when generating copy/dispose helper functions

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116948/new/ https://reviews.llvm.org/D116948 ___

[PATCH] D116048: [clang][CodeGen][UBSan] VLA size checking for unsigned integer parameter

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, very minor requests, but otherwise LGTM; feel free to commit with these changes. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2256 + llvm::Value *siz

[PATCH] D111566: [SYCL] Fix function pointer address space

rjmccall added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:763 + /// Set the address space for functions for the given target. + virtual void setProgramAddressSpace() { ProgramAddrSpace = 0; } + eandrews wrote: > I'm not entirely sure ab

[PATCH] D111566: [SYCL] Fix function pointer address space

rjmccall accepted this revision. rjmccall added a comment. LGTM, thanks! Comment at: clang/test/CodeGen/avr/functionptr-addrspace.c:3 + +int main() { + int (*p)(); eandrews wrote: > When writing the test I noticed an existing crash in the compiler. I am not >

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

rjmccall added a comment. I can understand how we got here, but it's a bad place to have ended up. Toolchains that do dead-stripping need a way to prevent it for specific objects and functions. Windows and Darwin toolchains have historically done aggressive dead-stripping in both the compiler

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

rjmccall added a comment. In D97446#3241828 , @JonChesterfield wrote: > I'd much refer we put variables in arrays corresponding to their intended > lifespan instead of the binary format they're destined for. I agree that LLVM features should generally m

[PATCH] D117435: [clang] Warning for inline constexpr functions

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. I don't remember if this is written up anywhere, but clang has a longstanding policy that style enforcement belongs in a linter rather than the compiler. One programmer's "redun

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

rjmccall added a comment. Don't worry about `GC::Strong`. ObjC GC is thoroughly dead (unless there's GNU-runtime interest in it?), and AFAIK we've never made any effort to incorporate it into our treatment of non-trivial structs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

[PATCH] D115222: [Coroutines] Remove unused coroutine builtin/intrinsics llvm.coro.param (NFC-ish)

rjmccall added a reviewer: GorNishanov. rjmccall added a comment. I imagine Gor hoped for this optimization to be implemented someday, assuming it's still allowed by the language specification. Maybe you would be interested in pursuing that? It sounds like it's really just (1) emitting the in

[PATCH] D115219: [C++20] [Coroutines] Mark coroutine done if unhandled_exception throws

rjmccall added a comment. I agree that you shouldn't call `suspend`, but doesn't `coro.end` have the behavior of marking the coroutine done? Should we just be calling `coro.end` on this path? Comment at: llvm/docs/Coroutines.rst:1661 +should return true. All of the '``llvm.c

[PATCH] D115219: [C++20] [Coroutines] Mark coroutine done if unhandled_exception throws

rjmccall added a comment. In D115219#3175660 , @ChuanqiXu wrote: > In D115219#3175582 , @rjmccall > wrote: > >> I agree that you shouldn't call `suspend`, but doesn't `coro.end` have the >> behavior of marking t

[PATCH] D112616: Fix crash on invalid code involving late parsed inline methods

rjmccall added a comment. The only alternative to this that I can see would be for the invalid code to produce more valid-seeming AST. Are deduction guides just not normally allowed for methods? Also, while I don't think it would eliminate the need to be defensive here, our recovery from misna

[PATCH] D115222: [Coroutines] Remove unused coroutine builtin/intrinsics llvm.coro.param (NFC-ish)

rjmccall added a comment. Like a lot of the switched-resume lowering, this intrinsic is extremely tied to C++ semantics. If C++ doesn't actually allow the optimization anymore, then I completely agree that we should go ahead and remove the intrinsic. If it's allowed and we just haven't implem

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

rjmccall added a comment. I am perfectly happy accepting that syllogism. Passing a value in registers is a trivial relocation. If there is a reason your type shouldn't be trivially relocated, you should not make it `trivial_abi`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[PATCH] D114732: [clang] Mark `trivial_abi` types as "trivially relocatable".

rjmccall added a comment. Trivial relocation doesn't imply that types have to be safe against being suddenly relocated during the middle of operations while they're not in a safe internal state. That is not a consideration. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D112616: Fix crash on invalid code involving late parsed inline methods

rjmccall added a comment. Okay. I guess the only choice, then, would be to not try to parse a template deduction guide when you're in a context where template deduction guides aren't allowed. Maybe that's not actually reasonable. Well, whatever. I don't really have a problem with this patch;

[PATCH] D115219: [C++20] [Coroutines] Mark coroutine done if unhandled_exception throws

rjmccall added a comment. In D115219#3178370 , @ChuanqiXu wrote: > In D115219#3177213 , @rjmccall > wrote: > >> Okay. Well, I'm glad it works. I guess I find it a little strange that >> `coro.end` doesn't alre

[PATCH] D115222: [Coroutines] Remove unused coroutine builtin/intrinsics llvm.coro.param (NFC-ish)

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, that's fine, I have no objection to removing it. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115222/new/ https://reviews.llvm

[PATCH] D115219: [C++20] [Coroutines] Mark coroutine done if unhandled_exception throws

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Minor suggestion in the docs, but otherwise LGTM. Comment at: llvm/docs/Coroutines.rst:1382 +The `coro.end` intrinsic in the normal path wouldn't do this since the corou

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

rjmccall added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:14583 if (Second && Second->getObjectKind() == OK_ObjCProperty) { ExprResult Result = SemaRef.CheckPlaceholderExpr(Second); rnk wrote: > This is also pseudo object handling code

[PATCH] D115790: [Coroutines] Run CoroEarly Pass in front of AlwaysInliner in O0 pass and warn for always_inline coroutine

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. I don't find it at all weird to say that there's an invariant that certain intrinsics can only appear in functions with a particular attribute, and so the frontend has a responsi

[PATCH] D115791: [CodeGen] Store ElementType in LValue

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115791/new/ https://reviews.llvm.org/D115791 ___

[PATCH] D114728: [Coroutine] Remove the prologue data of `-fsanitize=function` for split functions

rjmccall added a comment. I don't have a strong opinion about attribute vs. metadata; if metadata are the best technical path forward, that's fine with me. I don't think function metadata can be "lost" the same way that metadata internal to a function can, right? Repository: rG LLVM Github

[PATCH] D115792: [Modules] Incorrect ODR detection for unresolved using type

rjmccall added a comment. This seems reasonable, but I think it's probably better to just doing it in the ASTContext code (`getTypeDeclTypeSlow`) the same way it's done for e.g. `RecordDecl`s with previous declarations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

[PATCH] D115792: [Modules] Incorrect ODR detection for unresolved using type

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115792/new/ https://reviews.llvm.org/D115792 ___ cfe-commits mailing list

[PATCH] D116048: [clang][CodeGen][UBSan] VLA size checking for unsigned integer parameter

rjmccall added a comment. Thanks, patch basically looks good. Minor requests and an observation for possible follow-up work. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2247 // Otherwise, evaluate and record it. - if (const Expr *size = vat->getSizeExpr()) {

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rjmccall added a comment. In D96033#2630585 , @rsmith wrote: > @rjmccall I'd like your input on this patch in particular, if you have time. I've been specifically avoiding paying any attention to this patch because it sounds like an enormous time-sink to

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

rjmccall added a comment. In D97371#2596643 , @tbaeder wrote: > They are being applied to the `@try` at least (`-ast-print` prints them) and > we do some error checking for missing call expressions in > `handleNoMergeAttr()` in `SemaStmtAttr.cpp`. I don'

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rjmccall added a comment. In D96033#2630978 , @v.g.vassilev wrote: >>> I'm nervous in general about the looming idea of declaration unloading, but >>> the fact that it's been working in Cling for a long time gives me some >>> confidence that we can do t

[PATCH] D98816: PR49585: Emit the jump destination for a for loop 'continue' from within the scope of the condition variable.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Well, that's certainly some bug. Patch LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98816/new/ https://reviews.llvm.org/D98816

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rjmccall added a comment. To be clear, what I'm trying to say is that — absent consensus that it's a major architecture shift is appropriate — we need to consider what functionality is reasonably achievable without it. I'm sure that covers most of what you're trying to do; it just may not incl

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

rjmccall added a comment. Yes, I think that would be reasonable, thank you. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97371/new/ https://reviews.llvm.org/D97371 ___ cfe-commits mailing list cfe-commi

[PATCH] D98980: [CodeGen] Don't crash on for loops with cond variables and no increment

rjmccall added a comment. Yes, LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98980/new/ https://reviews.llvm.org/D98980 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.or

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks for seeing this through CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97371/new/ https://reviews.llvm.org/D97371 ___ cfe-

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

rjmccall added a comment. Windows exceptions code-generation is quite different; I don't know whether Clang supports ObjC on Windows in general. It'd be fine if you add a `-triple` argument to this test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97371/new/ https://reviews.llvm.or

[PATCH] D93377: [Clang] Add __ibm128 type to represent ppc_fp128

rjmccall added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:2668 // ::= g # __float128 + // ::= g # __ibm128 // UNSUPPORTED:::= Dd # IEEE 754r decimal floating point (64 bits) jwakely wrote: > steven.zh

<    26   27   28   29   30   31   32   33   >