[PATCH] D49457: DR330: when determining whether a cast casts away constness, consider qualifiers from all levels matching a multidimensional array

rjmccall added inline comments. Comment at: lib/Sema/SemaCast.cpp:535 +T1 = Unwrap(T1); +T2 = Unwrap(T2).withCVRQualifiers(T2.getCVRQualifiers()); + } rsmith wrote: > rjmccall wrote: > > Hmm. Just CVR? I understand that we can have problems here with

[PATCH] D49294: Sema: Fix explicit address space cast in C++

rjmccall added inline comments. Comment at: lib/Sema/SemaOverload.cpp:3150 + !getLangOpts().OpenCLCPlusPlus) +return false; + yaxunl wrote: > rjmccall wrote: > > It's not really OpenCL C++ that's special here, it's the possibility of > > promotions

[PATCH] D49083: [HIP] Register/unregister device fat binary only once

rjmccall added inline comments. Comment at: lib/CodeGen/CGCUDANV.cpp:444 +auto HandleValue = +CtorBuilder.CreateAlignedLoad(GpuBinaryHandle, CGM.getPointerAlign()); +llvm::Constant *Zero = llvm::Constant::getNullValue(HandleValue->getType());

[PATCH] D49688: [Sema] Fix a crash when a BlockExpr appears in a default-member-initializer of a class template

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. It makes sense that the BlockDecl's parent DC is the class, although I think it would be even better if we took a page from Swift and make a special DC for initializer expressions. I

[PATCH] D49403: More aggressively complete RecordTypes with Function Pointers

rjmccall added a comment. Are you sure you shouldn't do it based on some sort of actual annotation based on the frontend's knowledge of types instead of adding semantics for LLVM IR types that were never intended? https://reviews.llvm.org/D49403

[PATCH] D49688: [Sema] Fix a crash when a BlockExpr appears in a default-member-initializer of a class template

rjmccall added a comment. I honestly don't know why the `BlockDecl` is in the members list in the first place; that seems wrong, for the same reason that we wouldn't (I assume?) consider a lambda's implicit record to be a member. Repository: rC Clang https://reviews.llvm.org/D49688

[PATCH] D49403: More aggressively complete RecordTypes with Function Pointers

rjmccall added a comment. Sorry, I'm not following you. Are you doing some sort of type-based security analysis based on LLVM IR types? https://reviews.llvm.org/D49403 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D49718: [CodeGen][ObjC] Make block copy/dispose helper function exception-safe.

rjmccall added a comment. In https://reviews.llvm.org/D49718#1173038, @ahatanak wrote: > Note that in order to destruct C++ objects, I'm using pushDestroy which > pushes a NormalCleanup when exception handling isn't enabled. This is > different from PushDestructorCleanup which always pushes a

[PATCH] D49589: [UBSan] Strengthen pointer checks in 'new' expressions

rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:380 + /// True if sanitizer checks for current pointer value are generated. + bool PointerChecksAreEmitted = false; + sepavloff wrote: > rjmccall wrote: > > I don't think this is a good

[PATCH] D49718: [CodeGen][ObjC] Make block copy/dispose helper function exception-safe.

rjmccall added a comment. In https://reviews.llvm.org/D49718#1173883, @ahatanak wrote: > Address review comments. > > I think I should be able to merge pushBlockObjectRelease and > enterByrefCleanup, but the BlockFieldFlags that are passed are different. We > set BLOCK_FIELD_IS_WEAK in

[PATCH] D49643: [HIP] Add -target-cpu option for clang -cc1

rjmccall added a comment. The commit message here could be better. You're passing `-target-cpu` when running the device-mode compiler. https://reviews.llvm.org/D49643 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D49589: [UBSan] Strengthen pointer checks in 'new' expressions

rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:380 + /// True if sanitizer checks for current pointer value are generated. + bool PointerChecksAreEmitted = false; + I don't think this is a good way of doing this. Using global state

[PATCH] D49643: [HIP] pass `-target-cpu` when running the device-mode compiler

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

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

