[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] 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] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. In D76384#1986761 , @mibintc wrote: > In D76384#1986525 , @mibintc wrote: > > > @rjmccall Can you check the patch added last night here, commit > > 3ee1ec0b9dd6ee2350f39ae8a418bf3ce28d06cf

[PATCH] D77592: [NFC][CodeGen] Add enum for selecting the layout of components in the vtable

rjmccall added a comment. In D77592#1987677 , @leonardchan wrote: > In D77592#1985740 , @rjmccall wrote: > > > I'm not sure if the AST-level v-table layout abstraction really cares about > > these differences. I d

[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:511 +*r = *a + (*b * *c); + } + This is kindof an unnecessarily unreadable example. I know you haven't decided on calling convention treatment yet, but maybe the leading example

[PATCH] D77221: [AVR] Rework MCU family detection to support more AVR MCUs

rjmccall added a comment. Patch generally looks good, but I agree that it could use some basic tests. You don't need to do specifically test all 20 million releases, but make sure you cover the first and last in the array as well as something in the middle, just to make sure your scans are wor

[PATCH] D78495: [nfc] Accept addrspacecast allocas in InitTempAlloca

rjmccall added a comment. You're not testing an assertion, you're testing that code is generated correctly for some file on amdgcn. Just write an ordinary IR-generation test that currently crashes and this test fixes. This is not an NFC change. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D77954: [CUDA][HIP] Fix host/device based overload resolution

rjmccall added a comment. Okay, one minor fix. Comment at: clang/lib/Sema/SemaOverload.cpp:9389 + if (Cand2.Function->isInvalidDecl()) +return Comparison::Better; This is neglecting the case where they're both invalid. CHANGES SINCE LAST ACTION https

[PATCH] D78163: [AVR][NFC] Move preprocessor tests to Preprocessor directory

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

[PATCH] D77954: [CUDA][HIP] Fix host/device based overload resolution

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:9749 + if (isBetterMultiversionCandidate(Cand1, Cand2)) +return true; + erichkeane wrote: > yaxunl wrote: > > echristo wrote: > > > rjmccall wrote: > > > > yaxunl wrote: > > > > > rj

[PATCH] D78190: Add Bfloat IR type

rjmccall added a comment. Thanks for doing it this way, I think this is going to be much better long-term for LLVM. Comment at: llvm/docs/LangRef.rst:2896 + * - ``bfloat`` + - 16-bit brain floating-point value (8-bit mantissa) + scanon wrote: > bfloat an

[PATCH] D77954: [CUDA][HIP] Fix host/device based overload resolution

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:9749 + if (isBetterMultiversionCandidate(Cand1, Cand2)) +return true; + tra wrote: > rjmccall wrote: > > erichkeane wrote: > > > yaxunl wrote: > > > > echristo wrote: > > > > > rjmcc

[PATCH] D77954: [CUDA][HIP] Fix host/device based overload resolution

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

