[PATCH] D116861: [UBSan] Fix incorrect alignment reported when global new returns an offset pointer

rjmccall added a comment. If we want to assume a weaker alignment, we should change the calculation of `allocationAlign` rather than just changing this point downstream of that computation. But replacements of the standard `operator new` are restricted and cannot simply choose to return less-a

[PATCH] D116861: [UBSan] Fix incorrect alignment reported when global new returns an offset pointer

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprCXX.cpp:1658-1659 Target.getNewAlign(), getContext().getTypeSize(allocType))); allocationAlign = std::max( allocationAlign, getContext().toCharUnitsFromBits(AllocatorAlign)); } -

[PATCH] D119364: Refactor nested if else with ternary operator in CGExprScalar.cpp

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

rjmccall added a comment. Changing the C++03 POD definition is going to be substantially ABI-breaking at this point. Any logic to change it needs to be platform-specific. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.l

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

rjmccall added a comment. And this should be raised as an Itanium issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119051/new/ https://reviews.llvm.org/D119051 ___ cfe-commits mailing list cfe-commit

[PATCH] D119364: Refactor nested if else with ternary operator in CGExprScalar.cpp

rjmccall added a comment. It's nothing to do with your patch, and you don't need to worry about it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119364/new/ https://reviews.llvm.org/D119364 ___ cfe-comm

[PATCH] D112921: [clang] Enable sized deallocation by default in C++14 onwards

rjmccall added a comment. Ideally, I think, we would set this up to work something like `ObjCRuntime`, where we're making queries to a common place that contains all the information necessary to decide what runtime features are available. In particular, we shouldn't treat Apple platforms as fo

[PATCH] D112921: [clang] Enable sized deallocation by default in C++14 onwards

rjmccall added a comment. Hmm. Allowing a version on `-stdlib` is intuitively appealing, but I'm not sure it actually gives us the information we need. As I recall, `-stdlib` selects the high-level stdlib and not the low-level one, and those are related in code but not necessarily at runtime;

[PATCH] D124435: [X86] Always extend the integer parameters in callee

rjmccall added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:2988-2992 +.. option:: -mconservative-extend +Always extend the integer parameter both in the callee and caller. + +.. option:: -mno-conservative-extend +Keep the original integer parameter passi

[PATCH] D124435: [X86] Always extend the integer parameters in callee

rjmccall added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:2988-2992 +.. option:: -mconservative-extend +Always extend the integer parameter both in the callee and caller. + +.. option:: -mno-conservative-extend +Keep the original integer parameter passi

[PATCH] D124435: [X86] Always extend the integer parameters in callee

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1938 +return IsConservativeExtend ? ABIArgInfo::getConservativeExtend(Ty) +: ABIArgInfo::getExtend(Ty); } LiuChen3 wrote: > rjmccall wrote: > > Liu

[PATCH] D124211: Add __builtin_call_kcfi_unchecked

rjmccall added a comment. My only comment about the design is that maybe it should be `__builtin_kcfi_call_unchecked`, which does not seem like something you need to hold up LKML discussion for. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124211

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

rjmccall added a comment. IIRC, the reason it works it that way is that "warnings which default to an error" are really "errors which you can explicitly downgrade to a warning". Maybe those ought to be different categories, or maybe we ought to just be telling people to downgrade this specific

[PATCH] D124736: [CodeGen] Use ABI alignment for placement new

rjmccall added a comment. It should probably be the ABI alignment in all cases; I think the preferred alignment is just supposed to be used to opportunistically over-align things like e.g. local variables, which doesn't seem relevant for the ABI code involving a call to `operator new`. Reposi

[PATCH] D129464: [Clang][CodeGen] Set FP options of builder at entry to compound statement

rjmccall added a comment. This property adheres to a function definition, so it seems to me that an explicit *instantiation* ought to preserve it from the instantiated template definition, but an explicit *specialization* ought to be independent. i.e. #pragma float_control(precise, on, push)

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

rjmccall added a comment. Blocks that don't capture anything have always been allocated globally. This optimization is using the global ISA for blocks that do capture locals but which are known statically (on penalty of UB) to not escape the original scope of the block literal. Using the glob

[PATCH] D130420: [CodeGen] Consider MangleCtx when move lazy emission States

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I think this seems reasonable. I suspect we'll need to do more plumbing here if we need to support snapshotting / resetting to a snapshot, but this probably isn't the only place, and the

