[PATCH] D41394: [CodeGen] Support generation of TBAA info in the new format

rjmccall added a comment. Tests? Repository: rL LLVM https://reviews.llvm.org/D41394 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41394: [CodeGen] Support generation of TBAA info in the new format

rjmccall added a comment. Rewriting some of the most basic tests would be fine. Please either use new FileCheck lines or clone the existing tests, since we don't really know how long this transition will last. Repository: rL LLVM https://reviews.llvm.org/D41394 _

[PATCH] D41311: [CodeGen] Fix crash when a function taking transparent union is redeclared.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, I think that's reasonable enough. LGTM. https://reviews.llvm.org/D41311 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:/

[PATCH] D41394: [CodeGen] Support generation of TBAA info in the new format

rjmccall added a comment. You can pass multiple -check-prefix arguments to FileCheck and it'll match all of them. You can use that to make your test change simpler: make the existing RUN check for both PATH and OLD-PATH and the new RUN check for both PATH and NEW-PATH, then change all the exis

[PATCH] D41452: [CodeGen] Fix access sizes in new-format TBAA tags

rjmccall added a comment. LGTM. Repository: rL LLVM https://reviews.llvm.org/D41452 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1152 +NTFK_Struct, // non-trivial C struct. +NTFK_Array// array that has non-trivial elements. + }; We don't actually distinguish arrays in DestructionKind. Is it important here?

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1152 +NTFK_Struct, // non-trivial C struct. +NTFK_Array// array that has non-trivial elements. + }; ahatanak wrote: > rjmccall wrote: > > We don't actually distinguish arrays in De

[PATCH] D41394: [CodeGen] Support generation of TBAA info in the new format

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. That's great, thanks. LGTM. https://reviews.llvm.org/D41394 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cg

[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:1152 +NTFK_Struct, // non-trivial C struct. +NTFK_Array// array that has non-trivial elements. + }; ahatanak wrote: > rjmccall wrote: > > ahatanak wrote: > > > rjmccall wrote: > >

[PATCH] D41433: Unit tests for TBAA metadata generation.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Interesting. Okay, LGTM. Repository: rC Clang https://reviews.llvm.org/D41433 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

[PATCH] D41677: Change memcpy/memove/memset to have dest and source alignment attributes.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I'm glad to hear that progress is finally happening on this. The change to CGBuilder looks good to me. I'm going to take your word for it that the test changes are all just obvious update

[PATCH] D41539: [CodeGen] Decorate aggregate accesses with TBAA tags

rjmccall added a comment. You're sure you just want a single TBAA tag on memcpy's? I would think that it might be useful to encode separate path information for the two operands. https://reviews.llvm.org/D41539 ___ cfe-commits mailing list cfe-com

[PATCH] D41547: [CodeGen] Fix TBAA info for accesses to members of base classes

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

[PATCH] D41562: [CodeGen] Generate TBAA info on passing arguments and returning values

rjmccall added a comment. Aren't these always loads and stores from allocas? I would expect TBAA to be useless here because it's always strictly less informative than basic-AA, which knows that the access doesn't alias with anything. Repository: rL LLVM https://reviews.llvm.org/D41562 _

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:239 + bit IncludeC = includeC; +} I have no objection to allowing ObjC attributes to be spelled in [[clang::objc_whatever]] style. We can debate giving them a more appropriate standard

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:239 + bit IncludeC = includeC; +} aaron.ballman wrote: > rjmccall wrote: > > I have no objection to allowing ObjC attributes to be spelled in > > [[clang::objc_whatever]] style. We can d

[PATCH] D41039: Add support for attribute "trivial_abi"

rjmccall added a comment. In https://reviews.llvm.org/D41039#951807, @rjmccall wrote: > In https://reviews.llvm.org/D41039#951648, @ahatanak wrote: > > > I had a discussion with Duncan today and he pointed out that perhaps we > > shouldn't allow users to annotate a struct with "trivial_abi" if o

[PATCH] D41039: Add support for attribute "trivial_abi"

rjmccall added a comment. I'll trust Richard on the tricky Sema/AST bits. The functionality of the patch looks basically acceptable to me, although I'm still not thrilled about the idea of actually removing the attribute from the AST rather than just letting it not have effect. But we could c

[PATCH] D41039: Add support for attribute "trivial_abi"

rjmccall added inline comments. Comment at: include/clang/AST/Type.h:811 + bool hasTrivialABIOverride() const; + This should get a comment, even if it's just to refer to the CXXRecordDecl method. Comment at: lib/CodeGen/CGCall.cpp:3498 b

