[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rjmccall added inline comments. Comment at: clang/include/clang/Basic/Diagnostic.h:1086-1090 /// Note that many of these will be created as temporary objects (many call /// sites), so we want them to be small and we never want their address taken. /// This ensures that compile

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:487 + + // Walk both lists to get the full set of implied protocols + llvm::DenseSet AllImpliedProtocols; lanza wrote: > rjmccall wrote: > > You should add something like ", including all

[PATCH] D88913: [FPEnv] Use strictfp metadata in casting nodes

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:700 Res = ImplicitCastExpr::Create(Context, T, CK, E, nullptr, VK_RValue, - FPOptionsOverride()); + CurFPFeatureOverrides()); -

[PATCH] D87528: Enable '#pragma STDC FENV_ACCESS' in frontend cf. D69272 - Work in Progress

rjmccall added a comment. The GCC docs seem pretty clear that `-frounding-math` is meant to allow dynamic access to the FP environment. There might be optimizations that are perfectly legal with a constant but non-default rounding mode, and there might be some flag that only implies that weake

[PATCH] D76620: [SYCL] Implement __builtin_unique_stable_name.

rjmccall added a comment. Herald added a subscriber: yaxunl. Richard and I just found out about this commit, and we've decided to revert it. I apologize for the very late reversion, but the reality of Clang development is that it's very difficult for the code owners to watch literally every co

[PATCH] D89212: PR47663: Warn if an entity becomes weak after its first use.

rjmccall added a comment. This seems like a good idea in the abstract; we'll need to figure out what the practical consequences are. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89212/new/ https://reviews.llvm.org/D89212

[PATCH] D89212: PR47663: Warn if an entity becomes weak after its first use.

rjmccall added a comment. It does seem necessary to distinguish weak definitions from weak non-definitions, but that's completely reasonable — those two cases essentially act like totally different attributes that happen to be written with the same spelling. If we acknowledge that, I think tha

[PATCH] D76620: [SYCL] Implement __builtin_unique_stable_name.

rjmccall added a comment. Thank you. If it were SYCL-namespaced and -restricted, that would be fine, but as is it's positioned as a generic language feature. (I think we might still have had implementation/design feedback, though.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D89212: PR47663: Warn if an entity becomes weak after its first use.

rjmccall added a comment. Patch looks good to me. I think we're running some internal tests; do you want to wait for those? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89212/new/ https://reviews.llvm.org/D89212

[PATCH] D76620: [SYCL] Implement __builtin_unique_stable_name.

rjmccall added a comment. You know on both sides that a lambda is used as a kernel, yes? Why not simply introduce that into the mangling of lambdas, so that the subset of lambdas used as kernels form a stable sequence? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

[PATCH] D76620: [SYCL] Implement __builtin_unique_stable_name.

rjmccall added a comment. In D76620#2327910 , @erichkeane wrote: > In D76620#2327901 , @rjmccall wrote: > >> You know on both sides that a lambda is used as a kernel, yes? Why not >> simply introduce that into the

[PATCH] D76620: [SYCL] Implement __builtin_unique_stable_name.

rjmccall added a comment. In D76620#2327988 , @erichkeane wrote: > In D76620#2327976 , @rjmccall wrote: > >> In D76620#2327910 , @erichkeane >> wrote: >> >>> In D76620#23279

[PATCH] D76620: [SYCL] Implement __builtin_unique_stable_name.

rjmccall added a comment. In D76620#2328031 , @erichkeane wrote: > In D76620#2328011 , @rjmccall wrote: > >> In D76620#2327988 , @erichkeane >> wrote: >> >>> In D76620#23279

[PATCH] D89360: Treat constant contexts as being in the default rounding mode.

rjmccall added a comment. Okay. I can accept the fairly unfortunate thing with `const int` initializers. When is `IsConstantContext` set in C, just static initializers? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89360/new/ https://reviews.llv

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rjmccall added a comment. Thanks, this looks a lot better. Comment at: clang/include/clang/Basic/Diagnostic.h:1065 +/// +class StreamableDiagnosticBase { +public: I think I would prefer `StreamingDiagnostic` as the class name here. Comment at

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rjmccall added inline comments. Comment at: clang/include/clang/Basic/PartialDiagnostic.h:66 +return *this; + } + yaxunl wrote: > rjmccall wrote: > > Why are these template operators necessary? The LHS of the `<<` should > > convert to a base reference typ