[PATCH] D124736: [CodeGen] Use ABI alignment for placement new

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/D124736/new/ https://reviews.llvm.org/D124736 ___

[PATCH] D124435: [X86] Always extend the integer parameters in callee

rjmccall added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:3028 +extend the small integer argument in both caller and callee. Using other +value can disable this, which may improve performance and code size. + > Specifies how to handle s

[PATCH] D125635: Move NumStmts from CompoundStmtBitfields to fields of CompoundStmt

rjmccall added a comment. On the one hand, I'm not sure 8M statements in a block — or 1M, for that matter — is an unreasonable implementation limit. On the other hand, this patch looks like it only changes overall memory use on 32-bit hosts, because `CompoundStmt` has a 32-bit field prior to i

[PATCH] D124435: [X86] Always extend the integer parameters in callee

rjmccall added a comment. I'm fine with alternatives on the names. I just don't think we want names that imply that the caller and callee are parallel here, as if there were some sort of requirement that the callee always extends. In those conventions, the callee gets passed 8 or 16 meaningfu

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

rjmccall added a comment. Okay, well, first off, we definitely shouldn't be repeating conditions like `isX86()` all over the place. What you want to do is to have a general predicate which answers whether we should emit this operation with excess precision; imagine an architecture that wanted

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

rjmccall added a comment. We definitely don't want ObjC to differ here; thanks for the CC. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125919/new/ https://reviews.llvm.org/D125919 ___ cfe-commits maili

[PATCH] D125635: Make CompoundStmtBitfields::NumStmts not a bit-field

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/D125635/new/ https://reviews.llvm.org/D125635 __

[PATCH] D64811: Warn when NumParams overflows

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:6233 + + } else if (RequiresArg) Diag(Tok, diag::err_argument_required_after_attribute); The re-indentation here is wrong. Comment at: clang/lib/Sema/SemaDeclCXX.

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

rjmccall added a comment. In D65744#1629055 , @Anastasia wrote: > In D65744#1627589 , @rjmccall wrote: > > > I see. Is the deduction rule recursive or something, where a pointer to > > pointer is inferred to point

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

rjmccall added inline comments. Comment at: clang/docs/UsersManual.rst:1299 +.. option:: -fp-model=[values] + This should be something like `-fp-model=`. Square brackets mean optional elements in these docs. Comment at: clang/docs/UsersManu

[PATCH] D66360: Expose constructing a virtual method dispatch through the include/clang/CodeGen APIs.

rjmccall added inline comments. Comment at: include/clang/CodeGen/SwiftCallingConv.h:190 +llvm::FunctionType *type, +llvm::IRBuilderBase *builder); + This isn't the ri

[PATCH] D62731: [RFC] Add support for options -fp-model= and -fp-speculation= : specify floating point behavior

rjmccall added inline comments. Comment at: clang/docs/UsersManual.rst:1305 + and ``noexcept``. Note that -fp-model=[no]except can be combined with the + other three settings for this option. Details: + mibintc wrote: > rjmccall wrote: > > Combined how? With

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

rjmccall added a comment. In D65744#1652355 , @Anastasia wrote: > I don't think this is likely to change. Are you suggesting to move the > deduction logic for pointee of pointers, references and block pointers into > ASTContext helper that creates a poin

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

rjmccall added a comment. Herald added a subscriber: ributzka. Okay, now that I understand the source-compatibility issues a little better, I think this approach is probably okay. If it causes trouble, we can consider special-casing these headers to treat the member as `__unsafe_unretained` im

[PATCH] D66121: Debug Info: Nest Objective-C property function decls inside their container.

rjmccall added a comment. Can you prepare an NFC patch with just the changes relating to adding `ObjCPropertyImplDecl::get{Getter,Setter}MethodDecl`? I don't get why the redeclaration logic is changing. What happens when a normal method is implemented? Is that not linked up as a redeclaration

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:2060 + /// attr::ObjCOwnership implies an ownership qualifier was explicitly + /// specified rather than being added implicitly by the compiler. + bool isObjCOwnershipAttributedType(QualType Ty) const;

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:7460 + // the initializing expression type during the type deduction. + (T->isUndeducedAutoType() && IsPointee) || // OpenCL spec v2.0 s6.9.b: Okay, I understand why you're d

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

rjmccall added a comment. Could you give it a slightly more general name and then use it in the main semantic check in ActOnFields? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65256/new/ https://reviews.llvm.org/D65256 __

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