rjmccall added inline comments. Comment at: include/clang/Basic/FixedPoint.h:64 + +class APFixedPoint { + public: rjmccall wrote: > This should get a documentation comment describing the class. You should > mention that, like `APSInt`, it carries all of the

[PATCH] D49083: [HIP] Register/unregister device fat binary only once

rjmccall added a comment. Thanks for the comment. Comment at: lib/CodeGen/CGCUDANV.cpp:444 +auto HandleValue = +CtorBuilder.CreateAlignedLoad(GpuBinaryHandle, CGM.getPointerAlign()); +llvm::Constant *Zero = llvm::Constant::getNullValue(HandleValue->getType());

[PATCH] D49508: [CodeGen] VisitMaterializeTemporaryExpr(): don't skip NoOp Casts.

rjmccall added a comment. I think it would be reasonable to set a flag on `ImplicitCastExpr`s that are actually semantically part of an explicit cast. I don't think that would be hard to get Sema to do, either by passing a flag down to the code that builds those casts or just by retroactively

[PATCH] D49083: [HIP] Register/unregister device fat binary only once

rjmccall added inline comments. Comment at: lib/CodeGen/CGCUDANV.cpp:444 +auto HandleValue = +CtorBuilder.CreateAlignedLoad(GpuBinaryHandle, CGM.getPointerAlign()); +llvm::Constant *Zero = llvm::Constant::getNullValue(HandleValue->getType());

[PATCH] D49508: [Sema] Expr::skipRValueSubobjectAdjustments(): record skipped NoOp casts.

rjmccall added a comment. In https://reviews.llvm.org/D49508#1168599, @lebedev.ri wrote: > In https://reviews.llvm.org/D49508#1168584, @rjmccall wrote: > > > Hmm. I think the approach of flagging ICEs that are semantically part of > > an explicit cast is probably a better representation for

[PATCH] D49303: [CodeGen][ObjC] Treat non-escaping blocks as global blocks to make copy/dispose a no-op

rjmccall added inline comments. Comment at: docs/Block-ABI-Apple.rst:64 enum { +BLOCK_IS_NOESCAPE = (1 << 23), BLOCK_HAS_COPY_DISPOSE = (1 << 25), Something happened to my older comments here, but please document the meaning of this

[PATCH] D49085: [Sema] Emit a diagnostic for an invalid dependent function template specialization

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4111 + "candidate template ignored: %select{template is not a function template" + "|is not a member of the enclosing namespace}0">; Your first explanation has a

[PATCH] D49508: [Sema] Expr::skipRValueSubobjectAdjustments(): record skipped NoOp casts.

rjmccall added a comment. Hmm. I think the approach of flagging ICEs that are semantically part of an explicit cast is probably a better representation for tools across the board. If we *are* going to do it this way, though, I think you should (1) make the collection of skipped expressions

[PATCH] D49294: Sema: Fix explicit address space cast in C++

rjmccall added inline comments. Comment at: lib/Sema/SemaOverload.cpp:3150 + !getLangOpts().OpenCLCPlusPlus) +return false; + yaxunl wrote: > rjmccall wrote: > > yaxunl wrote: > > > rjmccall wrote: > > > > It's not really OpenCL C++ that's special here,

[PATCH] D49085: [Sema] Emit a diagnostic for an invalid dependent function template specialization

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4111 + "candidate template ignored: %select{template is not a function template" + "|is not a member

[PATCH] D49718: [CodeGen][ObjC] Make block copy/dispose helper function exception-safe.

rjmccall added a comment. In https://reviews.llvm.org/D49718#1174281, @ahatanak wrote: > Merge pushBlockObjectRelease and enterByrefCleanup. > > In https://reviews.llvm.org/D49718#1174113, @rjmccall wrote: > > > Heh, okay. It looks like the runtime doesn't do anything different, but I > >

[PATCH] D49403: More aggressively complete RecordTypes with Function Pointers

rjmccall added a comment. In https://reviews.llvm.org/D49403#1175350, @olga.chupina wrote: > I should probably add one more example to explain my point of view. > Suppose we have an indirect call in the program and we need to know all > possible goals for this indirect call. Then we would like

[PATCH] D49589: [UBSan] Strengthen pointer checks in 'new' expressions

rjmccall added inline comments. Comment at: lib/CodeGen/Address.h:46 +return IsChecked; + } + `Address` is a pretty low-level type to be changing here. Is this necessary? Can you find a way to propagate this just in higher-level types like

