[PATCH] D79735: FP LangOpts should not be dependent on CGOpts

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

[PATCH] D79730: [NFCi] Switch ordering of ParseLangArgs and ParseCodeGenArgs.

rjmccall added a comment. I don't think the reversion is necessary; it's being fixed to remove the dependency. This is a good change, though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79730/new/ https://reviews.llvm.org/D79730

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

rjmccall added a comment. The parameter variable is formally considered to be in a particular address space, and taking the address of it yields a pointer in that address space. That can only work for an indirect argument if either (1) the address space of the pointer that's actually passed ca

[PATCH] D79730: [NFCi] Switch ordering of ParseLangArgs and ParseCodeGenArgs.

rjmccall added a comment. I think it makes sense; if nothing else, we're trying to upstream all that work. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79730/new/ https://reviews.llvm.org/D79730 ___ c

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

rjmccall added a comment. I think we might have had to change that test on our fork when we changed the parsing order. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72841/new/ https://reviews.llvm.org/D72841

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

rjmccall added a comment. In D79744#2030481 , @arsenm wrote: > In D79744#2030294 , @rjmccall wrote: > > > The parameter variable is formally considered to be in a particular address > > space, and taking the addres

[PATCH] D78134: [Sema] Don't apply an lvalue-to-rvalue conversion to a discarded-value expression if it has an array type

rjmccall accepted this revision. rjmccall added a comment. Alright, let's go with this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78134/new/ https://reviews.llvm.org/D78134 ___ cfe-commits mailing

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

rjmccall added a comment. IIUC, the way within-statement contraction is supposed to work is that there are supposed to be blocking intrinsics inserted at various places. I don't remember the details, though, or if anyone's thought about how it interacts with cross-statement contraction, which

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

rjmccall added a comment. Okay. So the only real ABI here is the layout of the memory that the arguments are actually written into? And that memory needs to be treated as constant? Unfortunately, I think `byval` just doesn't match what you want because of the mutability — the frontend *has* t

[PATCH] D78767: [Sema] Teach -Wcast-align to compute a more accurate alignment when the source expression has array subscript or pointer arithmetic operators

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:13066 +std::tuple +static getBaseAlignmentAndOffsetFromLValue(const Expr *E, ASTContext &Ctx) { + E = E->IgnoreParens(); I think an `Optional` would be a more self-documenting type here,

[PATCH] D79378: PR34581: Don't remove an 'if (p)' guarding a call to 'operator delete(p)' under -Oz.

rjmccall added a comment. In D79378#2028705 , @rsmith wrote: > In D79378#2019336 , @rjmccall wrote: > > > Is it reasonable to figure out ahead of time that we can skip the null > > check completely? It'd be kindof

[PATCH] D78444: Perform ActOnConversionDeclarator after looking for any virtual functions it overrides

rjmccall added a comment. LGTM with a minor fix. Comment at: clang/lib/Sema/SemaDecl.cpp:10745 +if (CXXConversionDecl *Conversion = dyn_cast(NewFD)) + ActOnConversionDeclarator(Conversion); Indentation is off. CHANGES SINCE LAST ACTION https:

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

rjmccall added a comment. No, if that's how we handle that, then you're absolutely right that we shouldn't set the `contract` flag. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72841/new/ https://reviews.llvm.org/D72841 ___

[PATCH] D78767: [Sema] Teach -Wcast-align to compute a more accurate alignment when the source expression has array subscript or pointer arithmetic operators

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:13111 + } + } + return std::make_tuple(false, CharUnits::Zero(), CharUnits::Zero()); ahatanak wrote: > rjmccall wrote: > > Derived-to-base conversions. > Does this include cases where

