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

rjmccall added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:3387 Bits = (Bits << 1) | unsigned(II->isPoisoned()); -Bits = (Bits << 1) | unsigned(II->hasRevertedBuiltin()); +Bits <<= 1; // Previously used to indicate reverted builtin. Bits =

[PATCH] D86854: [CodeGen] Make sure the EH cleanup for block captures is conditional when the block literal is in a conditional context

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM modulo the minor fix Comment at: clang/lib/CodeGen/CGDecl.cpp:2107 +// FIXME: When popping normal cleanups, we need to keep this EH cleanup +// around in cas

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

rjmccall added a comment. I'd still like @rsmith to sign off here as code owner. Comment at: clang/include/clang/Basic/IdentifierTable.h:231 return ObjCOrBuiltinID == tok::NUM_OBJC_KEYWORDS; } Do we need to support reverting builtins anymore? ==

[PATCH] D86668: Fix Calling Convention of __float128 and long double(128bits) in i386

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1603 + if (IsLinuxABI && Ty->isFloatingType() && getContext().getTypeSize(Ty) == 128) +return 16; + I don't think this should be restricted to just Linux. It's a fix for all OSes

[PATCH] D86218: Teach the swift calling convention about _Atomic types

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

[PATCH] D86218: Teach the swift calling convention about _Atomic types

rjmccall added a comment. Please do this as a very late check rather than the first check. You need to check for extra atomic padding. If there's any difference between the sizes of the atomic type and its element, just add it as opaque data. Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D86049: RFC: Implement optional exportable wrapper function generation for objc_direct methods.

rjmccall added a comment. I think that, if we want to do this, we need to think carefully about what exactly we want the ABI to be. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86049/new/ https://reviews.llvm.org/D86049 _

[PATCH] D85917: [MSP430] Fix passing C structs and unions as function arguments

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/CodeGen/TargetInfo.cpp:7523 + return ABIArgInfo::getIndirectAliased( + getContext().getTypeAlignInChars(Ty), /*AddrSpace=*/0); +

[PATCH] D86020: [MemCpyOptimizer] Optimize passing byref function arguments down the stack

rjmccall added a comment. Sorry, I tried to say thiis more succinctly before, but what exactly is the semantic property of `byref` that allows this optimization? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86020/new/ https://reviews.llvm.org/D86

[PATCH] D86020: [MemCpyOptimizer] Optimize passing byref function arguments down the stack

rjmccall added a comment. If this optimization is valid, it's valid regardless of byref. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86020/new/ https://reviews.llvm.org/D86020 ___ cfe-commits mailing l

[PATCH] D85710: Code generation support for lvalue bit-field conditionals.

rjmccall added a comment. I always imagined that we'd implement this by just inverting the emission: emit the RHS of the assignment and then pass a callback down to a function that descended into conditional operators and called the callback when it hit a leaf. Having an LValue case that's an

[PATCH] D83325: [Sema] Iteratively strip sugar when removing address spaces.

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

[PATCH] D85312: [ADT] Move FixedPoint.h from Clang to LLVM.

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

[PATCH] D84992: [clang codegen] Use IR "align" attribute for static array arguments.

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

[PATCH] D84992: [clang codegen] Use IR "align" attribute for static array arguments.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2503 + llvm::Align Alignment = + CGM.getNaturalTypeAlignment(ETy).getAsAlign(); + AI->addAttrs(llvm::AttrBuilder().addAlignmentAttr(Alignment)); Is

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. In D75574#2202136 , @theraven wrote: > This feature looks generally useful. A few small suggestions: > > - This is really a way of transforming a formal protocol into an informal > protocol. Objective-C has had a convention of

[PATCH] D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary

rjmccall added a comment. In D82317#2200964 , @guiand wrote: > In D82317#2200789 , @rjmccall wrote: > >> Are you seriously adding an attribute to literally every argument and return >> value? Why is this the right

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. In D79279#2200916 , @rsmith wrote: > In D79279#2197176 , @rjmccall wrote: > >> I thought part of the point of `__builtin_memcpy` was so that C library >> headers could do `#define memcpy(x

[PATCH] D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary

rjmccall added a comment. Are you seriously adding an attribute to literally every argument and return value? Why is this the right representation? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82317/new/ https://reviews.llvm.org/D82317

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

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

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a reviewer: theraven. rjmccall added a comment. One thing that's come up so far: you generally need to be looking through non-runtime protocols, not ignoring them. This matters when non-runtime protocols inherit from ordinary protocols. It may be useful to provide a generic fun