[PATCH] D51426: [Sema] Prohibit function-scope compound literals with address spaces.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, with one minor suggestion. Comment at: lib/Sema/SemaExpr.cpp:5745 +// C99 6.5.2.5p6: Function scope compound literals must have automatic +// storage which

[PATCH] D51564: Distinguish `__block` variables that are captured by escaping blocks from those that aren't

rjmccall added inline comments. Comment at: include/clang/AST/Decl.h:977 + +unsigned EscapingByref : 1; }; This doesn't actually seem to be initialized anywhere. Comment at: include/clang/AST/Decl.h:1422 + + bool isNonEscapingByref()

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

rjmccall added inline comments. Comment at: lib/Sema/SemaChecking.cpp:4924 +<< Callee->getSourceRange(); + } + rjmccall wrote: > Why is the diagnostic at the end location? And why are you separately > checking whether it's ignored at the begin

[PATCH] D51200: Introduce per-callsite inline intrinsics

rjmccall added a comment. In https://reviews.llvm.org/D51200#1223768, @kuhar wrote: > In https://reviews.llvm.org/D51200#1223752, @rsmith wrote: > > > +rjmccall for CodeGen changes > > > @rsmith @rjmccall > I have one high-level question about the CodeGen part that I wasn't able to > figure

[PATCH] D51564: Distinguish `__block` variables that are captured by escaping blocks from those that aren't

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: include/clang/AST/Decl.h:977 + +unsigned EscapingByref : 1; }; ahatanak wrote: > rjmccall wrote: > > This doesn't actually seem

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

rjmccall closed this revision. rjmccall added a comment. Committed as r341489. Repository: rC Clang https://reviews.llvm.org/D44539 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D51426: [Sema] Prohibit function-scope compound literals with address spaces.

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

[PATCH] D51084: Implement -Watomic-implicit-seq-cst

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lib/Sema/SemaChecking.cpp:10974 + if (E->IgnoreParenImpCasts()->getType()->isAtomicType()) +return; CheckImplicitConversion(S,

[PATCH] D51426: [Sema] Prohibit function-scope compound literals with address spaces.

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

[PATCH] D51426: [Sema] Prohibit function-scope compound literals with address spaces.

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:5745 +// C99 6.5.2.5p6: Function scope compound literals must have automatic +// storage which generally excludes address space-qualified ones. +Diag(LParenLoc,

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

rjmccall added inline comments. Comment at: test/SemaObjC/property-in-class-extension-1.m:18 -@property (assign, readonly) NSString* changeMemoryModel; // expected-note {{property declared here}} +@property (unsafe_unretained, readonly) NSString* changeMemoryModel; //

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

rjmccall added a comment. LGTM, but I'd like Richard to sign off, too. Repository: rC Clang https://reviews.llvm.org/D45898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

rjmccall added inline comments. Comment at: include/clang/Basic/FixedPoint.h:31 + SatNoPadding, +}; + leonardchan wrote: > ebevhan wrote: > > rjmccall wrote: > > > I figured you'd want this to be a struct which include the scale, width, > > > signed-ness, and

[PATCH] D42154: Don't generate inline atomics for i386/i486

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Well, my point is that the example in the linked bug is asking for 486 code-generation, which is apparently unsupported by LLVM. Anyway, it's not a good reason to hold up this patch,