[PATCH] D78767: [Sema] Teach -Wcast-align to compute a more accurate alignment when the source expression has array subscript or pointer arithmetic operators

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:13122 +if (Base->isVirtual()) { + BaseAlignment = Ctx.getTypeAlignInChars(Base->getType()); + Offset = CharUnits::Zero(); Oh, this — and all the other places that do presume

[PATCH] D79903: FastMathFlags.allowContract should be init from FPFeatures.allowFPContractAcrossStatement

rjmccall added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2943 + if (Opts.FastRelaxedMath) +Opts.setDefaultFPContractMode(LangOptions::FPM_Fast); Opts.HexagonQdsp6Compat = Args.hasArg(OPT_mqdsp6_compat); mibintc wrote: > I chan

[PATCH] D72841: Add support for pragma float_control, to control precision and exception behavior at the source level

rjmccall added inline comments. Comment at: clang/lib/Serialization/ASTReader.cpp:7899 +if (FpPragmaCurrentLocation.isInvalid()) { + assert(*FpPragmaCurrentValue == SemaObj->FpPragmaStack.DefaultValue && + "Expected a default pragma float_control value"); ---

[PATCH] D79903: FastMathFlags.allowContract should be init from FPFeatures.allowFPContractAcrossStatement

rjmccall added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2943 + if (Opts.FastRelaxedMath) +Opts.setDefaultFPContractMode(LangOptions::FPM_Fast); Opts.HexagonQdsp6Compat = Args.hasArg(OPT_mqdsp6_compat); mibintc wrote: > rjmcca

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

rjmccall added a comment. In D79744#2035109 , @arsenm wrote: > In D79744#2030710 , @rjmccall wrote: > > > Okay. So the only real ABI here is the layout of the memory that the > > arguments are actually written int

[PATCH] D79631: #pragma float_control should be permitted at namespace scope

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

[PATCH] D79903: FastMathFlags.allowContract should be init from FPFeatures.allowFPContractAcrossStatement

rjmccall added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:2943 + if (Opts.FastRelaxedMath) +Opts.setDefaultFPContractMode(LangOptions::FPM_Fast); Opts.HexagonQdsp6Compat = Args.hasArg(OPT_mqdsp6_compat); michele.scandale wrote:

[PATCH] D78767: [Sema] Teach -Wcast-align to compute a more accurate alignment when the source expression has array subscript or pointer arithmetic operators

rjmccall added a comment. Other changes look good to me, thanks. Comment at: clang/lib/Sema/SemaChecking.cpp:13122 +if (Base->isVirtual()) { + BaseAlignment = Ctx.getTypeAlignInChars(Base->getType()); + Offset = CharUnits::Zero(); ahatanak wrote:

[PATCH] D78767: [Sema] Teach -Wcast-align to compute a more accurate alignment when the source expression has array subscript or pointer arithmetic operators

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/lib/Sema/SemaChecking.cpp:13122 +if (Base->isVirtual()) { + BaseAlignment = Ctx.getTypeAlignInChars(Base->getType()); + Offset = Cha

[PATCH] D79942: [clang] Add an API to retrieve implicit constructor arguments.

rjmccall added a comment. This looks good to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79942/new/ https://reviews.llvm.org/D79942 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:/

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

rjmccall added a comment. I don't understand why `noalias` is even a concern if the whole buffer passed to the kernel is read-only. `noalias` is primarily about proving non-interference, but if you can tell IR that the buffer is read-only, that's a much more powerful statement. Regardless, if

[PATCH] D77491: [Sema] Introduce BuiltinAttr, per-declaration builtin-ness

rjmccall added a comment. @rsmith This is a big enough architectural change that I'd appreciate your sign-off on the basic approach. Comment at: clang/lib/Sema/SemaDecl.cpp:3759 unsigned BuiltinID; - if (Old->isImplicit() && (BuiltinID = Old->getBuiltinID())) { + bool Rev

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

rjmccall added a comment. In D79744#2040348 , @jdoerfert wrote: > Drive by, I haven't read the entire history. > > In D79744#2040208 , @rjmccall wrote: > > > I don't understand why `noalias` is even a concern if the

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

rjmccall added a comment. In D79744#2040434 , @jdoerfert wrote: > In D79744#2040380 , @rjmccall wrote: > > > In D79744#2040348 , @jdoerfert > > wrote: > > > > > Drive by, I

[PATCH] D80166: [CGCall] Annotate reference parameters with "align" attribute.

rjmccall added a comment. Herald added a subscriber: sstefan1. Hmm. It looks like we did reserve space for this in the great pointer-alignment-assumption detente ("RFC: Enforcing pointer type alignment in Clang" from early 2016, which I'm aware was never really a consensus position, but which

[PATCH] D40176: [CodeGen] Collect information about sizes of accesses and access types for TBAA

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Alright, I guess this all makes sense. We do have some extensions that are playing around with v-table pointers, so it's probably fair to require the v-table pointer type to be passed dow

[PATCH] D40508: Replace long type names in IR with hashes

rjmccall added a comment. In Swift's IRGen, we have an option controlling whether to emit meaningful value names. That option can be set directly from the command line, but it defaults to whether we're emitting IR assembly (i.e. .ll, not .bc), which means that the clients most interested in ge

[PATCH] D40275: [CUDA] Report "unsupported VLA" errors only on device side.

rjmccall accepted this revision. rjmccall added a comment. In https://reviews.llvm.org/D40275#937010, @tra wrote: > In https://reviews.llvm.org/D40275#933253, @tra wrote: > > > @rjmccall : are you OK with this approach? If VLA is not supported by the > > target, CUDA is handled as a special case

[PATCH] D40275: [CUDA] Report "unsupported VLA" errors only on device side.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2188 + !Context.getTargetInfo().isVLASupported() && + shouldDiagnoseTargetSupportFromOpenMP()) { + // Some targets don't support VLAs. tra wrote: > tra wrote:

[PATCH] D40569: Use default IR alignment for cleanup.dest.slot.

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

[PATCH] D40275: [CUDA] Report "unsupported VLA" errors only on device side.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2188 + !Context.getTargetInfo().isVLASupported() && + shouldDiagnoseTargetSupportFromOpenMP()) { + // Some targets don't support VLAs. tra wrote: > rjmccall w

[PATCH] D40508: Replace long type names in IR with hashes

rjmccall added a comment. My skepticism about the raw_ostream is not about the design of having a custom raw_ostream subclass, it's that that subclass could conceivably be re-used by some other client. It feels like it belongs as an internal hack in Clang absent some real evidence that someone

[PATCH] D40508: Replace long type names in IR with hashes

rjmccall added a comment. In https://reviews.llvm.org/D40508#938675, @sepavloff wrote: > In https://reviews.llvm.org/D40508#938040, @rjmccall wrote: > > > My skepticism about the raw_ostream is not about the design of having a > > custom raw_ostream subclass, it's that that subclass could concei

[PATCH] D40508: Replace long type names in IR with hashes

rjmccall added a comment. In https://reviews.llvm.org/D40508#938854, @sepavloff wrote: > In https://reviews.llvm.org/D40508#938686, @rjmccall wrote: > > > In https://reviews.llvm.org/D40508#938675, @sepavloff wrote: > > > > > In https://reviews.llvm.org/D40508#938040, @rjmccall wrote: > > > > > >

[PATCH] D40508: Replace long type names in IR with hashes

rjmccall added a comment. Are you trying to use LLVM struct type identity to infer information about the source program? That is not and has never been a guarantee. https://reviews.llvm.org/D40508 ___ cfe-commits mailing list cfe-commits@lists.llv

[PATCH] D40806: CodeGen: Fix invalid bitcasts for memcpy

rjmccall added inline comments. Comment at: cfe/trunk/lib/CodeGen/CGCall.cpp:1238 + Address Casted = CGF.Builder.CreateBitCast(Tmp, CGF.AllocaInt8PtrTy); + Address SrcCasted = CGF.Builder.CreateBitCast(Src, CGF.AllocaInt8PtrTy); CGF.Builder.CreateMemCpy(Casted, SrcCasted, --

[PATCH] D40929: Unblock Swift Calling Convention Mangling on Windows

rjmccall added reviewers: rnk, majnemer. rjmccall added a comment. Reid, David, do you have a recommendation about the right way to support non-MS-supported CCs here? Repository: rC Clang https://reviews.llvm.org/D40929 ___ cfe-commits mailing l

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

rjmccall added a comment. 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 one of its > subobjects is non-trivial and is not annotated with "tr

[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block

rjmccall added a comment. Alternative #1 is actually the right option — only it shouldn't be an objc_retain, it should be an objc_retainAutoreleasedReturnValue. Repository: rC Clang https://reviews.llvm.org/D41050 ___ cfe-commits mailing list cf

[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block

rjmccall added a comment. In https://reviews.llvm.org/D41050#952720, @danzimm wrote: > Or do we need to abide by those semantics strictly here? Could you expand on > why that is, if that's the case? The autoreleased-return-value optimization cannot be understood purely in terms of its impact

[PATCH] D36915: [Sema] Diagnose local variables and parameters captured by lambda and block expressions in a default argument

rjmccall added a comment. This looks good to me. https://reviews.llvm.org/D36915 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

rjmccall added a comment. Hmm. The OVE-ing of the RHS of a property assignment is just there to make the original source structure clearer. Maybe the right solution here is to set a flag in the OVE that says that it's a unique semantic reference to its source expression, and then change IRGen

[PATCH] D41050: Fix over-release of return value of lambda implicitly converted to block/function pointer

rjmccall added a comment. We do try to avoid relying on optimizer behavior in our tests. I don't know why you need that here. Repository: rC Clang https://reviews.llvm.org/D41050 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://li

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. In D131979#3753358 , @yihanaa wrote: > In D131979#3752208 , @rjmccall > wrote: > >> From the test case, it looks like the builtin just ignores pointers to >> volatile types, which shoul

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. In D131979#3756067 , @yihanaa wrote: > In D131979#3756006 , @rjmccall > wrote: > >> In D131979#3753358 , @yihanaa >> wrote: >> >>> In D131979#3

[PATCH] D132352: Introduce noread_thread_id to address the thread identification problem in coroutines

rjmccall added a comment. Stackful coroutine bodies should be straightforward to support on top of the other work you've been doing, if anyone's actually interested in pursuing them. As far as the optimizer needs to know, a stackful coroutine function is just like a presplit stackless coroutin

[PATCH] D132352: Introduce noread_thread_id to address the thread identification problem in coroutines

rjmccall added a comment. In D132352#3757433 , @ChuanqiXu wrote: > In D132352#3757415 , @rjmccall > wrote: > >> Stackful coroutine bodies should be straightforward to support on top of the >> other work you've b

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. Thanks. Just a few small requests to re-use more of the existing logic here. Comment at: clang/lib/Sema/SemaChecking.cpp:7659 + << 0 << 2 << TheCall->getNumArgs() << Callee->getSourceRange(); + } + It looks like this should

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:7671 + AllArgs, CallType)) +return true; + yihanaa wrote: > rjmccall wrote: > > You can just pull the argument expressions out of the `CallExpr`; you don't

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:7671 + AllArgs, CallType)) +return true; + yihanaa wrote: > rjmccall wrote: > > yihanaa wrote: > > > rjmccall wrote: > > > > You can just pull the argument

[PATCH] D131701: [CodeGen][ObjC] Call synthesized copy constructor/assignment operator functions in getter/setter functions of non-trivial C struct properties

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:1429 + GetAddrOfLocalVar(*setterMethod->param_begin()), ivar->getType()); + callCStructCopyAssignmentOperator(Dst, Src); +} else { When the method takes the structure by va

[PATCH] D133202: [Clang][CodeGen] Avoid __builtin_assume_aligned crash when the 1st arg is array type

rjmccall added a comment. Other than the one thing, this looks good. Comment at: clang/lib/Sema/SemaChecking.cpp:7697 // The alignment must be a constant integer. - Expr *Arg = TheCall->getArg(1); + Expr *SecondArg = TheCall->getArg(1); This should be:

[PATCH] D131701: [CodeGen][ObjC] Call synthesized copy constructor/assignment operator functions in getter/setter functions of non-trivial C struct properties

rjmccall added a comment. Can we check for the right conditions when emitting the setter body and just deactivate the cleanup? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131701/new/ https://reviews.llvm.org/D131701

[PATCH] D133202: [Clang][CodeGen] Avoid __builtin_assume_aligned crash when the 1st arg is array type

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

[PATCH] D125419: [Arm64EC 7/?] clang side of Arm64EC varargs ABI.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:2457 +/*IsVectorCall=*/false, /*IsRegCall=*/false); + } + Hmm. Doesn't EC ABI lowering need to preserve this same state, or else you'll get incompatibilities when