[PATCH] D41539: [CodeGen] Decorate aggregate accesses with TBAA tags

rjmccall added a comment. Yes, I think that's fine. I think unconditionally attaching the tag is probably wrong; you need suppress it in the may_alias cases. I'll grant that the existing code doesn't honor that, but that seems like a bug we should go ahead and fix. You'll need to either prop

[PATCH] D41039: Add support for attribute "trivial_abi"

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, looks good to me. https://reviews.llvm.org/D41039 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:239 + bit IncludeC = includeC; +} aaron.ballman wrote: > rjmccall wrote: > > aaron.ballman wrote: > > > rjmccall wrote: > > > > I have no objection to allowing ObjC attributes to be spelle

[PATCH] D43805: Optionally use nameless IR types

rjmccall added inline comments. Comment at: include/clang/Driver/Options.td:1735 + HelpText<"Whether to use IR type names (option: none, use)">, + Values<"none,use">; def relocatable_pch : Flag<["-", "--"], "relocatable-pch">, Flags<[CC1Option]>, This is an un

[PATCH] D43839: Add an option to disable tail-call optimization for escaping blocks

rjmccall added a comment. Abandon this one, then, please. https://reviews.llvm.org/D43839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43839: Add an option to disable tail-call optimization for escaping blocks

rjmccall added a comment. Oh, sorry, somehow I missed that it was abandoned. https://reviews.llvm.org/D43839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43842: CodeGenObjCXX: handle inalloca appropriately for msgSend variant

rjmccall added a comment. Ugh, I hate `inalloca` *so much*. It's still an indirect return, right? It's just that the return-slot pointer has to get stored to the `inalloca` allocation like any other argument? Repository: rC Clang https://reviews.llvm.org/D43842 _

[PATCH] D43842: CodeGenObjCXX: handle inalloca appropriately for msgSend variant

rjmccall added a comment. Okay. In that case, this seems correct, although it seems to me that perhaps `inalloca` is not actually orthogonal to anything else. In fact, it seems to me that maybe `inalloca` ought to just be a bit on the CGFunctionInfo and the individual ABIInfos should be left

[PATCH] D43842: CodeGenObjCXX: handle inalloca appropriately for msgSend variant

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Alright, yeah, we can fix that later. LGTM. Repository: rC Clang https://reviews.llvm.org/D43842 ___ cfe-commits mailing list cfe-commits

[PATCH] D43841: Add an option to disable tail-call optimization for escaping blocks

rjmccall added a comment. Can you explain the rationale for limiting this to escaping blocks in more depth? That seems like an extremely orthogonal limitation; the user might be thinking about a very specific block and not really considering this in general. https://reviews.llvm.org/D43841

[PATCH] D43841: Add an option to disable tail-call optimization for escaping blocks

rjmccall added a comment. TCO is a pretty neglible optimization; its primary advantage is slightly better locality for stack memory. I guess the more compelling argument is that non-escaping blocks can by definition only be executed with the original block-creating code still active, so someon

[PATCH] D43841: Add an option to disable tail-call optimization for escaping blocks

rjmccall added inline comments. Comment at: include/clang/Driver/Options.td:1419 +def fno_disable_tail_calls_escaping_blocks : Flag<["-"], "fno-disable-tail-calls-escaping-blocks">, Group, Flags<[CC1Option]>; +def fdisable_tail_calls_escaping_blocks : Flag<["-"], "fdisable-tail

[PATCH] D43842: CodeGenObjCXX: handle inalloca appropriately for msgSend variant

rjmccall added a comment. That's a fair point. I agree that separate allocas would make this a lot cleaner, in both IR and frontend implementation. We could also set an inalloca bit (+ field index? or maybe keep the arg index -> field index mapping on the CGFunctionInfo) on each arg info *in

[PATCH] D43841: Add an option to disable tail-call optimization for escaping blocks

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:4846 +if (auto *BE = dyn_cast(Arg->IgnoreParenNoopCasts(Context))) + BE->getBlockDecl()->setDoesNotEscape(); + Can this be based on the noescape parameter bit on the function type?

[PATCH] D43841: Add an option to disable tail-call optimization for escaping blocks

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Alright, this looks good to me. Comment at: lib/Sema/SemaExpr.cpp:4846 +if (auto *BE = dyn_cast(Arg->IgnoreParenNoopCasts(Context))) + BE->getBlockDecl()