[PATCH] D85113: [ABI][NFC] Fix the confusion of ByVal and ByRef argument names

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

[PATCH] D83325: [Sema] Be more thorough when unpacking the AS-qualified pointee for a pointer conversion.

rjmccall added a comment. There's no way to do that, no. Stripping sugar down to the point where you don't have that qualifier anymore is the best we can do. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83325/new/ https://reviews.llvm.org/D83325

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. I thought part of the point of `__builtin_memcpy` was so that C library headers could do `#define memcpy(x, y, z) __builtin_memcpy(x, y, z)`. If so, the conformance issue touches `__builtin_memcpy` as well, not just calls to the library builtin. If that's not true, o

[PATCH] D85319: [analyzer] Get info from the LLVM IR for precision

rjmccall added a comment. Thanks. It'd be a good idea to mention that this is contingent on that discussion in the patch summary. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85319/new/ https://reviews.llvm.org/D85319 __

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:8816 + // FIXME: Should use byref when promoting pointers in structs, but this + // requires adding implementing the coercion. + if (!getContext().getLangOpts().OpenCL && LTy == OrigLTy && ---

[PATCH] D85319: [analyzer] Get info from the LLVM IR for precision

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. This seems a huge architectural change that we need to talk about. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85319/new/ https:

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2446 +order in which they occur (and in which they are observable) can only be +guaranteed using appropriate fences around the function call. Element size must +therefore be a lock-free size for the tar

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1997 case ABIArgInfo::Ignore: + case ABIArgInfo::IndirectAliased: return false; arsenm wrote: > arsenm wrote: > > rjmccall wrote: > > > arsenm wrote: > > > > rjmccall wrote: > >

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. Patch looks basically okay to me, although I'll second Richard's concern that we shouldn't absent-mindedly start producing overloaded `memcpy`s for ordinary `__builtin_memcpy`. Comment at: clang/docs/LanguageExtensions.rst:2446 +order in which they o

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. Sorry, this slipped out of my mind. I've started the process internally. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 ___ cfe-commit

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

rjmccall added a comment. Would you like to pick it back up? We laid out an implementation path: we need to track the fact that a delete was of an incomplete class type in the AST and then unconditionally treat such operations as trivial to destroy in IRGen. Repository: rC Clang CHANGES SI

[PATCH] D84540: [CodeGen][ObjC] Mark calls to objc_unsafeClaimAutoreleasedReturnValue as notail on x86-64

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

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1997 case ABIArgInfo::Ignore: + case ABIArgInfo::IndirectAliased: return false; arsenm wrote: > rjmccall wrote: > > In principle, this can be `inreg` just as much as Indirect ca

[PATCH] D83997: [os_log] Improve the way we extend the lifetime of objects passed to __builtin_os_log_format

rjmccall added a comment. In D83997#2169745 , @ahatanak wrote: > The use case for this is a macro in which the call to > `__builtin_os_log_format` that writes to the buffer and the call that uses > the buffer appear in two different statements. For examp

[PATCH] D84602: [MSP430] Expose msp430_builtin calling convention to C code

rjmccall added a comment. Is there only one special calling convention, or is there any chance that different builtin functions would use different conventions? I don't have a problem with introducing a new convention even if it's only used for "builtin" functions. Repository: rG LLVM Githu

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

rjmccall added inline comments. Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:52 +/// IndirectAliased - Similar to Indirect, but the pointer may not be +/// writable. +IndirectAliased, Hmm. I guess there's actually two different potential

[PATCH] D82999: [CodeGen] Check the cleanup flag before destructing lifetime-extended temporaries created in conditional expressions

rjmccall added a comment. I agree, that can be done separately. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82999/new/ https://reviews.llvm.org/D82999 ___ cfe-commits mailing list cfe-commits@lists.l

[PATCH] D83325: [Sema] Be more thorough when unpacking the AS-qualified pointee for a pointer conversion.

rjmccall added a comment. `removeAddrSpaceQualType` should guarantee that it removes the address space qualifier; you shouldn't need to do something special here. That means it needs to iteratively desugar and collect qualifiers as long as the type is still address-space-qualified. Repositor