[PATCH] D125419: [Arm64EC 7/?] clang side of Arm64EC varargs ABI.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:2457 +/*IsVectorCall=*/false, /*IsRegCall=*/false); + } + efriedma wrote: > rjmccall wrote: > > Hmm. Doesn't EC ABI lowering need to preserve this same state, or el

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:7651-7652 + Expr *FirstArg = TheCall->getArg(0); + if (auto *CE = dyn_cast(FirstArg)) +FirstArg = CE->getSubExprAsWritten(); rsmith wrote: > This looks very suspicious to me: this

[PATCH] D133202: [Clang] Avoid __builtin_assume_aligned crash when the 1st arg is array type

rjmccall added a comment. Please look into the history of that test case to see why we're specifically testing for the ability to redeclare this builtin. We could certainly allow this builtin to be redeclared using the `const void*` type — that doesn't really reflect the generic nature of the b

[PATCH] D128571: [X86] Support `_Float16` on SSE2 and up

rjmccall accepted this revision. rjmccall added a comment. Thank you, LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128571/new/ https://reviews.llvm.org/D128571 ___ cfe-commits mailing list cfe-com

[PATCH] D123952: [FPEnv] Allow CompoundStmt to keep FP options

rjmccall added inline comments. Comment at: clang/lib/Basic/LangOptions.cpp:214 +OverrideMask |= NAME##Mask; +#include "clang/Basic/FPOptions.def" + return FPOptionsOverride(*this, OverrideMask); Hmm. If we can assume that all of the options are single bits

