[PATCH] D80323: [SVE] Eliminate calls to default-false VectorType::get() from Clang

rjmccall added a comment. I'm sympathetic to wanting to get rid of the boolean flag, but this is a really invasive change for pretty minimal benefit. Why not leave `VectorType::get` as meaning a non-scalable vector type and come up with a different method name to get a scalable vector? Repos

[PATCH] D80317: [SYCL] Prohibit arithmetic operations for incompatible pointers

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:10090 // if both are pointers check if operation is valid wrt address spaces - if (S.getLangOpts().OpenCL && isLHSPointer && isRHSPointer) { + if ((S.getLangOpts().OpenCL || S.getLangOpts().SYCLIsDevice)

[PATCH] D80295: [Sema] Diagnose static data members in classes nested in unnamed classes

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:6904 << Name << RD->getTagKind(); Invalid = true; +} else if (RD->isLocalClass()) { john.brawn wrote: > rjmccall wrote: > > This diagnostic actually ignores th

[PATCH] D80317: [SYCL] Prohibit arithmetic operations for incompatible pointers

rjmccall added a comment. Please add a C test case just using the address_space attribute. Comment at: clang/include/clang/AST/Type.h:1069 + /// qualifiers. + bool isAddressSpaceOverlapping(const QualType &T) const { +Qualifiers Q = getQualifiers(); It's

[PATCH] D80323: [SVE] Eliminate calls to default-false VectorType::get() from Clang

rjmccall added a comment. In D80323#2049374 , @ctetreau wrote: > In D80323#2048457 , @rjmccall wrote: > > > I'm sympathetic to wanting to get rid of the boolean flag, but this is a > > really invasive change for pr

[PATCH] D80323: [SVE] Eliminate calls to default-false VectorType::get() from Clang

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. I've responded to the llvmdev thread, which I missed before. I would like us to hold off on this line or work until we come to a resolution there. Repository: rG LLVM Github

[PATCH] D80317: [SYCL] Prohibit arithmetic operations for incompatible pointers

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/include/clang/AST/Type.h:1069 + /// qualifiers. + bool isAddressSpaceOverlapping(const QualType &T) const { +Qualifiers Q = getQualifiers();

[PATCH] D72959: Relative VTables ABI on Fuchsia

rjmccall added a comment. Otherwise LGTM. Comment at: clang/lib/CodeGen/CGVTables.cpp:633 + llvm::Module &module = CGM.getModule(); + llvm::SmallString<16> componentName(globalVal->getName()); + `componentName` seems to no longer be used meaningfully. in this

[PATCH] D79800: [Sema] Remove default values for arguments prior to a parameter pack if the pack is used

rjmccall added a comment. I wrote this comment but apparently never submitted it on Phabricator, sorry. Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1987 + +if (Function->getNumParams() >= NumTemplatedParams) { + unsigned FirstDefault = 0; ---

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

rjmccall added a comment. In D79744#2047482 , @arsenm wrote: > In D79744#2040731 , @rjmccall wrote: > > > In D79744#2040434 , @jdoerfert > > wrote: > > > > > In D79744#20403

[PATCH] D79800: [Sema] Remove default values for arguments prior to a parameter pack if the pack is used

rjmccall added inline comments. Comment at: clang/test/CXX/drs/dr7xx.cpp:225 template void f(int i = 0, T ...args) {} void ff() { f(); } Quuxplusone wrote: > Is this even supposed to compile? The only valid specializations of `f` > require `T...` to be an em

[PATCH] D71739: [AssumeBundles] Use operand bundles to encode alignment assumptions

rjmccall added a comment. > there is any IR legality restriction about what combination or number of > bundles can be present in an assume. however there is restrictions about > argument of a given bundle. > having a single "align" bundle is just how the front-end used assume bundles > for its

[PATCH] D80295: [Sema] Diagnose more cases of static data members in local or unnamed classes

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, looks great. Comment at: clang/lib/Sema/SemaDecl.cpp:6904 << Name << RD->getTagKind(); Invalid = true; +} else if (RD->isLocalClas

[PATCH] D76791: [Matrix] Implement matrix index expressions ([][]).

rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:2648 +/// MatrixSubscriptExpr - Matrix subscript expression for the MatrixType +/// extension. +class MatrixSubscriptExpr : public Expr { Oh, that's interesting. So you've changed this to

[PATCH] D80440: [OpenCL] Prevent fused mul and add by default

rjmccall added a comment. I think "the default value is on" is meant to imply that contraction is allowed by default, not that you can write the pragma without an `on-off-switch` and it means "on". FWIW, that would be consistent with C, which also enables contraction by default. CHANGES SINC

[PATCH] D80417: Fix Darwin 'constinit thread_local' variables.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4145 + // referenced directly, without calling the thread-wrapper, so the linkage + // must not be changed. + // Is this a good idea? It seems to be making `constinit` part of the

[PATCH] D80237: [hip] Ensure pointer in struct argument has proper `addrspacecast`.

rjmccall added a comment. `addrspacecast` might be a real conversion. I feel like this is really going well beyond what argument coercion should be expected to do, and we need to step back and re-evaluate how we're doing this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D80374: [Clang] Enable KF and KC mode for [_Complex] __float128

rjmccall added a comment. Would you mind separating these two patches? They don't seem have any interdependencies. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80374/new/ https://reviews.llvm.org/D80374 ___

[PATCH] D76793: [Matrix] Implement + and - operators for MatrixType.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:11989 + else +return false; + I would suggest checking some preconditions and then just calling `PrepareScalarCast`. You should allow implicit conversions from class types, which somew

[PATCH] D80237: [hip] Ensure pointer in struct argument has proper `addrspacecast`.

rjmccall added a comment. In D80237#2051909 , @hliao wrote: > In D80237#2051887 , @rjmccall wrote: > > > `addrspacecast` might be a real conversion. I feel like this is really > > going well beyond what argument c

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

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:8880 + } +} + tambre wrote: > rjmccall wrote: > > Hmm. I'm concerned about not doing any sort of semantic compatibility > > check here before we assign the function special semanti