[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: > > > > aaron.ballman wrote: > > > > > rjmccall wrote: > > > > > >

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

rjmccall accepted this revision. rjmccall added a comment. Looks great to me! Thanks for taking this on, it's a pretty major improvement for users. Would you like to create an issue with itanium-cxx-abi to document this, or do you want me to handle that? https://reviews.llvm.org/D41039

[PATCH] D42092: implement C++ dr388 for the Itanium C++ ABI: proper handling of catching exceptions by reference

rjmccall added a comment. This is definitely something that the personality function should handle. If we want to do heroic things in the absence of personality function support, we can, but the code should at least be written to be conditional on personality support. If we can rev the

[PATCH] D42366: [CodeGen] Fix generation of TBAA tags for may-alias accesses

rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenTBAA.h:67 /* BaseType= */ nullptr, /* AccessType= */ nullptr, - /* Offset= */ 0, /* Size= */ 0); + /* Offset= */ 0, /* Size= */

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

rjmccall added a comment. Thank you. Maybe there should be an overload of EmitAggregateCopy that takes LValues? A lot of these cases start with an LValue on at least one side, and there are already some convenience functions to turn an Address into a naturally-typed LValue.

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

[PATCH] D42154: Don't generate inline atomics for i386/i486

rjmccall added a comment. In https://reviews.llvm.org/D42154#977991, @wmi wrote: > In https://reviews.llvm.org/D42154#977975, @efriedma wrote: > > > The LLVM backend currently assumes every CPU is Pentium-compatible. If > > we're going to change that in clang, we should probably fix the