[PATCH] D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible

rjmccall added a comment. Is there no way in LLVM to load partially-initialized memory and then freeze it that will preserve the parts of the value that are initialized? Because that seems like something that LLVM needs to be providing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[PATCH] D123952: [FPEnv] Allow CompoundStmt to keep FP options

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:747 + /// Return difference with the given option set. + FPOptionsOverride diffWith(const FPOptions &Base); + Can you make direction more obvious in the method name? `diffFrom`

[PATCH] D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible

rjmccall added a comment. In D128501#3613890 , @efriedma wrote: >> Is there no way in LLVM to load partially-initialized memory and then freeze >> it that will preserve the parts of the value that are initialized? Because >> that seems like something th

[PATCH] D123952: [FPEnv] Allow CompoundStmt to keep FP options

rjmccall added a comment. Looks good, thanks! Approved with the rename as discussed. Comment at: clang/include/clang/Basic/LangOptions.h:747 + /// Return difference with the given option set. + FPOptionsOverride diffWith(const FPOptions &Base); + sepavloff w

[PATCH] D123952: [FPEnv] Allow CompoundStmt to keep FP options

rjmccall added inline comments. Comment at: clang/lib/AST/Stmt.cpp:370-371 setStmts(Stmts); + if (hasStoredFPFeatures()) +setStoredFPFeatures(FPFeatures); } aaron.ballman wrote: > rjmccall wrote: > > aaron.ballman wrote: > > > There's a part of me that w

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