[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/include/clang/AST/Expr.h:710 + /// possible to compute the alignment, return zero. + CharUnits getAlignmentFromDecl(ASTContext &Ctx) const; + Nothing about the name here suggests that it only works on expression

[PATCH] D78655: [HIP] Let lambda be host device by default

rjmccall added a comment. Seems reasonable Comment at: clang/test/CodeGenCUDA/lambda.cu:29 + +// Check lambda is emitted in device compilation and accessind device variable. +// DEV-LABEL: define internal void @_ZZ4mainENKUlvE_clEv Typo CHANGES SINCE LAST ACT

[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

rjmccall added a comment. LGTM with one very minor fix. Comment at: clang/docs/LanguageExtensions.rst:511 +*r = *a + (*b * *c); + } + fhahn wrote: > rjmccall wrote: > > This is kindof an unnecessarily unreadable example. I know you haven't > > decided o

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

rjmccall added a comment. Is there something currently restricting these types to just x86, or do you need to do a much broader audit? Comment at: clang/lib/CodeGen/TargetInfo.cpp:2980 -return (Ty->isPromotableIntegerType() ? ABIArgInfo::getExtend(Ty) -

[PATCH] D77592: [NFC][CodeGen] Add enum for selecting the layout of components in the vtable

rjmccall added a comment. Okay. The plan sounds good to me. So you want to roll out this enum in this patch, but not really use it for much until the follow-up? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77592/new/ https://reviews.llvm.org/D7

[PATCH] D72959: Relative VTables ABI on Fuchsia

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:623 +llvm::Constant *C, llvm::GlobalVariable *VTable, unsigned vtableIdx, +unsigned lastAddrPoint) const { + // No need to get the offset of a nullptr. There's already an `addRel

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:2980 -return (Ty->isPromotableIntegerType() ? ABIArgInfo::getExtend(Ty) - : ABIArgInfo::getDirect()); +if (!Ty->isExtIntType()) + return (Ty->isPro

[PATCH] D78190: Add Bfloat IR type

rjmccall added inline comments. Comment at: llvm/docs/LangRef.rst:2896 + * - ``bfloat`` + - 16-bit brain floating-point value (8-bit mantissa) + rjmccall wrote: > scanon wrote: > > bfloat and fp128 should agree w.r.t. whether or not the implicit bit > > co

[PATCH] D78970: [CUDA][HIP] Fix ambiguity of new operator

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. This really has nothing to do with the `new` operator specifically, but it looks like a good fix. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78970/new/ https://reviews.llvm.or

[PATCH] D72959: Relative VTables ABI on Fuchsia

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:623 +llvm::Constant *C, llvm::GlobalVariable *VTable, unsigned vtableIdx, +unsigned lastAddrPoint) const { + // No need to get the offset of a nullptr. leonardchan wrote: > leona

[PATCH] D79052: [clang codegen] Fix alignment of "Address" for incomplete array pointer.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:176 CharUnits Alignment; - if (T->isIncompleteType()) { + if (T->getBaseElementTypeUnsafe()->isIncompleteType()) { Alignment = CharUnits::One(); // Shouldn't be used, but pessimistic is b

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

rjmccall added a comment. Could you answer my question up-thread about whether ExtInt is currently target-limited? If it isn't, we need to more broadly audit targets. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78785/new/ https://reviews.llvm.org/D78785 __

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

rjmccall added a comment. Every target does something similar, they just all do it in different ways because they're mostly written by different people. We should restrict this feature to targets where we've adequately audited the ABI. It's not a feature until the ABI work is done. Repositor

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

rjmccall added a comment. In D78785#2010738 , @erichkeane wrote: > In D78785#2010654 , @rjmccall wrote: > > > Every target does something similar, they just all do it in different ways > > because they're mostly wr

[PATCH] D78785: Fix x86/x86_64 calling convention for _ExtInt

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

[PATCH] D72281: [Matrix] Add matrix type to Clang.

rjmccall added inline comments. Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1274 + TRY_TO(TraverseType(TL.getTypePtr()->getElementType())); +}) + Might as well do this instead of accumulating the technical debt. MatrixTypeLoc should store the att

[PATCH] D72281: [Matrix] Add matrix type to Clang.

rjmccall added inline comments. Comment at: clang/include/clang/AST/RecursiveASTVisitor.h:1274 + TRY_TO(TraverseType(TL.getTypePtr()->getElementType())); +}) + fhahn wrote: > rjmccall wrote: > > Might as well do this instead of accumulating the technical debt.

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

rjmccall added a comment. Sorry, I should've been more straight with you. I'm still happy to raise this internally, but this is a hectic time of the year at Apple, and I'm not actually going to raise it until after WWDC. It wouldn't be making it into the new Xcode release anyway. Repository

[PATCH] D78827: Add support for #pragma clang fp reassoc(on|off) -- floating point control of associative math transformations

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:3159 +controlled with this pragma. +``#pragma clang fp allow_reassociation`` allows control over the reassociation +of floating point expressions. When enabled, this pragma allows the expression ---

[PATCH] D79279: Allow volatile parameters to __builtin_mem{cpy,move,set}

rjmccall added a comment. Most of the complexity of this patch is introduced by the decision to type-check these calls with a volatile-typed parameter, which seems like it does nothing but cause problems. If your goal is to make these functions do the right thing when given arbitrary pointer t

[PATCH] D79236: [docs] Regenerate DiagnosticsReference.rst

rjmccall added a comment. There's no good reason for this file to exist; documenting the warning options is useful, but the file should be generated as an intermediate output of the documentation build and then ultimately fed into Sphinx. There's even a TODO in the docs about it: > TODO: Gener

[PATCH] D72281: [Matrix] Add matrix type to Clang.

rjmccall added inline comments. Comment at: clang/include/clang/Basic/TypeNodes.td:73 def ExtVectorType : TypeNode; +def MatrixType : TypeNode; def FunctionType : TypeNode; I think some parts of your life might be easier if there were a common base class here.

[PATCH] D79118: Implement _ExtInt ABI for all ABIs in Clang, enable type for ABIs

rjmccall added a comment. A little cavalier with `byval`; I gave it a thorough audit, but you might want your own pass. Comment at: clang/lib/CodeGen/ABIInfo.h:107 +// only difference is that this considers _ExtInt as well. +bool isPromotableIntegerType(QualType Ty) co

[PATCH] D79279: Allow volatile parameters to __builtin_mem{cpy,move,set}

rjmccall added a comment. In D79279#2016496 , @jfb wrote: > In D79279#2015983 , @rjmccall wrote: > > > Most of the complexity of this patch is introduced by the decision to > > type-check these calls with a volatil

[PATCH] D79279: Allow volatile parameters to __builtin_mem{cpy,move,set}

rjmccall added a comment. In D79279#2016570 , @rjmccall wrote: > I do think this is a somewhat debatable change in the behavior of these > builtins, though. Let me put more weight on this. You need to propose this on cfe-dev. Repository: rG LLVM Gi

[PATCH] D79279: Allow volatile parameters to __builtin_mem{cpy,move,set}

rjmccall added a comment. In D79279#2016604 , @jfb wrote: > In D79279#2016573 , @rjmccall wrote: > > > In D79279#2016570 , @rjmccall > > wrote: > > > > > I do think this is

[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] 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] 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] 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] 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] 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 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] 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] 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 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] 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] 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] 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] 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] 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] 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] 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. 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] D78827: Add support for #pragma clang fp reassociate(on|off) -- floating point control of associative math transformations