[PATCH] D80417: Fix Darwin 'constinit thread_local' variables.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4145 + // referenced directly, without calling the thread-wrapper, so the linkage + // must not be changed. + // -

[PATCH] D80237: [hip] Ensure pointer in struct argument has proper `addrspacecast`.

rjmccall added a comment. Okay. Can you explain why we need to coerce in the first place, though? Especially if the representation is the same, why is your target-lowering requiring parameters to be coerced to involve pointers in a different address space? Repository: rG LLVM Github Monor

[PATCH] D80374: [Clang] Enable KF and KC mode for [_Complex] __float128

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3970 + DestWidth = 128; + break; case 'T': rjmccall wrote: > Are there interactions with the other mode specifiers? For example, should > this be allowed with integer mod

[PATCH] D80533: [Clang] Enable _Complex __float

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

[PATCH] D80374: [Clang] Enable KF and KC mode for [_Complex] __float128

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3970 + DestWidth = 128; + break; case 'T': Are there interactions with the other mode specifiers? For example, should this be allowed with integer modes? If so, I think

[PATCH] D76791: [Matrix] Implement matrix index expressions ([][]).

rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:2648 +/// MatrixSubscriptExpr - Matrix subscript expression for the MatrixType +/// extension. +class MatrixSubscriptExpr : public Expr { fhahn wrote: > rjmccall wrote: > > Oh, that's inte