rjmccall added a comment. In D113107#3620014 , @zahiraam wrote: > In D113107#3615372 , @zahiraam > wrote: > >> In D113107#3606106 , @rjmccall >> wrote: >> >>> In D113107

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:964 TestAndClearIgnoreImag(); BinOpInfo Ops; rjmccall wrote: > Please make a helper function to emit an operand as a possibly-promoted > complex value. As requested, please i

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

rjmccall added a comment. I think you should also at least support promoted arithmetic through the `_Real` and `_Imag` scalar operators before calling this complete. That should be simple — just a matter of calling EmitPromotedComplexExpr from the scalar path. Comment at: c

[PATCH] D122460: [clang] fixed bug #54406

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExprMember.cpp:690 +// int n = a.B::m; +if (BaseExpr && isa(DC) && isa(RDecl)) { + CXXRecordDecl *SRecord = cast(DC)->getCanonicalDecl(); We don't generally cite bug numbers in the source

[PATCH] D122573: [TBAA] Emit distinct TBAA tags for pointers with different depths,types.

rjmccall added a comment. Hmm. We know that the big picture here, distinguishing pointers by pointee type, is going to be disruptive and will probably need a specific enabling/disabling option. I'm not sure that distinguishing only by pointer depth is a minor enough step that it shouldn't be