rjmccall added a comment. Just minor requests now. Comment at: clang/docs/LanguageExtensions.rst:3177 +Both floating point reassociation and floating point contraction can be +controlled with this pragma. +``#pragma clang fp reassoc`` allows control over the reassociation -

[PATCH] D79118: Implement _ExtInt ABI for all ABIs in Clang, enable type for ABIs

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:4938 +if (EIT->getNumBits() > 128) + return getNaturalAlignIndirect(Ty, /*ByVal=*/true); + erichkeane wrote: > rjmccall wrote: > > Does this need to consider the aggregate-as-ar

[PATCH] D79236: [docs] Regenerate DiagnosticsReference.rst

rjmccall added a comment. In D79236#2018661 , @rsmith wrote: > In D79236#2015982 , @rjmccall wrote: > > > There's no good reason for this file to exist; documenting the warning > > options is useful, but the file s

[PATCH] D79236: [docs] Regenerate DiagnosticsReference.rst

rjmccall added a comment. In D79236#2018891 , @rsmith wrote: > In D79236#2018812 , @rjmccall wrote: > > > In D79236#2018661 , @rsmith wrote: > > > > > We do generate this doc

[PATCH] D79236: [docs] Regenerate DiagnosticsReference.rst

rjmccall added a comment. In D79236#2019184 , @rsmith wrote: > In D79236#2019005 , @rjmccall wrote: > > > Okay. So it doesn't just run `ninja docs-clang-html`? That's unfortunate. > > > It didn't last time I looke