[PATCH] D89475: [SemaObjC] Fix composite pointer type calculation for `void*` and pointer to lifetime qualified ObjC pointer type

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

[PATCH] D91874: [GNU ObjC] Fix a regression listing methods twice.

rjmccall added a comment. Functionally LGTM. I don't know if 11 is still taking changes, but if it is, you have code-owner approval. Comment at: clang/test/CodeGenObjC/gnu-method-only-once.m:8 +// merging happened was missed in the move and so we ended up emitting two +// cop

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added a comment. This seems like a good change, but we should make sure we test cases that may previously have been covered by the implicit deletion of all copy/move constructors. That may just be a matter of auditing tests. Comment at: clang/include/clang/Basic/Att

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:2986 purpose of calls. -A class annotated with ``trivial_abi`` can have non-trivial destructors or -copy/move constructors without automatically becoming non-trivial for the -purposes of calls. For

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6416 + if (D->hasAttr()) +return true; + I don't think we can just unconditionally check for the attribute. The rule for `trivial_abi` is that it overrides direct sources of non-tri

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6416 + if (D->hasAttr()) +return true; + zoecarver wrote: > rjmccall wrote: > > I don't think we can just unconditionally check for the attribute. The > > rule for `trivial_abi` is

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added a comment. That's an excellent example to study. The fundamental question we are looking to answer is whether a type representationally depends on its address. Absent the `trivial_abi` attribute, the criterion for knowing that it does not is whether it is possible to copy or mo

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added a comment. In D92361#2427652 , @zoecarver wrote: >> If it is not possible to copy or move the type at all, it is not possible to >> copy or move it trivially. > > This was a bit confusing to me because I think this changed between C++14 and

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added a comment. In D92361#2432578 , @ahatanak wrote: > It seems like you are discussing the case where a class/struct annotated with > `trivial_abi` contains a member that isn't destructively movable. In that > case, clang correctly diagnoses i

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added a comment. There is no such thing as an object "teleporting" in C++. Objects are always observed in memory with a specific address. When an argument is passed in registers, its address can be observed to be different on both sides, and that is not permitted; there must be some

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added a comment. In D92361#2433775 , @Quuxplusone wrote: > In D92361#2433190 , @rjmccall wrote: > >> There is no such thing as an object "teleporting" in C++. Objects are >> always observed in memory with

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

rjmccall added a comment. In D92361#2434376 , @zoecarver wrote: >> I think perhaps we are talking past each other and have reached the limits >> of what this sub-thread can hope to achieve. > > I agree. I am sure that we (or at least you two) could talk a

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