rjmccall added inline comments. Herald added a subscriber: ychen. Comment at: lib/Sema/SemaDecl.cpp:11142 +bool ignoreForTrivialityComputation(const FieldDecl *FD) { + // Ignore unavailable fields since they don't affect the triviality of the `shouldIgnoreForR

[PATCH] D65256: [Sema][ObjC] Mark C union fields that have non-trivial ObjC ownership qualifications as unavailable if the union is declared in a system header

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, thanks. LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65256/new/ https://reviews.llvm.org/D65256 __

[PATCH] D62731: [RFC] Add support for options -frounding-math, -fp-model=, and -fp-exception-behavior=, : Specify floating point behavior

rjmccall added a subscriber: scanon. rjmccall added a comment. I think this is a step in the right direction, thank you. I'd like @scanon to weigh in on the evolving design here. Comment at: clang/docs/UsersManual.rst:1314 + ``-ffp-model=strict``, or merely establish the ro

[PATCH] D62731: [RFC] Add support for options -frounding-math, -fp-model=, and -fp-exception-behavior=, : Specify floating point behavior

rjmccall added a comment. Hmm, you know, there are enough different FP options that I think we should probably split them all out into their own section in the manual instead of just listing them under "code generation". That will also give us an obvious place to describe the basic model, i.e.

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added a comment. In https://reviews.llvm.org/D28691#823810, @Anastasia wrote: > In https://reviews.llvm.org/D28691#820684, @rjmccall wrote: > > > In https://reviews.llvm.org/D28691#820641, @b-sumner wrote: > > > > > In https://reviews.llvm.org/D28691#820595, @rjmccall wrote: > > > > > >

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:1 +//===--- SyncScope.h - atomic synchronization scopes *- C++ -*-===// +// Capitalization. Comment at: include/clang/Basic/SyncScope.h:20 + +namespace Syn

[PATCH] D35693: [Driver][Darwin] Pass -munwind-table when !UseSjLjExceptions

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. Sounds fine to me. https://reviews.llvm.org/D35693 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-b

[PATCH] D32199: [TySan] A Type Sanitizer (Clang)

rjmccall added a comment. Has this proposal run aground? I'm going back over some old patches that I've been CC'ed on and trying to make sure they're not blocking on my review. https://reviews.llvm.org/D32199 ___ cfe-commits mailing list cfe-commi

[PATCH] D24461: CodeGen: Cast llvm.flt.rounds result to match __builtin_flt_rounds

rjmccall added a comment. Did this ever get committed? https://reviews.llvm.org/D24461 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added a comment. The patch generally looks good, but if you need to handle non-constant scopes, you should submit a new patch to address that. Comment at: lib/CodeGen/CGAtomic.cpp:896 +return V; + auto DestAS = getContext().getTargetAddressSpace(LangAS::o

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGCXXABI.cpp:43 if (RD->hasNonTrivialDestructor()) return false; v.g.vassilev wrote: > rjmccall wrote: > > Does canPassInRegisters() not subsume all of these earlier checks? > No, if I remove them

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

rjmccall added a comment. LGTM. I'm fine with the plan to handle potentially non-constant scopes in a follow-up patch. Comment at: include/clang/Basic/SyncScope.h:21 +/// \brief Defines the synch scope values used by the atomic builtins and +/// expressions +enum class SyncSc

[PATCH] D32199: [TySan] A Type Sanitizer (Clang)

rjmccall added a comment. Looks fine to me. https://reviews.llvm.org/D32199 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28691: Add OpenCL 2.0 atomic builtin functions as Clang builtin

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

[PATCH] D36347: Add new script to launch lldb and set breakpoints for diagnostics all diagnostics seen.

rjmccall edited reviewers, added: jasonmolenda, spyffe; removed: rjmccall. rjmccall added a comment. Since this is fundamentally an LLDB script, I've tagged a couple of LLDB people to review it. Jason, Sean: the idea here is to make it easier for clang developers to debug unexpected diagnostics

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

rjmccall added inline comments. Comment at: include/clang/Basic/AttrDocs.td:138 +Additionally, for block pointers, the same restriction apply to copies of +blocks. For example: + ``` Additionally, when the parameter is a `block pointer

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

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:5742 +// effect of performing a trivial copy of the type. +bool Sema::CanPassInRegisters(CXXRecordDecl *D) { + if (D->isDependentType()) This should probably be called something like "computeCa

[PATCH] D36437: [clang] Get rid of "%T" expansions

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

[PATCH] D36501: add flag to undo ABI change in r310401

rjmccall added a comment. Yeah, I think having an internal C++ ABI version makes a lot more sense than having a million different flags. Is there a reason to expose this as a knob to users at all? Repository: rL LLVM https://reviews.llvm.org/D36501 __

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

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:14024 + + if (OldFT->hasExtParameterInfos()) +for (unsigned I = 0, E = OldFT->getNumParams(); I != E; ++I) ahatanak wrote: > rjmccall wrote: > > I think we generally encourage the uses of b

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

rjmccall added inline comments. Comment at: test/FixIt/fixit-cxx0x.cpp:57 (void)[&, &i, &i]{}; // expected-error 2{{'&' cannot precede a capture when the capture default is '&'}} - (void)[=, this]{ this->g(5); }; // expected-error{{'this' cannot be explicitly captured}}

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

rjmccall added inline comments. Comment at: test/FixIt/fixit-cxx0x.cpp:57 (void)[&, &i, &i]{}; // expected-error 2{{'&' cannot precede a capture when the capture default is '&'}} - (void)[=, this]{ this->g(5); }; // expected-error{{'this' cannot be explicitly captured}}

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

rjmccall added inline comments. Comment at: test/FixIt/fixit-cxx0x.cpp:57 (void)[&, &i, &i]{}; // expected-error 2{{'&' cannot precede a capture when the capture default is '&'}} - (void)[=, this]{ this->g(5); }; // expected-error{{'this' cannot be explicitly captured}}

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

rjmccall added inline comments. Comment at: test/FixIt/fixit-cxx0x.cpp:57 (void)[&, &i, &i]{}; // expected-error 2{{'&' cannot precede a capture when the capture default is '&'}} - (void)[=, this]{ this->g(5); }; // expected-error{{'this' cannot be explicitly captured}}

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:47 + return Scopes; +} + You could just return an ArrayRef to a static const array. Comment at: lib/CodeGen/CGAtomic.cpp:687 + + auto *SC = Builder.CreateIntCast(Scop

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:46 + Scopes.push_back(SyncScope::OpenCLSubGroup); + return Scopes; +} t-tye wrote: > Should there be an assert/static_assert in case SyncScope enum grows due to > other languages? It m

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