[PATCH] D78655: [HIP] Add -fhip-lambda-host-device

rjmccall added a comment. I agree with Richard that just making lambdas HD by default in all modes seems like the right rule. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78655/new/ https://reviews.llvm.org/D78655 ___ cfe-commits mailing

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

rjmccall added a comment. Is it reasonable to figure out ahead of time that we can skip the null check completely? It'd be kindof nice to also take advantage of this at -O0 whenever we don't have real work to do. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://revie

[PATCH] D79213: [hip] Add noalias on restrict qualified coerced hip pointers

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2556-2563 + // Restrict qualified HIP pointers that were coerced to global pointers + // can be marked with the noalias attribute. + if (isCoercedHIPGlobalPointer(*this, getLangOpts(), ArgI, T

[PATCH] D72281: [Matrix] Add matrix type to Clang.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2112 + llvm::APSInt ArgRows(S.Context.getTypeSize(S.Context.IntTy), + ConstantMatrixArg->getNumRows()); + Result = DeduceNonTypeTemplateArgument( -

[PATCH] D72281: [Matrix] Add matrix type to Clang.

rjmccall added a comment. The test cases for function template partial specialization would look something like this: template using matrix = T __attribute__((matrix_type(R, C))); template struct selector {}; template selector<0> use_matrix(matrix m) {} template select

[PATCH] D78660: [SemaObjC] Add a warning for dictionary literals with duplicate keys

rjmccall added a comment. Some part of me wishes we could use expression profiling or ODR hashing or something like that for this, but I guess the semantics we're going for don't really match. Comment at: clang/lib/Sema/SemaExprObjC.cpp:948 +checkOneKey(IntegralKeys,

[PATCH] D79118: Implement _ExtInt ABI for all ABIs in Clang, enable type for ABIs

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:4938 +if (EIT->getNumBits() > 128) + return getNaturalAlignIndirect(Ty, /*ByVal=*/true); + rjmccall wrote: > erichkeane wrote: > > rjmccall wrote: > > > Does this need to consid

[PATCH] D78660: [SemaObjC] Add a warning for dictionary literals with duplicate keys

rjmccall accepted this revision. rjmccall added a comment. Anyway, LGTM. Comment at: clang/lib/Sema/SemaExprObjC.cpp:948 +checkOneKey(IntegralKeys, Result.Val.getInt(), Loc); + } +} erik.pilkington wrote: > bendjones wrote: > > rjmccall wrote:

[PATCH] D79118: Implement _ExtInt ABI for all ABIs in Clang, enable type for ABIs

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:4938 +if (EIT->getNumBits() > 128) + return getNaturalAlignIndirect(Ty, /*ByVal=*/true); + erichkeane wrote: > rjmccall wrote: > > rjmccall wrote: > > > erichkeane wrote: > > >

[PATCH] D78827: Add support for #pragma clang fp reassociate(fast|off) -- floating point control of associative math transformations

rjmccall added a comment. The reason we call FP contraction "fast" instead of "on" when it's cross-statement is because "on" is supposed to be consistent with the C language rules, whereas "fast" is stronger. There's no analogous situation with reassociation; I don't think the C standard is li

[PATCH] D79395: [clang][codegen] Hoist parameter attribute setting in function prolog.

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

[PATCH] D79394: [clang][codegen] Refactor argument loading in function prolog. NFC.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Oh, yes, if `Fn->getArg(n)` is no longer an `O(n)` linked-list walk, then this seems like a great change. I see that that happened... slightly over three years ago. Hooray for legacies.

[PATCH] D78827: Add support for #pragma clang fp reassociate(fast|off) -- floating point control of associative math transformations