rjmccall added a comment. I agree that it seems reasonable to preserve an explicit CC from the lambda on the converted function pointer. Comment at: clang/lib/Sema/SemaLambda.cpp:1268 +calcLambdaConversionFunctionCallConv(Sema &S, + const Fu

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaLambda.cpp:1278 + if (CallOpCC == DefaultMember) +return DefaultFree; + return CallOpCC; ...I made this comment in my first review, but Phabricator threw it away. The attributes let you explici

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

rjmccall added a comment. No, if you put a calling convention on a lambda and then convert it to a function pointer, you almost certainly want that CC to be honored. Is the `AttributedType` still present in `CallOperator->getType()`? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89559/

[PATCH] D89360: Treat constant contexts as being in the default rounding mode.

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

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

rjmccall added a comment. We can't have it always match, that would require us to reject the conversion to a bare function-pointer type, which is required by the standard. I'm not sure if MSVC's multiple conversions hack is conformant or not, but it's at least more conformant than that. CHANG

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

rjmccall added a comment. Yeah, it might be reasonable to just do that on targets where the defaults are different (just MSVC, I think?) and otherwise preserve the method's calling convention. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89559/new/ https://reviews.llvm.org/D89559 __

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rjmccall accepted this revision. rjmccall added a comment. Okay. This LGTM if you feel that the operator forwarders is the better approach. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84362/new/ https://reviews.llvm.org/D84362 ___ cfe-com

[PATCH] D89903: [CodeGen] Crash instead of generating broken code with self-capturing __block var

rjmccall added a comment. It's not optimal, but an alternative would be to force the variable to the heap immediately rather than waiting for a potential block copy. The variable would actually be uninitialized during its "copy", so we'd need to give it a trivial copy helper. But once the var

[PATCH] D89903: [CodeGen] Crash instead of generating broken code with self-capturing __block var

rjmccall added a comment. Of course, that wouldn't solve the general problem that the block might be getting used before its capture is fully initialized, but that's a general problem with uses within initializers in C. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:/

[PATCH] D89903: [CodeGen] Crash instead of generating broken code with self-capturing __block var

rjmccall added a comment. We do not actually support allocation failure for a lot of things around blocks. I don't think the copy-helper functions even have a way to propagate out a failure in copying a field. I have never seen any code in the wild that would handle `Block_copy` returning a n

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

rjmccall added a comment. Mangling more calling conventions when mangling function types in Itanium (except at the top level) is the right thing to do. There's already a place to do so in the mangling. We just haven't done this yet because a lot of those calling convention are supported by ot

[PATCH] D83448: [CodeGen] Emit destructor calls to destruct non-trivial C struct temporaries created by conditional and assignment operators

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

[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

rjmccall added inline comments. Comment at: clang/lib/Basic/Targets/AMDGPU.cpp:57 +Constant, // sycl_constant +Private, // sycl_private Generic, // ptr32_sptr I feel like we may be outgrowing these address-space map arrays. Comme

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

rjmccall added a comment. In D71726#2207700 , @yaxunl wrote: > clang does not always emit atomic instructions for atomic builtins. Clang may > emit lib calls for atomic builtins. Basically clang checks target info about > max atomic inline width and if t

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

rjmccall added a comment. > Yes, there are no generically available libcalls for atomic float math -- but > that's okay -- let LLVM handle transform into a cmpxchg loop when required. I suspect Yaxun's target cannot provide libcalls at all, which is why he wants to diagnose up-front. But I agr

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. I have objections to the code change here. I'll leave the conceptual question to other people interested in the HIP toolchain. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 __

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. Argh, sorry! I meant to say "I have *no* objections". CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.ll

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:486 + if (LangOpts.HIP) +Options.AllowFPOpFusion = llvm::FPOpFusion::Standard; + tra wrote: > yaxunl wrote: > > tra wrote: > > > yaxunl wrote: > > > > tra wrote: > > > > > I don't

[PATCH] D89998: [c++20] For P0732R2 / P1907R1: Basic code generation and name mangling supportfor non-type template parameters of class type and template parameter objects.

rjmccall added inline comments. Comment at: clang/lib/AST/APValue.cpp:1047 + if (MergeLV(getLVForType(*TI.getType(), computation))) +break; +} else if (const Expr *E = V.getLValueBase().dyn_cast()) { I'm not sure what ABIs you're thinking about,

[PATCH] D90336: [Sema] Diagnose annotating `if constexpr` with a likelihood attribute

rjmccall added a comment. Patch generally looks good. Minor complaint about how you're using notes. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3163 def note_attribute_has_no_effect_here : Note< - "annotating the %select{infinite loop}0 here">; + "annotatin

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rjmccall added a comment. It sounds like what you want is a diagnostic library that's almost completely abstracted over the kinds of entities that can be stored in a diagnostic, including the definition of a source location. I don't think that's incompatible with this patch; there's no need to

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

rjmccall added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:1013 CXXMethodDecl *getLambdaStaticInvoker() const; + CXXMethodDecl *getLambdaStaticInvoker(CallingConv CC) const; + Probably worth clarifying in the comment which invoker is return

[PATCH] D89559: PR47372: Fix Lambda invoker calling conventions

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Alright, LGTM. Comment at: clang/include/clang/AST/DeclCXX.h:1013 CXXMethodDecl *getLambdaStaticInvoker() const; + CXXMethodDecl *getLambdaStaticInvoker(CallingConv C

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rjmccall added a comment. I may not have been clear. I'm not saying SourceLocation is a meaningful concept in the driver. I'm saying that if you generalize the concept of "source location" to "location in the input", there is a clear analogue in the driver (namely, a position in the argument

[PATCH] D83448: [CodeGen] Emit destructor calls to destruct non-trivial C struct temporaries created by conditional and assignment operators

rjmccall added a comment. Pointer authentication can also make structs non-trivial to copy. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83448/new/ https://reviews.llvm.org/D83448 ___ cfe-commits mailin

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. I agree this is useful. However, you need to update the manual to cover `faststd`. Could you also an IRGen test for this rather than only testing CUDA assembly output? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90174/new/ https://reviews.llvm.org/D90174

[PATCH] D90174: [HIP] Fix regressions due to fp contract change

rjmccall added a comment. Hmm. Do we actually want this behavior of `fast` overriding pragmas? What do other compilers do here? It might be reasonable to just treat this as a bug. Comment at: clang/docs/LanguageExtensions.rst:3214 +should be noted that ``-ffp-contract=fast`

[PATCH] D101968: Fix bad mangling of for a closure in the initializer of a variable at global namespace scope.

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

[PATCH] D102015: [clang CodeGen] Don't crash on large atomic function parameter.

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

[PATCH] D102443: [PowerPC] Added multiple PowerPC builtins

rjmccall added a comment. If these builtins are generally supposed to be accessed through a compiler-provided header, you could make that header define these functions using underlying builtins that have a `__builtin` prefix. That's not completely necessary, though; we do support some other no

[PATCH] D102443: [PowerPC] Added multiple PowerPC builtins

rjmccall added a comment. The Clang part of this looks fine to me; I can't review the backend changes, but you have partial approval. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102443/new/ https://reviews.llvm.org/D102443 _

[PATCH] D102689: [C++4OpenCL] Allow address space conversion in reinterpret_cast

rjmccall added a reviewer: rsmith. rjmccall added a comment. Top-level qualifiers aren't normally meaningful on pr-values. The C standard says that casts are to the unqualified type: N2454 6.5.4p5: Preceding an expression by a parenthesized type name converts the value of the expression to th

[PATCH] D102689: [C++4OpenCL] Allow address space conversion in reinterpret_cast

rjmccall added a comment. In D102689#2778072 , @rsmith wrote: > In D102689#2778011 , @rjmccall > wrote: > >> The C++ standard does not appear to have similar wording. On the other >> hand, the C++ standard says

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

rjmccall added a comment. In D96418#2780687 , @leonardchan wrote: > Hi all, it looks like this commit led to some unexpected behavior in our > build. When compiling something like: > > // clang++ -c -S -o - /tmp/test.cc -std=c++17 -O1 > static int do_

[PATCH] D102443: [PowerPC] Added multiple PowerPC builtins

rjmccall added a comment. Alright, that's fine with me, too. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102443/new/ https://reviews.llvm.org/D102443 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D102996: [ObjC][ARC] Use the addresses of the ARC runtime functions instead of integer 0/1 for the operand of bundle "clang.arc.attachedcall"

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

[PATCH] D103112: Reimplement __builtin_unique_stable_name-

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2413 +(or type of the expression) that is stable across split compilations, mainly to +support SYCL/Data Parallel C++ language. + The semantics here seem specific to SYCL. In fact, if

[PATCH] D102478: [Matrix] Emit assumption that matrix indices are valid.

rjmccall added a comment. Do you want to generate these even at -O0? Comment at: llvm/include/llvm/IR/MatrixBuilder.h:242 +auto *Cmp = B.CreateICmpULT(Idx, NumElts); +if (!isa(Cmp)) { + Function *TheFn = xbolva00 wrote: > Prefer early exit? Should

[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

rjmccall added a comment. In D98799#2761684 , @dblaikie wrote: > OK - poked around a bit more to better understand this, so attempting to > summarize my current understanding (excuse the repetition from previous parts > of this thread) and results. > > -

[PATCH] D103112: Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2443 + // Computes a unique stable name for the type of the given expression. + constexpr const char * __builtin_unique_stable_name( expression ); + These need to be updated for the r

[PATCH] D103112: Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name

rjmccall added a comment. Thanks, that seems to work out cleanly. Comment at: clang/include/clang/AST/Expr.h:2045 +// representation of the type (or type of the expression) in a way that permits +// us to properly encode information about the SYCL kernels. +class UniqueStableNa

[PATCH] D103112: Reimplement __builtin_unique_stable_name as __builtin_sycl_unique_stable_name

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

[PATCH] D96418: [clang] Refactor mustprogress handling, add it to all loops in c++11+.

rjmccall added a comment. There's no reliable way to report this with UBSan because in general we can't ever know that a loop will not terminate. That said, we could report *obviously* trivial loops, either with UBSan or just with a diagnostic. If the body of your loop really is empty, and th

[PATCH] D102689: [C++4OpenCL] Allow address space conversion in reinterpret_cast

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaCast.cpp:2359 - if (SrcType == DestType) { + if (SrcType == Self.Context.removeAddrSpaceQualType(DestType)) { // C++ 5.2.10p2 has a note that mentions that, subject to all other I think the u

[PATCH] D98799: [UniqueLinkageName] Use consistent checks when mangling symbo linkage name and debug linkage name.

rjmccall added a reviewer: ahatanak. rjmccall added a comment. Hmm. CC'ing Akira. Akira, do you know why we're building a fake FunctionDecl as part of emitting these helper functions? Is it something we can avoid? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://rev

[PATCH] D94979: [CGExpr] Use getCharWidth() more consistently in CCGExprConstant. NFC

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Clang has generally made a point of trying to be as neutral about `CHAR_BITS` as it can be, even though LLVM has not. As the code owner of this code, I view this as a fix consistent with

[PATCH] D92808: [ObjC][ARC] Annotate calls with attributes instead of emitting retainRV or claimRV calls in the IR

rjmccall added a comment. Okay, so it's important that we emit both attributes? I'll take your word for it. Comment at: llvm/include/llvm/Analysis/ObjCARCRVAttr.h:56 + if (hasRetainRVOrClaimRVAttr(CB)) +CB->removeAttribute(llvm::AttributeList::ReturnIndex, getRVAttrKeySt

[PATCH] D95070: Fix crash when emitting NullReturn guards for functions returning BOOL

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

[PATCH] D92808: [ObjC][ARC] Annotate calls with attributes instead of emitting retainRV or claimRV calls in the IR

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:2342 +attrs = attrs.addAttribute(CGF.getLLVMContext(), + llvm::AttributeList::ReturnIndex, "rv_marker"); +callBase->setAttributes(attrs); ahatanak wrote

[PATCH] D95181: [CodeGen][ObjC] Fix broken IR generated when there is a nil receiver check

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. This looks good, but in the long term it would be great if we could eliminate the need for `emitARCOperationAfterCall` and just emit the call properly in the first place. Please add a tes

[PATCH] D89490: Introduce __attribute__((darwin_abi))

rjmccall added a comment. In D89490#2514695 , @aguinet wrote: >> I may be over-reacting to the way the patch seemed to be touching on the C++ >> ABI in multiple places. My understanding is that `ms_abi` is just a >> calling-convention attribute; it's ba

[PATCH] D92808: [ObjC][ARC] Annotate calls with attributes instead of emitting retainRV or claimRV calls in the IR

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. I know you mentioned doing this for more targets in follow-up commits. It would be good to also do this for `-O0`, so that we have a single correct pattern coming out of the fronte

[PATCH] D92808: [ObjC][ARC] Annotate calls with attributes instead of emitting retainRV or claimRV calls in the IR

rjmccall added a comment. In D92808#2521042 , @rsmith wrote: > This change violates layering by including an Analysis header from IR; I'll > be landing a revert as soon as I've finished testing it. Should the header just be in IR? We'd like to avoid dup

[PATCH] D94987: DR39: Perform ambiguous subobject checks for class member access as part of object argument conversion, not as part of name lookup.

rjmccall added a comment. > You can. The rule is that you resolve using-declarations to the declarations > they name, and you resolve typedef declarations to the types they name, and > the resulting set of declarations and types must be the same in every > subobject in which there are (non-shad

[PATCH] D94987: DR39: Perform ambiguous subobject checks for class member access as part of object argument conversion, not as part of name lookup.

rjmccall added inline comments. Comment at: clang/include/clang/AST/CXXInheritance.h:77 - CXXBasePath() = default; + /// Additional data stashed on the base path by its consumers. + union { rsmith wrote: > rjmccall wrote: > > rsmith wrote: > > > rjmccall wro

[PATCH] D95488: Itanium Mangling: In 'enable_if', omit X/E around .

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

[PATCH] D95487: Itanium Mangling: Fix handling of in .

rjmccall added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:3912 +IsPrimaryExpr = false; + }; + I think it might be more reasonable to just check for the relatively small number of primary-expression cases in `mangleTemplateArgExpr` and skip

[PATCH] D93922: Itanium Mangling: Mangle `__alignof__` differently than `alignof`.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. This change looks good to me given the follow-up commits. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93922/new/ https://reviews.llvm.org/

[PATCH] D94987: DR39: Perform ambiguous subobject checks for class member access as part of object argument conversion, not as part of name lookup.

rjmccall added inline comments. Comment at: clang/include/clang/AST/CXXInheritance.h:77 - CXXBasePath() = default; + /// Additional data stashed on the base path by its consumers. + union { rsmith wrote: > rjmccall wrote: > > rsmith wrote: > > > rjmccall wro

[PATCH] D95487: Itanium Mangling: Fix handling of in .

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

[PATCH] D95487: Itanium Mangling: Fix handling of in .

rjmccall added a comment. I agree, that seems like the right decision. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95487/new/ https://reviews.llvm.org/D95487 ___ cfe-commits mailing list cfe-commits@li

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

rjmccall added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:1478 + enum class AtomicOperationKind { +Unsupported, +Init, This shouldn't be here; if you have places that don't always represent an atomic operation, queries for the k

[PATCH] D95608: [OpenCL][PR48896] Fix address space in binding of initializer lists to references

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:4297 + ? S.Context.getQualifiedType(T1, T1Quals.withoutAddressSpace()) + : cv1T1; // Not reference-related. Create a temporary and bind to that. Should we be rejecting th

[PATCH] D69322: [hip][cuda] Enable extended lambda support on Windows.

rjmccall added a comment. I agree that we should aim to avoid broad memory impact for uncommon features, and this seems to apply. Comment at: clang/include/clang/AST/DeclCXX.h:395-400 /// The number used to indicate this lambda expression for name /// mangling in the

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

rjmccall added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:1479 +Unsupported, +Init, +C11LoadStore, yaxunl wrote: > rjmccall wrote: > > `atomic_init` is not actually an atomic operation, so there's never an > > inherent reason

[PATCH] D95608: [OpenCL][PR48896] Fix address space in binding of initializer lists to references

rjmccall added a comment. Thanks, LGTM with the value-kind fix. Comment at: clang/lib/Sema/SemaInit.cpp:4308 + if (T1Quals.hasAddressSpace()) +Sequence.AddQualificationConversionStep(cv1T1, VK_XValue); +} else Anastasia wrote: > rjmccall wrote:

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

rjmccall added a comment. In D71726#2537101 , @yaxunl wrote: > In D71726#2537054 , @tra wrote: > >> In D71726#2536966 , @jyknight wrote: >> >>> My concern is that this is tre

[PATCH] D95608: [OpenCL][PR48896] Fix address space in binding of initializer lists to references

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. I think idiomatically we normally just use `(void)` instead of spelling it out with `static_cast`, since there's only one possible meaning for a cast to `void`. I don't car

[PATCH] D95561: [Clang] Introduce Swift async calling convention.

rjmccall added a comment. Broadly LGTM Comment at: clang/include/clang/Basic/AttrDocs.td:4398 + let Content = [{ +TODO + }]; varungandhi-apple wrote: > I have left this as a TODO for now, so that it can be filled in later when > the exact details of the

[PATCH] D95561: [Clang] Introduce Swift async calling convention.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8106-8115 + case ParsedAttr::AT_SwiftAsyncCall: case ParsedAttr::AT_VectorCall: case ParsedAttr::AT_MSABI: case ParsedAttr::AT_SysVABI: case ParsedAttr::AT_Pcs: case ParsedAttr::AT_IntelOc

[PATCH] D95910: Fix the guaranteed alignment of memory returned by malloc/new on Darwin

rjmccall added a comment. It's 16 bytes on all Darwin platforms, including 32-bit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95910/new/ https://reviews.llvm.org/D95910 ___ cfe-commits mailing list cf

[PATCH] D95691: Implement P2173 for attributes on lambdas

rjmccall added a comment. The patch seems technically okay to me. Do we need to recognize lambdas in any tentative-parse situations, or is it always the reverse (e.g. recognizing ObjC message sends as not-a-lambda)? The warning is a bit weird. If we don't think it's certain that the committee

<    15   16   17   18   19   20   21   22   23   24   >