[PATCH] D43995: Do not generate calls to fentry with __attribute__((no_instrument_function))

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Seems reasonable to me. Repository: rC Clang https://reviews.llvm.org/D43995 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:

[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision

rjmccall added inline comments. Comment at: include/clang/AST/Expr.h:875 + /// is set to true. + bool IsUnique = false; + Humor me and pack this in the bitfields in Stmt, please. :) Comment at: include/clang/AST/Expr.h:932 + void setIsUniq

[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision

rjmccall added a comment. Oh, and you need to serialize this bit. https://reviews.llvm.org/D39562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44095: [ObjC] Allow declaring __weak pointer fields in C structs in ObjC-ARC

rjmccall added inline comments. Comment at: include/clang/AST/Decl.h:3631 +PassedIndirectly = true; + } + I feel like this flag should be set by Sema for C++ types that have to be passed indirectly as well; it can then become the single point of truth for

[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision

rjmccall added inline comments. Comment at: include/clang/AST/Expr.h:875 + /// is set to true. + bool IsUnique = false; + rjmccall wrote: > Humor me and pack this in the bitfields in Stmt, please. :) Thanks! Comment at: lib/CodeGen/CGExpr.cpp

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

rjmccall added a comment. I'm sorry, I did a review of your patch on Phabricator but apparently never submitted it. Comment at: lib/CodeGen/CGCall.h:219 + RValue RV; + LValue LV; /// The l-value from which the argument is derived. +}; This could

[PATCH] D44221: Avoid including ScopeInfo.h from Sema.h

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

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3427 (void)InitialArgSize; -RValue RVArg = Args.back().RV; -EmitNonNullArgCheck(RVArg, ArgTypes[Idx], (*Arg)->getExprLoc(), AC, -ParamsToSkip + Idx); -// @llvm.objectsize s

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

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

[PATCH] D44039: [Sema] Make getCurFunction() return null outside function parsing

rjmccall added a comment. I agree that having those sites just no-op themselves is the cleanest approach. https://reviews.llvm.org/D44039 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm

[PATCH] D44095: [ObjC] Allow declaring __weak pointer fields in C structs in ObjC-ARC

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks. Comment at: lib/CodeGen/CGNonTrivialStruct.cpp:764 +Object = CGF->EmitObjCConsumeObject(QT, Object); +CGF->EmitARCStoreWeak(Addrs[DstIdx], Object, t

[PATCH] D44435: Add the module name to __cuda_module_ctor and __cuda_module_dtor for unique function names

rjmccall added inline comments. Comment at: lib/CodeGen/CGCUDANV.cpp:281 + // get name from the module to generate unique ctor name for every module + SmallString<128> ModuleName Please explain in the comment *why* you're doing this. It's just for debugging

[PATCH] D44445: CodeGen: Reduce LValue and CallArgList memory footprint before recommitting r326946

rjmccall added inline comments. Comment at: lib/CodeGen/CGValue.h:234 this->Quals = Quals; this->Alignment = Alignment.getQuantity(); assert(this->Alignment == Alignment.getQuantity() && Please saturate Alignment here instead of allowing it to be t

[PATCH] D44445: CodeGen: Reduce LValue and CallArgList memory footprint before recommitting r326946

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

[PATCH] D44435: Add the module name to __cuda_module_ctor and __cuda_module_dtor for unique function names

rjmccall added inline comments. Comment at: lib/CodeGen/CGCUDANV.cpp:281 + // get name from the module to generate unique ctor name for every module + SmallString<128> ModuleName SimeonEhrig wrote: > tra wrote: > > SimeonEhrig wrote: > > > rjmccall wrote: > >

[PATCH] D44445: CodeGen: Reduce LValue and CallArgList memory footprint before recommitting r326946

rjmccall added a comment. Yeah, I think the test is probably a bit abusive to run unconditionally for all hosts. We can just know that we improved things, and if we regress, it will break them again. Repository: rC Clang https://reviews.llvm.org/D5 __

[PATCH] D44435: Add the module name to __cuda_module_ctor and __cuda_module_dtor for unique function names

rjmccall added inline comments. Comment at: lib/CodeGen/CGCUDANV.cpp:281 + // get name from the module to generate unique ctor name for every module + SmallString<128> ModuleName v.g.vassilev wrote: > rjmccall wrote: > > SimeonEhrig wrote: > > > tra wrote: >

[PATCH] D44435: Add the module name to __cuda_module_ctor and __cuda_module_dtor for unique function names

rjmccall added inline comments. Comment at: lib/CodeGen/CGCUDANV.cpp:281 + // get name from the module to generate unique ctor name for every module + SmallString<128> ModuleName rsmith wrote: > rjmccall wrote: > > v.g.vassilev wrote: > > > rjmccall wrote: >

[PATCH] D44536: Avoid segfault when destructor is not yet known

rjmccall added a comment. I'm not sure it's supposed to be a valid state to get into IRGen with a non-trivial destructor that isn't yet declared. Richard? Repository: rC Clang https://reviews.llvm.org/D44536 ___ cfe-commits mailing list cfe-com

[PATCH] D44536: Avoid segfault when destructor is not yet known

rjmccall added a comment. Hmm. Sema is lazy about actually creating implicit destructor declarations, but it's supposed to only do it whenever the destructor is actually used for something. I suspect that Sema just thinks that nothing is using c::~c, because the only thing that does use it is

[PATCH] D44536: Avoid segfault when destructor is not yet known

rjmccall added a comment. In https://reviews.llvm.org/D44536#1039428, @rjmccall wrote: > Hmm. Sema is lazy about actually creating implicit destructor declarations, > but it's supposed to only do it whenever the destructor is actually used for > something. I suspect that Sema just thinks that

[PATCH] D17893: Sema: Add semantic analysis for the C++ ABI stability attributes and whitelist.

rjmccall added a comment. I think this is okay. We can review further if we see other problems. https://reviews.llvm.org/D17893 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D44505: [MS] Always use base dtors in place of complete/vbase dtors when possible

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

[PATCH] D44559: [Sema] Wrong width of result of mul operation

rjmccall added a comment. The purpose of this analysis is not to compute the theoretical information content of the computation result. We are conservative about bounds precisely so that we do not warn in situations like these. Repository: rC Clang https://reviews.llvm.org/D44559 __

[PATCH] D44559: [Sema] Wrong width of result of mul operation

rjmccall added a comment. I think we're correct not to warn here and that GCC/ICC are being noisy. The existence of a temporary promotion to a wider type doesn't justify warning on arithmetic between two operands that are the same size as the ultimate result. It is totally fair for users to t

[PATCH] D44559: [Sema] Wrong width of result of mul operation

rjmccall added a comment. In https://reviews.llvm.org/D44559#1040928, @lebedev.ri wrote: > In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote: > > > I think we're correct not to warn here and that GCC/ICC are being noisy. > > The existence of a temporary promotion to a wider type doesn

[PATCH] D44559: [Sema] Wrong width of result of mul operation

rjmccall added a comment. In https://reviews.llvm.org/D44559#1041002, @lebedev.ri wrote: > In https://reviews.llvm.org/D44559#1041001, @rjmccall wrote: > > > In https://reviews.llvm.org/D44559#1040928, @lebedev.ri wrote: > > > > > In https://reviews.llvm.org/D44559#1040799, @rjmccall wrote: > > >

[PATCH] D44640: [CodeGen] Add funclet token to ARC marker

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. We should really just switch the reclaim to be marked with a bundle in the first place, but that's not a reasonable thing to ask you to do. Repository: rC Clang https://reviews.

[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision

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

[PATCH] D44536: Avoid segfault when destructor is not yet known

rjmccall added a comment. I think Richard is probably catching up from a week at the C++ committee. To be clear, I am objecting to this; I think the destructor should clearly have been created at this point. I'm just hoping Richard will have an idea for how best to fix it. Repository: rC C

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

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

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

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

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

rjmccall added a comment. Is there a specific reason to change the implementation instead of changing the documentation? Repository: rC Clang https://reviews.llvm.org/D47627 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

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

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

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

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

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

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

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

rjmccall added a comment. In https://reviews.llvm.org/D47564#1119874, @smeenai wrote: > In https://reviews.llvm.org/D47564#1118424, @rjmccall wrote: > > > No, it was just a general question. Have you gotten this to a point where > > it's testable? > > > Yup, it's been working fine in my local t

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

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

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

rjmccall added a comment. Well, Sema should always be diagnosing conflicts. If you don't have a specific reason to allow replacement, I would prefer just fixing the documentation to state this as a precondition. Repository: rC Clang https://reviews.llvm.org/D47627 ___

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

rjmccall added a comment. In https://reviews.llvm.org/D47694#1120375, @yaxunl wrote: > In https://reviews.llvm.org/D47694#1120367, @rjmccall wrote: > > > Why not just have the driver disable RTTI in the frontend invocation? > > > CUDA/HIP uses single source for device and host. The host code may

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

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

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

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, We can try this, then. Repository: rC Clang https://reviews.llvm.org/D47564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

rjmccall added a comment. Oh, I see, because you're worried that the host code might contain `dynamic_cast` or similar features which would complain if RTTI were disabled. `getLangOpts().CUDAIsDevice` implies `getLangOpts().CUDA`, so I think you can just check the former. Otherwise LGTM. htt

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

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

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

rjmccall added inline comments. Comment at: lib/Sema/SemaObjCProperty.cpp:2554 + PropertyTy->isObjCRetainableType() && + !PropertyTy->isObjCClassType()) { +Diag(Loc, diag::warn_objc_property_assign_on_object); Please use `isObjCARCImplicitlyUnretain

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

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:13010 + if (SemaRef.getLangOpts().OpenCLCPlusPlus) { +if (auto *PtrTy = ResultType.getTypePtr()->getAs()) { + ResultType = RemoveAddressSpaceFromPtr(SemaRef, PtrTy); `getTypePtr()` is

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

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

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

rjmccall added a reviewer: t.p.northover. rjmccall added a comment. Okay, as a code change this seems more reasonable to me. I haven't really thought through the ABI-compatibility issues, though. CC'ing Tim. https://reviews.llvm.org/D46013 ___ cf

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

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

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

rjmccall accepted this revision. rjmccall added a comment. LGTM. Comment at: lib/Sema/SemaExprCXX.cpp:2030 + } +} svenvh wrote: > rjmccall wrote: > > Anastasia wrote: > > > svenvh wrote: > > > > rjmccall wrote: > > > > > I think a better interpretati

[PATCH] D48040: Implement constexpr __builtin_*_overflow

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

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

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

[PATCH] D48040: Implement constexpr __builtin_*_overflow

rjmccall added a comment. Thanks, comment change looks good. LGTM. https://reviews.llvm.org/D48040 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

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

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

rjmccall added a comment. In https://reviews.llvm.org/D47233#1129110, @smeenai wrote: > Wrapping an Objective-C exception inside a C++ exception means dynamically > constructing a C++ exception object and traversing the class hierarchy of the > thrown Obj-C object to populate the catchable type

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

rjmccall added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:784 +return !hasDefinition() || !isDynamicClass() || hasAnyDependentBases(); + } + Both of these new methods deserve doc comments explaining that they're conservative checks becau

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1623 + const CXXRecordDecl *SourceClassDecl = + E->getType().getTypePtr()->getPointeeCXXRecordDecl(); + const CXXRecordDecl *DstClassDecl = DestTy->getPointeeCXXRecordDecl(); ---

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGObjCMac.cpp:7457-7460 CGObjCNonFragileABIMac::GetEHType(QualType T) { // There's a particular fixed type info for 'id'. if (T->isObjCIdType() || T->isObjCQualifiedIdType()) { +if (CGM.getTriple().isWindowsMSVCEn

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

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

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

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

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGObjCMac.cpp:7457-7460 CGObjCNonFragileABIMac::GetEHType(QualType T) { // There's a particular fixed type info for 'id'. if (T->isObjCIdType() || T->isObjCQualifiedIdType()) { +if (CGM.getTriple().isWindowsMSVCEn

[PATCH] D48531: [CodeGen] Provide source locations for UBSan type checks when emitting constructor calls.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D48531 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types

rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:1954 + llvm::APInt getFixedPointMin(QualType Ty) const; + llvm::APInt getFixedPointOne(QualType Ty) const; Are these opaque bit-patterns? I think there should be a type which abstract

[PATCH] D53046: [Sema] Fix an error-on-valid with friends and templates

rjmccall added a comment. The linking does actually happen in this test case, right? Can we just do something when linking them to unify their `Common` structures? Repository: rC Clang https://reviews.llvm.org/D53046 ___ cfe-commits mailing lis

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

rjmccall added a comment. In https://reviews.llvm.org/D50616#1259769, @leonardchan wrote: > @ebevhan @rjmccall Seeing that the saturation intrinsic in > https://reviews.llvm.org/D52286 should be put on hold for now, would it be > fine to submit this patch as is? Then if the intrinsic is finishe

[PATCH] D53046: [Sema] Fix an error-on-valid with friends and templates

rjmccall added a comment. In https://reviews.llvm.org/D53046#1259945, @erik.pilkington wrote: > In https://reviews.llvm.org/D53046#1259933, @rjmccall wrote: > > > The linking does actually happen in this test case, right? Can we just do > > something when linking them to unify their `Common` st

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