rjmccall added a comment. That makes sense, thanks. I think I'm comfortable using on/off for this, but it's definitely good to stop and consider, and you're absolutely right that it needs to be documented. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D79118: Implement _ExtInt ABI for all ABIs in Clang, enable type for ABIs

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:719 + !getContext().getTargetInfo().hasInt128Type())) + return getNaturalAlignIndirect(Ty); + erichkeane wrote: > rjmccall wrote: > > It's very weird for 64 and 128 to be sh

[PATCH] D78827: Add support for #pragma clang fp reassociate(on|off) -- floating point control of associative math transformations

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

[PATCH] D72281: [Matrix] Add matrix type to Clang.

rjmccall added a comment. > The type of m1 matches the matrix argument in all 3 definitions of use_matrix > and for some reason the return type is not used to disambiguate the > definitions: C++ does not have the ability to use return types to disambiguate function calls. > I am not sure wher

[PATCH] D79118: Implement _ExtInt ABI for all ABIs in Clang, enable type for ABIs

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

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

rjmccall added a comment. We're also seeing test failures in Apple's Clang fork, e.g. test/CodeGen/finite-math.c:12:10: error: NSZ: expected string not found in input // NSZ: fadd nsz ^ :11:20: note: scanning from here define void @foo() #0 { ^ :15:9: n

[PATCH] D72281: [Matrix] Add matrix type to Clang.

rjmccall added a comment. In D72281#2023111 , @fhahn wrote: > Ah right, thanks for clarifying. I think I managed to fix the remaining > problems. Previously the patch did not handle DependentSizedMatrixTypes with > non-dependent constant dimensions prope

[PATCH] D78827: Add support for #pragma clang fp reassociate(on|off) -- floating point control of associative math transformations

rjmccall added a comment. That's actually really interesting. Is there a paper describing the desired model in more detail? I think the natural interpretation of this pragma is to say that the specific operations written within the pragma are considered to be associative and are therefore all

[PATCH] D79510: [PATCH] When pragma FENV_ACCESS is ignored do not modify Sema.CurFPFeatures

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

[PATCH] D72281: [Matrix] Add matrix type to Clang.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2143 +return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP, + DimExpr, Info, Deduced); +} else if (const Co

[PATCH] D77491: [Sema] Fix incompatible builtin redeclarations in non-global scope

rjmccall requested changes to this revision. rjmccall added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Sema/SemaDecl.cpp:3759 unsigned BuiltinID; - if (Old->isImplicit() && (BuiltinID = Old->getBuiltinID())) { + bool RevertedBuiltin

[PATCH] D72281: [Matrix] Add matrix type to Clang.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2143 +return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP, + DimExpr, Info, Deduced); +} else if (const Co

[PATCH] D72281: [Matrix] Add matrix type to Clang.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:2143 +return DeduceNonTypeTemplateArgument(S, TemplateParams, NTTP, +

[PATCH] D72959: Relative VTables ABI on Fuchsia

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.def:357 +"allow use of clang's relative C++ ABI " +"for classes with vtables on targets that support this") "Use an ABI-incompatible v-table layout that uses relat

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

rjmccall added a comment. In D72841#2027740 , @plotfi wrote: > Hi @rjmccall, I am also seeing similar failures. It is failing on apple's > master (and the swift branches as well) because ParseLangArgs and > ParseCodeGenArgs are getting called in the oppo

[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/Frontend/CompilerInvocation.cpp:3185 Opts.FiniteMathOnly = Args.hasArg(OPT_ffinite_math_only) || Args.hasArg(OPT_cl_finite_math_only) || Args.hasArg(OPT_cl_fast_relaxed_math); mibintc wrote: >

[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/Frontend/CompilerInvocation.cpp:3192 + Opts.NoHonorNaNs = + Opts.FastMath || CGOpts.NoNaNsFPMath || Opts.FiniteMathOnly; + Opts.NoHonorInfs = mibintc wrote: > @rjmccall I could set these by using Args.h

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