[PATCH] D76791: [Matrix] Implement matrix index expressions ([][]).

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:4650 + (Base->isTypeDependent() || RowIdx->isTypeDependent() || + (ColumnIdx && ColumnIdx->isTypeDependent( +return new (Context) MatrixSubscriptExpr(Base, RowIdx, ColumnIdx, --

[PATCH] D80237: [hip] Ensure pointer in struct argument has proper `addrspacecast`.

rjmccall added a comment. In D80237#2055902 , @arsenm wrote: > In D80237#2051933 , @rjmccall wrote: > > > Okay. Can you explain why we need to coerce in the first place, though? > > Especially if the representati

[PATCH] D76793: [Matrix] Implement + and - operators for MatrixType.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:8587 +if (S.Context.hasSameType(M1, M2)) + AddCandidate(M1, M2); + I don't think this works if one side or the other has e.g. a templated conversion to a matrix type. Y

[PATCH] D76791: [Matrix] Implement matrix index expressions ([][]).

rjmccall added a comment. I also want to confirm explicitly that you've decided that the right language design is for matrix components to not be addressable. You're convinced that that's an important restriction to get the code generation you want? Repository: rG LLVM Github Monorepo CHAN

[PATCH] D76791: [Matrix] Implement matrix index expressions ([][]).

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:1900 + assert(!LV.isMatrixElt() && + "loads of matrix element LValues should be handled elsewhere"); assert(LV.isBitField() && "Unknown LValue type!"); This should be handled her

[PATCH] D80315: Fix CC1 command line options mapping into fast-math flags.

rjmccall added a comment. The code cleanups all seems reasonable. The actual changes in code-generation changes are because we were failing to set these reliably? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80315/new/ https://reviews.llvm.org/D

[PATCH] D76793: [Matrix] Implement + and - operators for MatrixType.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:11989 + else +return false; + fhahn wrote: > rjmccall wrote: > > I would suggest checking some preconditions and then just calling > > `PrepareScalarCast`. > > > > You should allow imp

[PATCH] D80315: Fix CC1 command line options mapping into fast-math flags.

rjmccall added a comment. In D80315#2058735 , @michele.scandale wrote: > In D80315#2058549 , @rjmccall wrote: > > > The code cleanups all seems reasonable. The actual changes in > > code-generation changes are be

[PATCH] D79800: [Sema] Implement DR2233

rjmccall added a comment. A couple of minor requests. Otherwise this looks good to me, although I really would like @rsmith to sign off on this approach, especially given his involvement with the DRs. Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1985 +// pa

[PATCH] D80374: [Clang] Enable KF and KC mode for [_Complex] __float128

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

[PATCH] D76793: [Matrix] Implement + and - operators for MatrixType.

rjmccall added a comment. Thanks, functionality is looking good for the non-assignment operators. Comment at: clang/lib/Sema/SemaExpr.cpp:12112 +return InvalidOperands(Loc, OriginalLHS, OriginalRHS); + } + You need to not actually apply this conversion to

[PATCH] D80315: Fix CC1 command line options mapping into fast-math flags.

rjmccall added a comment. In D80315#2059160 , @michele.scandale wrote: > In D80315#2058914 , @rjmccall wrote: > > > In D80315#2058735 , > > @michele.scandale wrote: > > > >

[PATCH] D76793: [Matrix] Implement + and - operators for MatrixType.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/lib/Sema/SemaExpr.cpp:12112 +return InvalidOperands(Loc, OriginalLHS, OriginalRHS); + } + fhahn wrote: > rjmccall wrote: > >

[PATCH] D80315: Fix CC1 command line options mapping into fast-math flags.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. >> I'm actually surprised it doesn't. I can't imagine why someone enabling >> fast math would want contraction to be disabled. > > Just to be clear the clang driver does the right thing.

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

rjmccall added a comment. I'll review this when it's rebased on top of the other commit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80462/new/ https://reviews.llvm.org/D80462 ___ cfe-commits mailing

[PATCH] D80323: [SVE] Eliminate calls to default-false VectorType::get() from Clang

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

[PATCH] D76791: [Matrix] Implement matrix index expressions ([][]).

rjmccall added a comment. > Yes at the moment I think we want to limit element wise > accesses/modifications to go through the access operator only, to guarantee > we can rely on the vector forms in codegen. > > Additionally I think at least initially we want to avoid handing out pointers > to

[PATCH] D76791: [Matrix] Implement matrix index expressions ([][]).

rjmccall added a comment. In D76791#2064434 , @fhahn wrote: > In D76791#2063926 , @rjmccall wrote: > > > > Yes at the moment I think we want to limit element wise > > > accesses/modifications to go through the acce

[PATCH] D80315: Fix CC1 command line options mapping into fast-math flags.

rjmccall added a comment. Per a private discussion, it seems like the right thing to do here would be stop recognizing `-ffast-math` flag in cc1 and just put the driver in charge of all these individual flags. That may necessitate adding a `-fdefine-fast-math` cc1 option to control the `#defin

[PATCH] D80237: [hip] Ensure pointer in struct argument has proper `addrspacecast`.

rjmccall added a comment. In D80237#2062991 , @arsenm wrote: > In D80237#2058108 , @rjmccall wrote: > > > In D80237#2055902 , @arsenm wrote: > > > > > In D80237#2051933

[PATCH] D80925: Fix compiler crash when trying to parse alignment argument as a constant expression.

rjmccall added a comment. Narrowly this seems to fix the immediate problem, but I feel like we're in trouble if tentative parsing is changing the semantic context in ways that persist. In particular, I'm concerned that we could end up tentatively parsing an ODR use as part of an expression and

[PATCH] D76791: [Matrix] Implement matrix index expressions ([][]).

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks. Comment at: clang/lib/CodeGen/CGExpr.cpp:3787 + ColIdx->getType()->getScalarSizeInBits()); + llvm::Type *IntTy = llvm::Intege

[PATCH] D80925: Fix compiler crash when trying to parse alignment argument as a constant expression.

rjmccall added a comment. In D80925#2066915 , @ABataev wrote: > In D80925#2066728 , @rjmccall wrote: > > > Narrowly this seems to fix the immediate problem, but I feel like we're in > > trouble if tentative parsing

[PATCH] D80315: Fix CC1 command line options mapping into fast-math flags.

rjmccall closed this revision. rjmccall added a comment. 8a8d703be0986dd6785cba0b610c9c4708b83e89 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80315/new/ https://reviews.llvm.o

[PATCH] D80925: Fix compiler crash when trying to parse alignment argument as a constant expression.

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseExpr.cpp:1009 + Actions, Sema::ExpressionEvaluationContext::Unevaluated, + Sema::ReuseLambdaContextDecl); + Res = Actions.TransformToPotentiallyEvaluated(Res.get()); --

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

rjmccall added a comment. In addition to everyone else's concerns about generating this number randomly, it's also inherently less testable. I'm sure there's something else you could use that's reasonably likely to be unique, like a hash of the input filename or output filename or a combination

[PATCH] D80925: Fix compiler crash when an expression parsed in the tentative parsing and must be claimed in the another evaluation context.

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseExpr.cpp:1009 + Actions, Sema::ExpressionEvaluationContext::Unevaluated, + Sema::ReuseLambdaContextDecl); + Res = Actions.TransformToPotentiallyEvaluated(Res.get()); --

[PATCH] D80925: Fix compiler crash when an expression parsed in the tentative parsing and must be claimed in the another evaluation context.

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

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

rjmccall added a comment. In D79744#2069324 , @arsenm wrote: > In D79744#2050498 , @rjmccall wrote: > > > > For the purpose here, only the callee exists. This is essentially a > > > freestanding function, the entry

[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] 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 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] 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] 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] 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] 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. 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] 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. 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. 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. 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] 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] 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] 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] 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] 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] 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] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

rjmccall added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1508 void checkFunc(SourceLocation Loc, FunctionDecl *FD) { +auto DiagsCountIt = DiagsCount.find(FD); FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back(); yaxunl wrote

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + sepavloff wrote: > rjmccall wrote: > > sepavloff wrote: > > > erichkeane wrote: > > > > rjmccall wrote: > > > > > erichkeane wrote: > > > > > > rjmcc

[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

rjmccall added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1508 void checkFunc(SourceLocation Loc, FunctionDecl *FD) { +auto DiagsCountIt = DiagsCount.find(FD); FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back(); yaxunl wrote

[PATCH] D77379: [FPEnv] Use single enum to represent rounding mode

rjmccall added inline comments. Comment at: llvm/include/llvm/ADT/FloatingPointMode.h:26 +/// assigned to the rounding modes must agree with the values used by FLT_ROUNDS +/// (C11, 5.2.4.2.2p8). +enum class RoundingMode : int8_t { I agree that we should use one

[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

rjmccall added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1508 void checkFunc(SourceLocation Loc, FunctionDecl *FD) { +auto DiagsCountIt = DiagsCount.find(FD); FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back(); yaxunl wrote

[PATCH] D77028: Speed up deferred diagnostic emitter

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thank you. Minor request, but feel free to commit with that change. Comment at: clang/lib/Sema/Sema.cpp:1558 +// visited before. +if (Done.count(FD)) + retu

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. I would guess that some visitor still has a `VisitCompoundAssignOperator` implementation, and that the behavior of `VisitBinAssign` happens to do what's needed for it. You basically have two options: - Keep a `VisitCompoundAssignOperator` around in `StmtVisitor` for a

[PATCH] D77379: [FPEnv] Use single enum to represent rounding mode

rjmccall added a reviewer: scanon. rjmccall added inline comments. Comment at: llvm/include/llvm/ADT/FloatingPointMode.h:26 +/// assigned to the rounding modes must agree with the values used by FLT_ROUNDS +/// (C11, 5.2.4.2.2p8). +enum class RoundingMode : int8_t {

[PATCH] D77545: Represent FP options in AST by special Expression node

rjmccall added a comment. The goal here seems to be to avoid the need to store pragma state in operator expressions. As I mentioned in another review, I'm not sure how directly interesting that goal is if we can avoid memory overhead in the common case. Storing pragma state in operators certa

[PATCH] D77028: Speed up deferred diagnostic emitter

rjmccall added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1558 +// visited before. +if (Done.count(FD)) + return; yaxunl wrote: > rjmccall wrote: > > `insert` returns whether it changed the set, so you can combine this check > > with the i

[PATCH] D76182: [AVR] Support aliases in non-zero address space

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. I agree this needs a test case. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4550 // Create the new alias itself, but don't set a name yet. + unsigned AS

[PATCH] D77379: [FPEnv] Use single enum to represent rounding mode

rjmccall added inline comments. Comment at: llvm/include/llvm/ADT/FloatingPointMode.h:26 +/// assigned to the rounding modes must agree with the values used by FLT_ROUNDS +/// (C11, 5.2.4.2.2p8). +enum class RoundingMode : int8_t { sepavloff wrote: > rjmccall wr

[PATCH] D77379: [FPEnv] Use single enum to represent rounding mode

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. I can accept this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77379/new/ https://reviews.llvm.org/D77379 __

[PATCH] D77545: Represent FP options in AST by special Expression node

rjmccall added a comment. In D77545#1972344 , @sepavloff wrote: > The solution in D76384 (Move FPFeatures > from BinaryOperator bitfields to Trailing storage) causes several concerns. > > 1. It requires substantial code c

[PATCH] D77545: Represent FP options in AST by special Expression node

rjmccall added a comment. > ! In D77545#1973765 , @sepavloff > wrote: > >> ! In D77545#1973169 , @rjmccall >> wrote: >> all of which will just do the wrong thing if they don't preserve and pass >> down the right

[PATCH] D73188: [AST] Improve overflow diagnostics for fixed-point constant evaluation.

rjmccall accepted this revision. rjmccall added a comment. LGTM. I'm somewhat surprised that this is the implementation design for integer overflow diagnostics, but since it is, I see no reason not to adopt it for fixed-point as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. Okay, that's fine, it's definitely a simpler change to not mess with the basic node hierarchy. The one problem I see is that you've (accidentally?) set up the `FPOptions` methods so that you can't call them on a `BinaryOperator` that's actually a `CompoundAssignOperat

[PATCH] D77592: [NFC}[CodeGen] Make VTable initialization a method of CGCXXABI

rjmccall added a comment. This is a weird point to allow further ABI customization of. I understand why you want to customize this, but I wonder if it's actually worthwhile to make a `virtual` function for it vs. just checking some sort of flag in the builder. Isn't there quite a lot of struc

[PATCH] D57226: [Fixed Point] [AST] Add an AST serialization code for fixed-point literals.

rjmccall added a comment. Some minor requests, but otherwise this LGTM. Comment at: include/clang/Serialization/ASTBitCodes.h:1637 + /// A FixedPointLiteral record. + EXPR_FIXEDPOINT_LITERAL, + rjmccall wrote: > ebevhan wrote: > > I'm unsure if this i

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

rjmccall added a comment. Scanned through the first bit. Comment at: clang/docs/LanguageExtensions.rst:500 +Clang provides a matrix extension, which is currently being implemented. See +:ref:`matrixsupport` for more details. + This should include just a bit mor

[PATCH] D77754: [MS] Fix packed struct layout for arrays of aligned non-record types

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, so this is basically just saying that the required-alignment-defeats-packed rule looks through arrays? Make sense. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D77743: [HIP] Emit symbols with kernel name in host binary

rjmccall added a comment. Is the renaming just being done to avoid breakpoints from triggering in the stub? Can you not disable debugging the stub using whatever mechanism `__attribute__((nodebug))` uses? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77743/new/ https://reviews.llvm.o

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

rjmccall added a comment. If someone writes up a short proposal for this, with motivation and impact, we'd be happy to present it internally. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 _

[PATCH] D77954: [CUDA][HIP] Fix overload resolution issue for device host functions

rjmccall added a comment. If `nvcc` ignores host/device-ness when selecting overloads, that's probably the specified behavior, right? I agree that it would be better to not ignore it, but Clang shouldn't just make up better rules for languages with external specifications. CHANGES SINCE LAST

[PATCH] D77954: [CUDA][HIP] Fix overload resolution issue for device host functions

rjmccall added a comment. I'm not saying that we need to be bug-for-bug-compatible with `nvcc`, I'm just saying that we should be able to point to *something* to justify our behavior. I take it that the CUDA spec has rules for some amount of host/device-based overloading? What are they based

[PATCH] D77954: [CUDA][HIP] Fix overload resolution issue for device host functions

rjmccall added a comment. In D77954#1976456 , @yaxunl wrote: > In D77954#1976378 , @rjmccall wrote: > > > I'm not saying that we need to be bug-for-bug-compatible with `nvcc`, I'm > > just saying that we should be

<    9   10   11   12   13   14   15   16   17   18   >