[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

[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

[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

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

[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

[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] 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,

[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

[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] 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] 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] 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] 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

[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<["-"],

[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] 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

[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

[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

[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

[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

[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

[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

[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] 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] D45289: Disable -fmerge-all-constants as default.

rjmccall added inline comments. Comment at: include/clang/Driver/Options.td:1286 def fno_merge_all_constants : Flag<["-"], "fno-merge-all-constants">, Group, Flags<[CC1Option]>, HelpText<"Disallow merging of constants">; def fno_modules : Flag <["-"], "fno-modules">,

[PATCH] D45289: Disable -fmerge-all-constants as default.

rjmccall added a comment. It's not unreasonable to just add -fmerge-all-constants to the command line invocations for the individual tests, yeah. We can take those off as necessary later. Repository: rC Clang https://reviews.llvm.org/D45289

[PATCH] D45384: [ObjC++] Never pass structs with `__weak` fields in registers

rjmccall added a comment. Well, but I think CanPassInRegisters==false in the base class does always mean CanPassInRegisters==false in the subclass. Repository: rC Clang https://reviews.llvm.org/D45384 ___ cfe-commits mailing list

[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

rjmccall added a comment. Hmm. I'm not actually sure *why* it's not okay to forward callee-cleanup arguments. Do we just not have the necessary logic to disable the cleanup in the caller? Repository: rC Clang https://reviews.llvm.org/D45382

[PATCH] D44616: Update existed CodeGen TBAA tests

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

[PATCH] D42092: implement C++ dr388 for the Itanium C++ ABI: proper handling of catching exceptions by reference

rjmccall added a comment. In https://reviews.llvm.org/D42092#1058841, @rsmith wrote: > In https://reviews.llvm.org/D42092#1058772, @rjmccall wrote: > > > Issue #3 is tricky; it's arguably not okay to force a landing at a landing > > pad if we're not actually catching anything. > > > I think the

[PATCH] D45306: PR36992 don't overwrite virtual bases in tail padding

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

[PATCH] D45384: [ObjC++] Never pass structs with `__weak` fields in registers

rjmccall added a comment. In https://reviews.llvm.org/D45384#1060369, @ahatanak wrote: > Yes. I intended it as a property that propagates to classes that contain or > derive from the type. > > Would it make it less confusing if I merged CXXRecordDecl::CanPassInRegisters > and

[PATCH] D45384: [ObjC++] Never pass structs with `__weak` fields in registers

rjmccall added a comment. In https://reviews.llvm.org/D45384#1060192, @ahatanak wrote: > In https://reviews.llvm.org/D45384#1060164, @rjmccall wrote: > > > Well, but I think CanPassInRegisters==false in the base class does always > > mean CanPassInRegisters==false in the subclass. > > > I think

[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

rjmccall added a comment. If that's the problem, then I think the right design is for CallArg to include an optional cleanup reference for a cleanup that can be deactivated at the instant of the call (we should assert that this exists for parameters that are destroyed in the callee), and then

[PATCH] D45212: [HIP] Let CUDA toolchain support HIP language mode and amdgpu

rjmccall added a comment. This looks okay to me, but I think it would better if someone with more expertise in the design of the driver and frontend code could review this. Comment at: lib/Driver/Driver.cpp:2267 +if ((IA->getType() != types::TY_CUDA) && +

[PATCH] D45384: [ObjC++] Never pass structs with `__weak` fields in registers

rjmccall added a comment. Just a couple minor requests; if you accept them, feel free to commit. Comment at: include/clang/AST/Decl.h:3556 +/// indirectly. This value is used only in C++. +APK_CannotPassInRegs, + I think it's probably worth spelling

[PATCH] D44580: Sema: allow comparison between blocks & block-compatible objc types

rjmccall closed this revision. rjmccall added a comment. Committed as r329508. Repository: rC Clang https://reviews.llvm.org/D44580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45305: ObjCGNU: Fix empty v3 protocols being emitted two fields short

rjmccall accepted this revision. rjmccall added a comment. Hmm. Alright, I guess. Repository: rC Clang https://reviews.llvm.org/D45305 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45212: [HIP] Let CUDA toolchain support HIP language mode and amdgpu

rjmccall added inline comments. Comment at: include/clang/Basic/Cuda.h:61 + GFX900, + GFX902, LAST, yaxunl wrote: > rjmccall wrote: > > Does this actually have anything to do with HIP? You have a lot of changes > > in this patch which seem to just be

[PATCH] D45277: [CUDA] Add amdgpu sub archs

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks for splitting the patch up. LGTM. https://reviews.llvm.org/D45277 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D44984: [HIP] Add hip file type and codegen for kernel launching

rjmccall added inline comments. Comment at: lib/Frontend/CompilerInvocation.cpp:2109 + Opts.HIP = true; + } + yaxunl wrote: > rjmccall wrote: > > Why is this done here? We infer the language mode from the input kind > > somewhere else. > It is usually

[PATCH] D45489: [HIP] Add input type for HIP

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

[PATCH] D45441: [HIP] Add predefined macros __HIPCC__ and __HIP_DEVICE_COMPILE__

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. If so, LGTM. Comment at: lib/Frontend/InitPreprocessor.cpp:473 + Builder.defineMacro("__HIP_DEVICE_COMPILE__"); + } } I assume these names are

[PATCH] D45412: [Sema] Fix PR22637 - IndirectFieldDecl's discard qualifiers during template instantiation.

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

[PATCH] D45411: [Sema] Fix PR35832 - Ambiguity accessing anonymous struct/union with multiple bases.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Well, that is a really silly bug. Fix LGTM. https://reviews.llvm.org/D45411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D45410: [Sema] Remove dead code in BuildAnonymousStructUnionMemberReference. NFCI

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I think there was a point when we weren't always creating CXXThisExprs eagerly for these accesses. Now that we are, yeah, this should be dead. Repository: rC Clang

[PATCH] D45578: Add a command line option 'fregister_dtor_with_atexit' to register destructor functions annotated with __attribute__((destructor)) using __cxa_atexit or atexit.

rjmccall added inline comments. Comment at: include/clang/Driver/Options.td:1613 +def fregister_dtor_with_atexit : Flag<["-"], "fregister-dtor-with-atexit">, Group, Flags<[CC1Option]>, + HelpText<"Use atexit or __cxa_atexit to register destructors">; def fuse_init_array :

[PATCH] D44984: [HIP] Add hip file type and codegen for kernel launching

rjmccall added inline comments. Comment at: lib/CodeGen/CGCUDANV.cpp:98 +std::string CGNVCUDARuntime::addPrefixToName(CodeGenModule , + std::string FuncName) const { + if (CGM.getLangOpts().HIP) Can you take these as

[PATCH] D45212: [HIP] Let CUDA toolchain support HIP language mode and amdgpu

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I'd still prefer if someone with more driver-design expertise weighed in, but we might not have any specialists there. LGTM, although you might consider changing your tests a bit:

<    3   4   5   6   7   8   9   10   11   12   >