rjmccall added inline comments. Comment at: lib/CodeGen/CGAtomic.cpp:696 +if (S != Default) + SI->addCase(Builder.getInt32(static_cast(S)), B); + t-tye wrote: > rjmccall wrote: > > t-tye wrote: > > > Is it documented in the SyncScope enum that the enumer

[PATCH] D36327: [OpenCL] Allow targets emit optimized pipe functions for power of 2 type sizes

rjmccall added a comment. Could you just implement this in SimplifyLibCalls? I assume there's some way to fill in TargetLibraryInfo appropriately for a platform. Is that too late for your linking requirements? https://reviews.llvm.org/D36327 ___

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:59 +return "opencl_subgroup"; + } +} yaxunl wrote: > rjmccall wrote: > > t-tye wrote: > > > Should there be a default/assert/static_assert to allow SyncScope enum to > > > grow due

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

rjmccall added inline comments. Comment at: lib/Sema/SemaLambda.cpp:959 + ? diag::ext_equals_this_lambda_capture_cxx2a + : diag::warn_cxx1z_compat_equals_this_lambda_capture); hamzasood wrote: > faisalv wrote: > >

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:92 +/// \brief Defines the synch scope ABI for OpenCL. +class AtomicScopeOpenCLABI : public AtomicScopeABI { +public: yaxunl wrote: > rjmccall wrote: > > Please correct me if I'm wrong, b

[PATCH] D36572: Implemented P0409R2 - Allow lambda capture [=, this]

rjmccall added inline comments. Comment at: test/FixIt/fixit-cxx0x.cpp:57 (void)[&, &i, &i]{}; // expected-error 2{{'&' cannot precede a capture when the capture default is '&'}} - (void)[=, this]{ this->g(5); }; // expected-error{{'this' cannot be explicitly captured}}

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

rjmccall added inline comments. Comment at: include/clang/Basic/SyncScope.h:92 +/// \brief Defines the synch scope ABI for OpenCL. +class AtomicScopeOpenCLABI : public AtomicScopeABI { +public: yaxunl wrote: > rjmccall wrote: > > yaxunl wrote: > > > rjmccall wrot