[PATCH] D122573: [TBAA] Emit distinct TBAA tags for pointers with different depths,types.

rjmccall added a comment. In D122573#3414357 , @fhahn wrote: > In D122573#3412109 , @rjmccall > wrote: > >> Hmm. We know that the big picture here, distinguishing pointers by pointee >> type, is going to be dis

[PATCH] D122822: [Clang][CodeGen]Add constant array support for __builtin_dump_sturct

rjmccall added a comment. In D122822#3420162 , @rsmith wrote: > I'm concerned about the direction of this patch given @rjmccall's comments on > https://reviews.llvm.org/D112626 -- presumably the way we'd want to address > those comments would be to conv

[PATCH] D122820: [GH54588]Fix ItaniumMangler for NTTP unnamed unions w/ unnamed structs

rjmccall added a comment. Unfortunately, this initialization case does allow a situation where Comment at: clang/lib/AST/ItaniumMangle.cpp:5561 + // field represents an anonymous record type. + const CXXRecordDecl *RD = FD->getType()->getAsCXXRecordDecl(); +

[PATCH] D122820: [GH54588]Fix ItaniumMangler for NTTP unnamed unions w/ unnamed structs

rjmccall added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:5576 + "program to refer to the anonymous union, and there is therefore no need " + "to mangle its name. '"); +} erichkeane wrote: > rjmccall wrote: > > Unfortunately, I think

[PATCH] D122820: [GH54588]Fix ItaniumMangler for NTTP unnamed unions w/ unnamed structs

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/AST/ItaniumMangle.cpp:5576 + "program to refer to the anonymous union, and there is therefore no need " + "to mangle its name. '"); +

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

rjmccall added a comment. In D116203#3423594 , @cjdb wrote: > In D116203#3277515 , @zoecarver > wrote: > >> This patch looks awesome, Chris. >> >> Does it make sense to have builtins for `add_const`, etc.? Isn't

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

rjmccall added a comment. In D116203#3430332 , @aaron.ballman wrote: > In D116203#3425512 , @cjdb wrote: > >> I've noticed that libstdc++ has `using __remove_cv = typename >> remove_cv::type`, which causes Clang

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

rjmccall added a comment. It looks like you haven't implemented the target-specific logic for this yet. I cannot let you commit until you do that, because you will be breaking the ABI on Apple platforms. Comment at: clang/include/clang/Basic/CodeGenOptions.h:150 +Assumed

[PATCH] D130964: [X86][BF16] Enable __bf16 for x86 targets.

rjmccall added a comment. How are you actually implementing `__bf16` on these targets? There isn't even hardware support for conversions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130964/new/ https://reviews.llvm.org/D130964

[PATCH] D131012: No longer place const volatile global variables in a read only section

rjmccall added a comment. I think this is fine for the ABI; the section is generally a definition-specific property and doesn't affect use sites. Do we also need to check for volatile fields of records? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