[PATCH] D84343: [AST] Keep FP options in trailing storage of CallExpr

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

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. In D79279#2170187 , @jfb wrote: > In D79279#2170157 , @rjmccall wrote: > > > I think the argument is treated as if it were 1 if not given. That's all > > that ordinary memcpy formally gua

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. I think the argument is treated as if it were 1 if not given. That's all that ordinary memcpy formally guarantees, which seems to work fine (semantically, if not performance-wise) for pretty much everything today. I don't think you need any restrictions on element si

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. I don't think any of these should allow _Atomic unless we're going to give it some sort of consistent atomic semantics (which is hard to imagine being useful), and I think you should just take an extra argument of the minimum access width on all of them uniformly if yo

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. In D79279#2169522 , @jfb wrote: > In D79279#2168649 , @rjmccall wrote: > > > In D79279#2168533 , @jfb wrote: > > > > > In D79279#2168479

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. In D79279#2168533 , @jfb wrote: > In D79279#2168479 , @rjmccall wrote: > > > Is there a need for an atomic memcpy at all? Why is it useful to allow > > this operation to take on "atomic"

[PATCH] D83997: [os_log] Improve the way we extend the lifetime of objects passed to __builtin_os_log_format

rjmccall added a comment. Why is the lifetime extended to the enclosing block scope anyway? I understand why we need a clang.arc.use — the optimizer can't reasonably understand that the object has to live within the buffer — but isn't the buffer only used for the duration of the call? Why is

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. Is there a need for an atomic memcpy at all? Why is it useful to allow this operation to take on "atomic" semantics — which aren't actually atomic because the loads and stores to elements are torn — with hardcoded memory ordering and somewhat arbitrary rules about wha

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added a comment. You need to add user docs for these builtins. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8917 + +def err_atomic_type_must_be_lock_free : Error<"_Atomic type must always be lock-free, %0 isn't">; + I don't know why yo

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

rjmccall added a comment. Arguably we should add this attribute to all indirect arguments. I can understand not wanting to update all the test cases, but you could probably avoid adding a new IndirectByRef kind of ABIArgInfo by treating kernels specially in ConstructAttributeList. Or, sorry,

[PATCH] D83812: [clang][RelativeVTablesABI] Do not emit stubs for architectures that support a PLT relocation

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

[PATCH] D84147: Use typedef to represent storage type in FPOption and FPOptionsOverride

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

[PATCH] D83812: [clang][RelativeVTablesABI] Do not emit stubs for architectures that support a PLT relocation

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:633 + auto Arch = targetTriple.getArch(); + if (Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::x86_64) +return false; Could you add a method to TargetCodeGenInfo for this in

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

rjmccall added a comment. In D82663#2153176 , @ebevhan wrote: > In D82663#2144551 , @rjmccall wrote: > > > In D82663#2144219 , @ebevhan wrote: > > > > > In D82663#2142426

[PATCH] D82513: [CodeGen] Store the return value of the target function call to the thunk's return value slot directly when the return type is an aggregate instead of doing so via a temporary

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

[PATCH] D83502: Change behavior with zero-sized static array extents

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Comment at: clang/lib/CodeGen/CGCall.cpp:2515 +} else { + AI->addAttr(llvm::Attribute::NonNull); +} ---

[PATCH] D83502: Change behavior with zero-sized static array extents

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2515 +} else { + AI->addAttr(llvm::Attribute::NonNull); +} aaron.ballman wrote: > rjmccall wrote: > > Isn't the old logic still correct? If

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

rjmccall added a comment. In D82663#2144219 , @ebevhan wrote: > In D82663#2142426 , @rjmccall wrote: > > > Would it be sensible to use a technical design more like what the matrix > > folks are doing, where LLVM pr

[PATCH] D82513: [CodeGen] Store the return value of the target function call to the thunk's return value slot directly when the return type is an aggregate instead of doing so via a temporary

rjmccall added a comment. I agree that avoiding the copy is best. However, at the very least, if that function isn't going to handle the aggregate case correctly, it should assert that it isn't in it. Otherwise this LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D83502: Change behavior with zero-sized static array extents

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2515 +} else { + AI->addAttr(llvm::Attribute::NonNull); +} Isn't the old logic still correct? If the element size is static and the element

[PATCH] D80858: [CUDA][HIP] Support accessing static device variable in host code