[PATCH] D36580: [OpenCL] Support variable memory scope in atomic builtins

rjmccall accepted this revision. rjmccall added a comment. Thanks, that looks great. https://reviews.llvm.org/D36580 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36790: [ObjC] Messages to 'self' in class methods should use class method dispatch to avoid multiple method ambiguities

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. Class methods can be inherited; this entire approach is bogus. Repository: rL LLVM https://reviews.llvm.org/D36790 ___ cfe-commi

[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

rjmccall resigned from this revision. rjmccall added a comment. I don't think I'm best-equipped to review this, sorry. https://reviews.llvm.org/D36527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/list

[PATCH] D36790: [ObjC] Messages to 'self' in class methods should use class method dispatch to avoid multiple method ambiguities

rjmccall added a reviewer: doug.gregor. rjmccall added a comment. Tagging Doug. Repository: rL LLVM https://reviews.llvm.org/D36790 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36790: [ObjC] Messages to 'self' in class methods should use class method dispatch to avoid multiple method ambiguities

rjmccall added a comment. I think it's fair to treat instancetype as an inherited requirement — that is, the rules of covariant override always apply, which essentially means that overriders of instance-returning methods must also return instancetype whether they say so explicitly or not. But

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. The meaning we've agreed on for LangAS::Default is to be the address space of local declarations, which corresponds quite well to __private in OpenCL. I think your concern about diagnostics is better addressed by changing the pretty-printer than by changing Sema to gi

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added a comment. Well, we don't currently have a concept of a non-canonical qualifier, but I think it probably wouldn't be difficult to support; you would just need ASTContext::getQualifiedType to be aware of it. https://reviews.llvm.org/D35082 _

[PATCH] D36876: [IRGen] Evaluate constant static variables referenced through member expressions

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:1323 + if (result.HasSideEffects && !AllowSideEffects) { +assert(!isa(E) && "declrefs should not have side effects"); return ConstantEmission(); The side effects here are those associate

[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11846 // an address space. if (T.getAddressSpace() != 0) { // OpenCL allows function arguments declared to be an array of a type yaxunl wrote: > Anastasia wrote: > > yaxunl wrote: > > >

[PATCH] D36876: [IRGen] Evaluate constant static variables referenced through member expressions

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprComplex.cpp:163 +else + CGF.EmitLValue(ME->getBase()); +return *Constant; There's an EmitIgnoredExpr you could use. Also, I think it would be fine to add a generic tryEmitMemb

[PATCH] D36876: [IRGen] Evaluate constant static variables referenced through member expressions

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprComplex.cpp:163 +else + CGF.EmitLValue(ME->getBase()); +return *Constant; rjmccall wrote: > There's an EmitIgnoredExpr you could use. > > Also, I think it would be fine to add

[PATCH] D36876: [IRGen] Evaluate constant static variables referenced through member expressions

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Looks great, thanks. Repository: rL LLVM https://reviews.llvm.org/D36876 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://li

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

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:3851 + ->getType() + ->getPointerAddressSpace(); const unsigned ArgAddrSpace = yaxunl wrote: > rjmccall wro

[PATCH] D72010: [CodeGen] Use CreateFNeg in buildFMulAdd

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3350 +MulOp0 = Builder.CreateFNeg(MulOp0, "neg"); + if (negAdd) +Addend = Builder.CreateFNeg(Addend, "neg"); craig.topper wrote: > I removed the 'else' here because logically

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

rjmccall added a comment. The solution described in that comment is not acceptable. We are not going to tell users that they cannot assign symbols explicitly to sections because LLVM is unable to promise not to miscompile if they do. It is LLVM's responsibility to correctly compile valid code

[PATCH] D72114: [MS] Overhaul how clang passes overaligned args on x86_32

rjmccall added inline comments. Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:91 bool InAllocaSRet : 1;// isInAlloca() + bool InAllocaIndirect : 1;// isInAlloca() bool IndirectByVal : 1; // isIndirect() Would it be better to handle `inallo

[PATCH] D72114: [MS] Overhaul how clang passes overaligned args on x86_32

rjmccall added inline comments. Comment at: clang/test/CodeGen/x86_32-arguments-win32.c:77 +// CHECK-LABEL: define dso_local void @receive_vec_256(<8 x float> inreg %x, <8 x float> inreg %y, <8 x float> inreg %z, <8 x float>* %0, <8 x float>* %1) +// CHECK-LABEL: define dso_loca

[PATCH] D72114: [MS] Overhaul how clang passes overaligned args on x86_32

rjmccall added inline comments. Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:91 bool InAllocaSRet : 1;// isInAlloca() + bool InAllocaIndirect : 1;// isInAlloca() bool IndirectByVal : 1; // isIndirect() rnk wrote: > rnk wrote: > > rjmccall

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

rjmccall added a comment. In D68101#1804006 , @bd1976llvm wrote: > I looked into these today. I think we can do the first of these. I have put a > WIP patch up here: https://reviews.llvm.org/D72194. Could you comment on the > approach taken? Thank you.

[PATCH] D72271: [Clang] Handle target-specific builtins returning aggregates.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4333 +if (!ReturnValue.isNull() && + ReturnValue.getValue().getType()->getPointerElementType()->isStructTy()) + return RValue::getAggregate(ReturnValue.getValue(), The re

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

rjmccall added a comment. In D68101#1806135 , @nickdesaulniers wrote: > In D68101#1802220 , @bd1976llvm > wrote: > > > Below is the code comment from the new patch explaining the new approach, > > please take a l

[PATCH] D72114: [MS] Overhaul how clang passes overaligned args on x86_32

rjmccall added inline comments. Comment at: clang/test/CodeGen/x86_32-arguments-win32.c:77 +// CHECK-LABEL: define dso_local void @receive_vec_256(<8 x float> inreg %x, <8 x float> inreg %y, <8 x float> inreg %z, <8 x float>* %0, <8 x float>* %1) +// CHECK-LABEL: define dso_loca

[PATCH] D72114: [MS] Overhaul how clang passes overaligned args on x86_32

rjmccall added inline comments. Comment at: clang/test/CodeGen/x86_32-arguments-win32.c:77 +// CHECK-LABEL: define dso_local void @receive_vec_256(<8 x float> inreg %x, <8 x float> inreg %y, <8 x float> inreg %z, <8 x float>* %0, <8 x float>* %1) +// CHECK-LABEL: define dso_loca

[PATCH] D72271: [Clang] Handle target-specific builtins returning aggregates.

rjmccall added a comment. In D72271#1807790 , @simon_tatham wrote: > The current return value of `EmitTargetBuiltinExpr` will be the `llvm::Value > *` corresponding to the last of a sequence of store instructions that writes > the constructed aggregate

[PATCH] D50119: P1144 "Trivially relocatable" (0/3): Compiler support for `__is_trivially_relocatable(T)`

rjmccall added a comment. If the committee is actively looking into this problem and considering multiple alternatives, I don't see a particular need to accept your proposal into Clang in advance of the committee's decision. Arguably that would even be somewhat inappropriate, since it might be

[PATCH] D50119: P1144 "Trivially relocatable" (0/3): Compiler support for `__is_trivially_relocatable(T)`

rjmccall added a comment. If the committee *isn't* taking this up, they absolutely should, though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D50119/new/ https://reviews.llvm.org/D50119 ___ cfe-commi

[PATCH] D50119: P1144 "Trivially relocatable" (0/3): Compiler support for `__is_trivially_relocatable(T)`

rjmccall added a comment. In D50119#1808727 , @Quuxplusone wrote: > In D50119#1808616 , @rjmccall wrote: > > > If the committee is actively looking into this problem and considering > > multiple alternatives, I don

[PATCH] D72375: Disallow an empty string literal in an asm label

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

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

rjmccall added a reviewer: rjmccall. rjmccall added a comment. It's not unusual for new warnings to require changes to other tests. I agree with enabling this by default. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72231/new/ https://reviews.ll

[PATCH] D68578: [HIP] Fix device stub name

rjmccall added a comment. I mean, I made a recommendation and you dismissed it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68578/new/ https://reviews.llvm.org/D68578 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.l

[PATCH] D70172: [CUDA][HIP] Fix assertion due to dtor check on windows

rjmccall added a comment. I thought you were saying that the destructor decl hadn't been created yet, but I see now that you're saying something more subtle. `CurContext` is set to the destructor because the standard says in [class.dtor]p13: At the point of definition of a virtual destructor

[PATCH] D72271: [Clang] Handle target-specific builtins returning aggregates.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4351 +default: + llvm_unreachable("Bad evaluation kind in EmitBuiltinExpr"); +} We generally don't add `default` cases to exhaustive switches. You can put this `llvm_unreac

<    13   14   15   16   17   18   19   20   21   22   >