[PATCH] D130964: [X86][BF16] Enable __bf16 for x86 targets.

rjmccall added a comment. In D130964#3695408 , @FreddyYe wrote: > In D130964#3694540 , @bkramer wrote: > >> In D130964#3694473 , @rjmccall >> wrote: >> >>> How are you ac

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

rjmccall added a comment. That is an interesting idea. I like that it integrates this into `-fclang-abi-compat`. The way that `-mno-conservative-small-integer-abi` ends up meaning opposite things based on the abi-compat setting worries me a lot, though. Repository: rG LLVM Github Monorepo

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

rjmccall added a comment. I know what you're saying, but I don't think it matches any model of how programmers use command line flags. You're imagining that a programmer sits down and considers all of their flags deeply and holistically before touching any of them, and that's just not how thes

[PATCH] D131012: No longer place const volatile global variables in a read only section

rjmccall added a comment. On the one hand, we can certainly just have different behavior on BPF targets if this is a common expectation there. On the other hand, if there isn't a requirement to put `const volatile` objects in writable memory, then I'd rather not. If someone has specific expect

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

rjmccall added a comment. In D124435#3701163 , @jyknight wrote: > In D124435#3697259 , @rjmccall > wrote: > >> I know what you're saying, but I don't think it matches any model of how >> programmers use command

[PATCH] D131143: [Clang] Interaction of FP pragmas and function template instantiation

rjmccall added a comment. I completely agree with Eli. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131143/new/ https://reviews.llvm.org/D131143 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:290 + } + return PromotedTy; + } Indentation is off Comment at: clang/lib/CodeGen/CGExprComplex.cpp:305 +result.second = Builder.CreateFPTrunc(resul

[PATCH] D105516: [clang][PassManager] Add -falways-mem2reg to run mem2reg at -O0

rjmccall added a comment. I agree with Eli: we should decide what the goals are here and then use those goals to decide if we can identify a desirable permanent feature and, if so, what the appropriate name for that feature is. It sounds like your goal is to get readable assembly that still cor

[PATCH] D106005: [Docs] Define matrix initialisation in MatrixTypes documentation

rjmccall added inline comments. Comment at: clang/docs/MatrixTypes.rst:279 +The number of constituent arrays must equal the number rows in the matrix type M and the number of elements +in each constituent array must equal the number of columns in the matrix type. + -

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

rjmccall added a comment. In D116203#3434515 , @cjdb wrote: > In D116203#3431612 , @rjmccall > wrote: > >> In D116203#3430332 , >> @aaron.ballman wrote: >> >>> In D11620

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

rjmccall added a comment. I'm totally fine with a specifier that removes all qualifiers; I just think we can't use it for `std::remove_cv`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116203/new/ https://reviews.llvm.org/D116203 ___

[PATCH] D123642: [clang codegen] Assume arguments of __atomic_* are aligned.

rjmccall added a comment. The GCC documentation really ought to specify what alignment assumptions these functions make, if any. But they control the specification here, so if they're making alignment assumptions, then I agre

[PATCH] D123898: Fix crash in ObjC codegen introduced with 5ab6ee75994d645725264e757d67bbb1c96fb2b6

rjmccall added a comment. LGTM. Could you add a test case? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123898/new/ https://reviews.llvm.org/D123898 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D123898: Fix crash in ObjC codegen introduced with 5ab6ee75994d645725264e757d67bbb1c96fb2b6

rjmccall added a comment. In D123898#3455933 , @theraven wrote: > I'd like to. The test case from the original bug report was very large but > I'm not sure how to trigger this with anything comprehensibly small. You > need something where the callee i

[PATCH] D123950: Look through calls to std::addressof to compute pointer alignment.

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

[PATCH] D118804: Revert "[Clang] Propagate guaranteed alignment for malloc and others"

rjmccall added a comment. I'm not sure your new wording is any clearer; a In D118804#3304337 , @urnathan wrote: > In D118804#3304280 , @aaron.ballman > wrote: > >> In D118804#3304261

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