rjmccall added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:10068 +isa(D) && cast(D)->isFileVarDecl() && +cast(D)->getStorageClass() == SC_Static) { + return GVA_StrongExternal; JonChesterfield wrote: > yaxunl wrote: > > rjmccall

[PATCH] D82513: [CodeGen] Store the return value of the target function call to the thunk's return value slot directly when the return type is an aggregate instead of doing so via a temporary

rjmccall added a comment. This seems fine. I do wonder if the "real" bug is that this ought to be handled properly in EmitReturnFromThunk, but regardless, the fix seems acceptable. Is there a similar bug with trivial_abi? Should we have a test case for that? Repository: rG LLVM Github Mon

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

rjmccall added a comment. Either way, I think. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79730/new/ https://reviews.llvm.org/D79730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://l

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

rjmccall added a comment. Would it be sensible to use a technical design more like what the matrix folks are doing, where LLVM provides a small interface for emitting operations with various semantics? FixedPointSemantics would move to that header, and Clang would just call into it. That way

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

rjmccall added a comment. I agree with Eli that this should be considered a bugfix in the implementation of a recent language change and should just be rolled out consistently for all targets. If this were a five-year-old feature, the ABI considerations would be different, but it's not. CHAN

[PATCH] D83317: [Sema] Teach -Wcast-align to compute alignment of CXXThisExpr

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

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1605 + })); + }; + I am not a fan of this lambda style, not because I dislike lambdas, but because you've pulled a ton of code that's supporting one or two cases (that could

[PATCH] D82999: [CodeGen] Check the cleanup flag before destructing lifetime-extended temporaries created in conditional expressions

rjmccall added a comment. In D82999#2129417 , @ahatanak wrote: > In test case `test13` in clang/test/CodeGenCXX/exceptions.cpp, I think you > can turn `invoke void @_ZN6test131AC1Ev` into `call void @_ZN6test131AC1Ev`, > no? If the false expression throw

[PATCH] D82999: [CodeGen] Check the cleanup flag before destructing lifetime-extended temporaries created in conditional expressions

rjmccall added a comment. In D82999#2129031 , @ahatanak wrote: > After-full-expression cleanup looks fine to me. `pushCleanupAfterFullExpr` > sets the flags and saves the values when it's in a conditional branch. > > I think ideally we shouldn't have to c

[PATCH] D82999: [CodeGen] Check the cleanup flag before destructing temporaries created in conditional expressions

rjmccall added a comment. Please adjust the commit message to be clear that this is about lifetime-extended temporaries; it's not like we got this wrong for all temporaries. Do we need to do anything to ensure that the after-full-expression cleanup is configured conditionally? Repository:

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

rjmccall added a comment. Can the missing bit just be added? It seems to me that frontends ought to be able to emit the obvious intrinsic for the semantic operation here rather than having to second-guess the backend. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D82781: [OpenCL] Fix missing address space deduction in template variables

rjmccall added a comment. Seems like you shouldn't do it earlier if the type is dependent. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82781/new/ https://reviews.llvm.org/D82781 ___ cfe-commits maili

[PATCH] D82392: [CodeGen] Add public function to emit C++ destructor call.

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

[PATCH] D82392: [CodeGen] Add public function to emit C++ destructor call.

rjmccall added a comment. Can we do a design more like what we did with constructors? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82392/new/ https://reviews.llvm.org/D82392 ___ cfe-commits mailing li

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

rjmccall added a comment. Why not legalize to the signed operation? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82663/new/ https://reviews.llvm.org/D82663 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D82473: [Matrix] Use 1st/2nd instead of first/second in matrix diags.

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

[PATCH] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

rjmccall added a comment. In D81869#2112440 , @mibintc wrote: > I decided that I shouldn't make float options that define a macro, like > -ffast-math, as BENIGN_LANGOPT, I made ffp-contract= , fp-exception-behavior > and rounding-mode BENIGN That seems

[PATCH] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

rjmccall added a comment. Just a bunch of minor suggestions. LGTM if you get all the tests worked out and it actually works the way you want on the tests. Comment at: clang/include/clang/AST/Expr.h:2280 } + FPOptionsOverride getFPFeatures(const LangOptions &LO) const { +

[PATCH] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

rjmccall added inline comments. Comment at: clang/include/clang/AST/Stmt.h:620 +//unsigned FPFeatures : 21; +unsigned FPFeatures : 32; }; riccibruno wrote: > mibintc wrote: > > This is a temporary change, I know you don't want me to change the assert

[PATCH] D81311: [RFC] LangRef: Define byref parameter attribute

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

[PATCH] D72782: [Matrix] Add __builtin_matrix_column_store to Clang.

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:10792 def err_builtin_matrix_pointer_arg: Error< - "%select{first|second}0 argument must be a pointer

[PATCH] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:2280 } + FPOptionsOverride getFPFeatures(const LangOptions &LO) const { +if (UnaryOperatorBits.HasFPFeatures) I would call this one getFPOptionOverrides or getOverriddenFPOptions.

[PATCH] D81795: [clang] Enable -mms-bitfields by default for mingw targets

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

[PATCH] D81857: Fix ConstantAggregateBuilderBase::getRelativeOffset

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

[PATCH] D81795: [clang] Enable -mms-bitfields by default for mingw targets

rjmccall added a comment. Seems reasonable; GCC is the "system compiler" for this platform. Does `isWindowsGNUEnvironment` exactly track the condition that GCC uses? It's just MinGW, not Cygwin? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D817

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall accepted this revision. rjmccall added a comment. LGTM, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81420/new/ https://reviews.llvm.org/D81420 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:327 return true; TheCall->setArg(2, Arg.get()); } jtmott-intel wrote: > rjmccall wrote: > > I know this is existing code, but this is a broken mess. Please change > > thi

[PATCH] D80462: Fix floating point math function attributes definition.

rjmccall closed this revision. rjmccall added a comment. To ssh://github.com/llvm/llvm-project a98d618f6e5f..7fac1acc6171 master -> master Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80462/new/ https://reviews.llvm.org/D80462 _

[PATCH] D81311: [RFC] LangRef: Define inmem parameter attribute

rjmccall added a comment. In D81311#2088568 , @jdoerfert wrote: > In D81311#2088075 , @rjmccall wrote: > > > In D81311#2087592 , @jdoerfert > > wrote: > > > > > In D81311#20

[PATCH] D81311: [RFC] LangRef: Define inmem parameter attribute

rjmccall added a comment. In D81311#2087592 , @jdoerfert wrote: > In D81311#2086326 , @rjmccall wrote: > > > In D81311#2086227 , @jdoerfert > > wrote: > > > > > Do we allow

[PATCH] D81624: [CodeGen] Simplify the way lifetime of block captures is extended

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

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added inline comments. Comment at: clang/test/Sema/builtins-overflow.c:39 +_ExtInt(129) result; +_Bool status = __builtin_mul_overflow(x, y, &result); // expected-error {{signed _ExtInt of bit sizes greater than 128 not supported}} + } jtmott-i

[PATCH] D81311: [RFC] LangRef: Define inmem parameter attribute

rjmccall added a comment. In D81311#2086227 , @jdoerfert wrote: > Do we allow `inmem` to be used for other purposes? I would assume the answer > is yes, as we do not forbid it. I don't know what else we might use it for off-hand, but yes, I think the f

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:327 return true; TheCall->setArg(2, Arg.get()); } I know this is existing code, but this is a broken mess. Please change this to: ``` ExprResult Arg = DefaultFunctionAr

[PATCH] D77592: [clang] Frontend components for the relative vtables ABI

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

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7939 "to a non-const integer (%0 invalid)">; +def err_overflow_builtin_extint_size : Error< + "_ExtInt argument larger than 64-bits to overflow builtin requires runtime "

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/include/clang/Sema/Sema.h:12126 + ExprResult SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall, +

[PATCH] D81311: [RFC] LangRef: Define inmem parameter attribute

rjmccall added a comment. In D81311#2083295 , @arsenm wrote: > In D81311#2078235 , @rjmccall wrote: > > > I wonder if `byref` would be a better name for this, as a way to say that > > the object is semantically a d

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added a comment. Linking compiler-rt is something that should be automatically happening. It's possible that compiler-rt will have different maximum widths on different targets, though. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7939 "to a non-c

[PATCH] D80462: Fix floating point math function attributes definition.

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

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

rjmccall added inline comments. Comment at: clang/include/clang/Sema/Sema.h:4708 + /// conversion. + ExprResult tryConvertExprToTy(Expr *E, QualType Ty); + Please spell out "type" in the method name. Comment at: clang/include/clang/Sema/Sema.

<    5   6   7   8   9   10   11   12   13   14   >