[PATCH] D95624: [OpenCL][PR48896] Fix default address space in template argument deduction

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/D95624/new/ https://reviews.llvm.org/D95624 ___ cfe-commits mailing list cfe-commit

[PATCH] D62574: Add support for target-configurable address spaces.

rjmccall added a comment. Sorry for never actually reviewing this. I have no objection to taking a refactor that implements the Embedded C address-space overlap rules even if we don't have an in-tree target that uses it. I'll try to find time to review. Repository: rG LLVM Github Monorepo

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

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

[PATCH] D95691: Implement P2173 for attributes on lambdas

rjmccall added a comment. In D95691#2540619 , @rsmith wrote: > In D95691#2540450 , @rjmccall wrote: > >> The warning is a bit weird. If we don't think it's certain that the >> committee will adopt this syntax, I d

[PATCH] D95691: Implement P2173 for attributes on lambdas

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticGroups.td:269 +def CXXPre2BCompatPedantic : + DiagGroup<"c++98-c++11-c++14-c++17-c++20-compat-pedantic", [CXXPre2BCompat]>; Uh, I think we're a couple standard releases past the po

[PATCH] D95691: Implement P2173 for attributes on lambdas

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticGroups.td:269 +def CXXPre2BCompatPedantic : + DiagGroup<"c++98-c++11-c++14-c++17-c++20-compat-pedantic", [CXXPre2BCompat]>; Quuxplusone wrote: > aaron.ballman wrote: > > rjmccall w

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

rjmccall added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4409 + calls to other ``swiftasynccall`` functions must be in tail position + (in all language modes). In particular: + - For C, this implies that ``return async_f();`` (``async_f`` is another --

[PATCH] D95691: Implement P2173 for attributes on lambdas

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticGroups.td:269 +def CXXPre2BCompatPedantic : + DiagGroup<"c++98-c++11-c++14-c++17-c++20-compat-pedantic", [CXXPre2BCompat]>; rsmith wrote: > rjmccall wrote: > > Quuxplusone wrote: >

[PATCH] D110280: [modules] Fix IRGen assertion on accessing ObjC ivar inside a method.

rjmccall added a comment. Hmm. It sounds like we misbehave if we're working with a non-canonical ivar. While it does seem preferable in general for lookups done after merging modules to return the canonical ivar, I'm not sure we can rely on the AST only having references to that ivar, since e

[PATCH] D110485: Support [[no_unique_address]] for all targets.

rjmccall added a comment. We should not add a standard feature with ABI impact if we aren't certain we're going to match MSVC's ABI on it. I'm sorry, this needs to wait until MSVC releases a compiler that supports this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110485/new/ https:

[PATCH] D110068: [Clang][AST] Resolve FIXME: Remove ObjCObjectPointer from isSpecifierType

rjmccall added a comment. `ObjCObjectPointerType` used to have a much weirder representation for the builtin object types like `id` and `Class`, and it's possible that this was an artifact of that. Otherwise I'm not sure, sorry. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110068/new

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. I think it would be better to provide a generic ABI specification that is designed to "lower" `_BitInt` types into more basic concepts that ABIs typically already have rules for. It would be appropriate to note at the top that this is only a generic specification and

[PATCH] D110281: Change __builtin_sycl_unique_stable_name to just use an Itanium mangling

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Oh, well, that's a huge simplification! Thank you for pushing on this with the language group. This LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110281/new/ https://revi

[PATCH] D109948: [Clang] Enable _Complex __ibm128 type

rjmccall added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:1095 CanQualType FloatComplexTy, DoubleComplexTy, LongDoubleComplexTy; - CanQualType Float128ComplexTy; + CanQualType Float128ComplexTy, Ibm128ComplexTy; CanQualType VoidPtrTy, NullPtrTy; ---

[PATCH] D109950: [Clang] Enable IC/IF mode for __ibm128

rjmccall added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:44 #include "clang/Basic/TargetCXXABI.h" +#include "clang/Basic/TargetInfo.h" #include "clang/Basic/XRayLists.h" It's a shame to have to do this in such a pervasively-included head

[PATCH] D110485: Support [[no_unique_address]] for all targets.

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. MSVC gets to chose the ABI rules for their platform. It is not Clang policy to pick ABI rules and then break them later. If you'd like to contribute an implementation of `[[msvc

[PATCH] D92808: [ObjC][ARC] Use operand bundle 'clang.arc.rv' instead of explicitly emitting retainRV or claimRV calls in the IR

rjmccall added a comment. The ultimate code-generation consequences of this feature aren't very generic, so we shouldn't let ourselves get caught up designing an extremely general feature that ultimately either doesn't do what we need it to do or doesn't actually work for any of the generalizat

[PATCH] D96816: [ObjC] Encode pointers to C++ classes as "^v" if the encoded string would otherwise include template specialization types

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. You might want to test nested classes of class templates. I don't know if we encode nested classes in any reasonable way in the first place. I'm generally okay with being aggressive about

[PATCH] D96816: [ObjC] Encode pointers to C++ classes as "^v" if the encoded string would otherwise include template specialization types

rjmccall added a comment. Since this is just in pointee types, I suspect this is fine for dispatch outside of targets (like that one old Cray) where `sizeof(void*) != sizeof(some_struct*)`, which I don't believe we generally support. But yeah, if this affects GNU-runtime ivar mangling, we need

[PATCH] D96802: [Clang] Add proper target checks for SwiftAsync convention.

rjmccall added a comment. The TargetInfo classes are already target-architecture-specific, so it's somewhat strange for them all to funnel to a single function that then immediately switches on the target architecture. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D96802: [Clang] Add proper target checks for SwiftAsync convention.

rjmccall added a comment. In D96802#2569810 , @varungandhi-apple wrote: >> The TargetInfo classes are already target-architecture-specific, so it's >> somewhat strange for them all to funnel to a single function that then >> immediately switches on the

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

rjmccall added a comment. LGTM other than the TargetInfo thing. Comment at: clang/include/clang/Basic/TargetInfo.h:1424 + triple.isAArch64(); + } + As I commented in the other patch, I think this would be cleaner in the long term if you just impleme

[PATCH] D96838: Add GNU attribute 'retain'

rjmccall added a comment. GCC 11 hasn't been released yet, so can we still engage with GCC about the semantics of this attribute? This is a very low-level attribute, and I don't understand why it was made separate instead of just having `used` add the appropriate section flag on targets that s

[PATCH] D96838: Add GNU attribute 'retain'

rjmccall added a comment. In D96838#2578083 , @MaskRay wrote: > In D96838#2578073 , @rjmccall wrote: > >> GCC 11 hasn't been released yet, so can we still engage with GCC about the >> semantics of this attribute?

[PATCH] D96838: Add GNU attribute 'retain'

rjmccall added a comment. In D96838#2578101 , @MaskRay wrote: > In D96838#2578097 , @rjmccall wrote: > >> In D96838#2578083 , @MaskRay wrote: >> >>> In D96838#2578073

[PATCH] D96838: Add GNU attribute 'retain'

rjmccall added a comment. > I have documented the behaviors in clang/include/clang/Basic/AttrDocs.td. Do > you have suggestions on the wording? Included; please let me know what you think. Comment at: clang/include/clang/Basic/AttrDocs.td:63-76 +The attribute, when attached t

[PATCH] D96838: Add GNU attribute 'retain'

rjmccall added inline comments. Comment at: clang/include/clang/Basic/Attr.td:2655 + +def Retain : InheritableAttr { + let Spellings = [GCC<"retain">]; MaskRay wrote: > aaron.ballman wrote: > > MaskRay wrote: > > > aaron.ballman wrote: > > > > MaskRay wrote: > >

[PATCH] D96838: Add GNU attribute 'retain'

rjmccall added a comment. Yeah, both COFF and Mach-O have longstanding ways to protect linker dead-stripping, and the compiler already has to manually trigger them on those targets for `used`, so it's certainly implementable to also trigger them for `retain`-without-`used`. I just don't think

[PATCH] D97223: Add Alignment argument to IRBuilder CreateAtomicRMW and CreateAtomicCmpXchg.

rjmccall added a comment. Looks like you didn't update the .bc reader/writer and the .ll printer/parser. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97223/new/ https://reviews.llvm.org/D97223 ___ cfe-c

[PATCH] D97223: Add Alignment argument to IRBuilder CreateAtomicRMW and CreateAtomicCmpXchg.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Ah, sorry, I missed that. In that case, LGTM, but you may want to respond to other reviews. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D9

[PATCH] D95984: [CodeGen] Fix codegen for __attribute__((swiftasynccall)).

rjmccall added a comment. Okay, seems fine to me. Comment at: clang/lib/CodeGen/CGStmt.cpp:1156 + CallingConv::CC_SwiftAsync)) { +auto CI = cast(&Builder.GetInsertBlock()->back()); +CI->setTailCallKind(llvm::CallInst::TCK_Tail); Hmm. I guess t

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

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

[PATCH] D95984: [CodeGen] Fix codegen for __attribute__((swiftasynccall)).

rjmccall added a comment. Please add some C++ tests, just in case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95984/new/ https://reviews.llvm.org/D95984 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D103563: [HIP] Fix amdgcn builtin for long type

rjmccall added a comment. If you want a way to say "some 64-bit type" in the builtins file, we could add that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103563/new/ https://reviews.llvm.org/D103563

[PATCH] D101156: [Clang] Support a user-defined __dso_handle

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4425 + if (ReplaceInitWithGV) +Init = llvm::ConstantExpr::getBitCast(GV, GV->getValueType()); + Can we actually do this bitcast for arbitrary initializers? This seems problemat

[PATCH] D103603: [Sema][RISCV][SVE] Allow ?: to select Typedef BuiltinType in C

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

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

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

[PATCH] D103040: Print default template argument if manually specified in typedef declaration.

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

[PATCH] D101156: [Clang] Support a user-defined __dso_handle

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

[PATCH] D99696: [clang] NRVO: Improvements and handling of more cases.

rjmccall added a comment. When `__block` variables get moved to the heap, they're supposed to be moved if possible, not copied. It looks like PerformMoveOrCopyInitialization requires NRVO info to be passed in to ever do a move. Maybe it's being passed in wrong when building `__block` copy exp

[PATCH] D97224: Use Address for CGBuilder's CreateAtomicRMW and CreateAtomicCmpXchg.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Alright, well, this does look cleaner. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97224/new/ https://reviews.llvm.org/D97224 ___

[PATCH] D99517: Implemented [[clang::musttail]] attribute for guaranteed tail calls.

rjmccall added a comment. It's not generically true that "anything can be tail-called if it's `noreturn`". For one, `noreturn` doesn't imply that the function doesn't exit by e.g. throwing or calling `longjmp`. For another, the most important user expectation of tail calls is that a long seri

[PATCH] D107841: CodeGen: No need to check for isExternC if HasStrictReturn is already false

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

[PATCH] D107394: [AIX] "aligned" attribute does not decrease alignment

rjmccall added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1975 + bool AlignAttrCanDecreaseAlignment = + AlignIsRequired && (Ty->getAs() != nullptr || FieldPacked); + Okay, so first off, the comment and variable names here make this s

[PATCH] D97824: [ObjC][ARC] Don't add operand bundle `clang.arc.attachedcall` to a call if the call already has the operand bundle

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

[PATCH] D111814: Fix consteval crash when transforming 'this' expressions

rjmccall accepted this revision. rjmccall added a comment. This seems reasonable. You could also give `ExitFunctionBodyRAII` an explicit `pop()` operation, but either way is fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111814/new/ https://

[PATCH] D111669: No longer crash when a consteval function returns a structure

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. Hmm. Generally these cases are expected to handle the situation where there's no result slot passed in, which currently isn't exclusive to an ignored result (although you could

[PATCH] D109950: [Clang] Enable IC/IF mode for __ibm128

rjmccall added a comment. Oh, yes, I think this should be preserving the old logic there and just adding a new clause for explicit requests for ibm128, right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109950/new/ https://reviews.llvm.org/D1099

[PATCH] D109950: [Clang] Enable IC/IF mode for __ibm128

rjmccall added a comment. In D109950#3097544 , @eandrews wrote: > In D109950#3097161 , @rjmccall > wrote: > >> Oh, yes, I think this should be preserving the old logic there and just >> adding a new clause for e

[PATCH] D112975: Fix complex types declared using mode TC

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. For posterity in case someone tracks down this review: `TC` corresponds to an unspecified 128-bit format, which on some targets is a double-double format (like `__ibm128_t`) and on others

[PATCH] D109950: [Clang] Enable IC/IF mode for __ibm128

rjmccall added a comment. Thanks. @hubert.reinterpretcast, @qiucf, can you verify that other compilers for PPC follow the logic for `TF` / `TC` that we now have with Elizabeth's patch? There are three different type specifiers (`long double`, `__ibm128`, and `float128_t`) which represent form

[PATCH] D112941: [clang] Add support for the new pointer authentication builtins.

rjmccall added a comment. Mostly LGTM, although I am not the most unbiased reviewer. :) Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:839 +def err_ptrauth_disabled : + Error<"pointer authentication is disabled for the current target">; +def err_ptrauth_invalid_k

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. `std::addressof(&someFunction)` certainly ought to return a signed pointer under ptrauth, so if your goal here is to get a completely unadorned symbol address, I think you do need a different builtin, and it probably ought to return a `void*` to emphasize that it shoul

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. Yeah, I think that's a better name. The documentation can say that ideally this also wouldn't include things like the THUMB bit, but there are technical limitations that make it hard to achieve that. You could also make this Just Work for things like C++ member functi

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. In D108479#3113055 , @pcc wrote: > (Adding back @rsmith, @rjmccall.) > > In D108479#3113035 , @samitolvanen > wrote: > >> In D108479#3112492 , @

[PATCH] D112941: [clang] Add support for the new pointer authentication builtins.

rjmccall added inline comments. Comment at: clang/include/clang/Driver/Options.td:2865-2872 +let Group = f_Group in { + let Flags = [CC1Option] in { +def fptrauth_intrinsics : Flag<["-"], "fptrauth-intrinsics">, + HelpText<"Enable pointer-authentication intrinsics">; +

[PATCH] D113352: [clang] Run LLVM Verifier in modes without CodeGen too

rjmccall added a comment. Seems like a good idea to me. Minor request; otherwise, feel free to commit. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1438 +// Add it here so that it runs prior to the BC/LL emission pass +MPM.addPass(VerifierPass()); + T

[PATCH] D113352: [clang] Run LLVM Verifier in modes without CodeGen too

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5016 llvm::Constant *Resolver = - GetOrCreateLLVMFunction(IFA->getResolver(), ResolverTy, GD, + GetOrCreateLLVMFunction(IFA->getResolver(), ResolverTy, {},

[PATCH] D113352: [clang] Run LLVM Verifier in modes without CodeGen too

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:5016 llvm::Constant *Resolver = - GetOrCreateLLVMFunction(IFA->getResolver(), ResolverTy, GD, + GetOrCre

[PATCH] D105169: [Clang/Test]: Rename enable_noundef_analysis to disable-noundef-analysis and turn it off by default

rjmccall added a comment. Hmm. I wonder if that's related to the problem uncovered by the verifier in https://reviews.llvm.org/D113352. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105169/new/ https://reviews.llvm.org/D105169 __

[PATCH] D8467: C++14: Disable sized deallocation by default due to ABI breakage

rjmccall added a comment. Conceptually, this is (and will always be) a platform decision. On Apple platforms, we have a formalized concept of deployment target, where specific minimum OS versions support sized deallocation and others do not. On most other platforms, this is much vaguer — basi

[PATCH] D8467: C++14: Disable sized deallocation by default due to ABI breakage

rjmccall added a comment. In D8467#3125471 , @rnk wrote: > In D8467#3125386 , @rjmccall wrote: > >> Conceptually, this is (and will always be) a platform decision. On Apple >> platforms, we have a formalized concept

[PATCH] D112921: [clang] Enable sized deallocation by default in C++14 onwards

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. Yeah, I think this either needs deployment restriction on Apple platforms or it needs the thunk / weak-linking approach from the original patch when the target OS isn't recent en

[PATCH] D112921: [clang] Enable sized deallocation by default in C++14 onwards

rjmccall added a comment. In D112921#3127767 , @rnk wrote: > Let's not bring back the weak function thunk approach. That approach caused > problems described in my commit, D8467 , > which is why the code was removed. The

[PATCH] D95984: [CodeGen] Fix codegen for __attribute__((swiftasynccall)).

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

[PATCH] D97733: Fix infinite recursion during IR emission if a constant-initialized lifetime-extended temporary object's initializer refers back to the same object.

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

[PATCH] D95691: Implement P2173 for attributes on lambdas

rjmccall added a comment. Sorry. I have no problem with continuing the existing pattern, I guess. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95691/new/ https://reviews.llvm.org/D95691 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

rjmccall added a comment. We don't have any reason to allow attributes on any of the `@` statements, but there's no reason to disallow them grammatically, as long as we still diagnose them as invalid (which I assume is tested somewhere?). Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D97224: Use Address for CGBuilder's CreateAtomicRMW and CreateAtomicCmpXchg.

rjmccall added a comment. Do we really consider the existing atomic intrinsics to not impose added alignment restrictions? I'm somewhat concerned that we're going to produce IR here that's either really suboptimal or, worse, unimplementable, just because we over-interpreted some cue about alig

[PATCH] D97371: [clang][parser] Remove questionable ProhibitAttributes() call in objc parsing

rjmccall added a comment. We should never silently accept and ignore an attribute unless (1) that's allowable semantics for the attribute or (2) we have to for source compatibility. That test is specifically checking that we allow `__attribute__((nomerge))` before `@try` statements. Are we ju

[PATCH] D97777: Add __builtin_isnan(__fp16) testcase

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

[PATCH] D97224: Use Address for CGBuilder's CreateAtomicRMW and CreateAtomicCmpXchg.

rjmccall added a comment. In D97224#2604069 , @jyknight wrote: > In D97224#2596410 , @rjmccall wrote: > >> Do we really consider the existing atomic intrinsics to not impose added >> alignment restrictions? I'm so

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4450 +Value *Extra = Builder.CreateSelect(Cmp, Diff, Zero); +return RValue::get(Builder.CreateAdd(SizeCall, Extra)); } Okay, so you're implicitly increasing the coroutine size

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4450 +Value *Extra = Builder.CreateSelect(Cmp, Diff, Zero); +return RValue::get(Builder.CreateAdd(SizeCall, Extra)); } ychen wrote: > ychen wrote: > > rjmccall wrote: > > > Oka

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

rjmccall added a comment. Let's try to avoid adding a new builtin for what we acknowledge is a workaround. Builtins become part of the language supported by the compiler, so we shouldn't add them casually. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

rjmccall added a comment. In D97915#2607567 , @ychen wrote: > In D97915#2607338 , @rjmccall wrote: > >> Let's try to avoid adding a new builtin for what we acknowledge is a >> workaround. Builtins become part of t

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:4450 +Value *Extra = Builder.CreateSelect(Cmp, Diff, Zero); +return RValue::get(Builder.CreateAdd(SizeCall, Extra)); } lewissbaker wrote: > ychen wrote: > > rjmccall wrote: > >

[PATCH] D97857: [Matrix] Add support for matrix-by-scalar division.

rjmccall added inline comments. Comment at: clang/docs/MatrixTypes.rst:127 +is only supported for expression ``M1 / S1``, where ``M1`` is of matrix type +and ``S1`` is of a real type. You don't need to be so formal in this paragraph, because you're about to get

[PATCH] D97857: [Matrix] Add support for matrix-by-scalar division.

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

[PATCH] D98337: Add support for digit separators in C2x

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

[PATCH] D97224: Use Address for CGBuilder's CreateAtomicRMW and CreateAtomicCmpXchg.

rjmccall added a comment. In D97224#2604537 , @jyknight wrote: >> I'm less certain about what to do with the `__atomic_*` builtins > > The `__atomic` builtins have already been supporting under-aligned pointers > all along -- and that behavior is unchange

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. I don't know for certain, but I would guess that the kernel wants to get the address of the first instruction in the function for the purposes of some sort of later PC-based table lookup, which means that yes, it probably *does* need to bypass descriptors on CHERI / It

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. In D108479#3129571 , @jrtc27 wrote: > For CHERI there's the added complication that descriptors and trampolines can > exist for security reasons when crossing security domains, and you absolutely > should not let one compartmen

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

rjmccall added a comment. Can we split the patch which changes what types are enabled for various x86 sub-targets out from the patch that changes the semantics of operations on `_Float16`? Or is there a good reason it's disabled currently, namely because the semantics are wrong? ===

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. Do any of the proposed use cases actually require this to be a constant expression? Some of these patterns can be cheaply implemented with code generation even if the target doesn't otherwise support constants; for example, we could just mask the THUMB bit off on 32-b

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. If we do need to support constant expressions of this, I think we should have the constant evaluator produce an expression rather than an l-value, the way we do with some other builtin calls. That should stop the comparison problem more generally. Repository: rG L

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1315 + if ((SrcType->isHalfType() || iSFloat16Allowed) && + !CGF.getContext().getLangOpts().NativeHalfType) { // Cast to FP using the intrinsic if the half type itself isn't supported. -

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

rjmccall added a reviewer: scanon. rjmccall added a comment. + Steve Canon Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1315 + if ((SrcType->isHalfType() || iSFloat16Allowed) && + !CGF.getContext().getLangOpts().NativeHalfType) { // Cast to FP using the intrinsic i

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

rjmccall added a comment. Does GCC actually change the formal types of expressions to `float`, or is it doing this "no intermediate casts thing" as some sort of fp_contract-style custom emission of trees of expressions that involve `_Float16`? In any case, doing operation-by-operation emulation

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

rjmccall added a comment. In D113107#3137921 , @zahiraam wrote: > In D113107#3136464 , @rjmccall > wrote: > >> Does GCC actually change the formal types of expressions to `float`, or is >> it doing this "no inte

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

rjmccall added a comment. > Basically agree with everything John said, with a note that #3 is not quite > FP_CONTRACT, which allows evaluating an expression as if intermediate steps > were infinitely-precise, but rather FLT_EVAL_METHOD == 32 as defined in > ISO/IEC TS 18661-3: "evaluate operati

[PATCH] D114099: Enable `_Float16` type support on X86 without the avx512fp16 flag

rjmccall added inline comments. Comment at: clang/lib/Basic/Targets/X86.cpp:242 HasAVX512FP16 = true; - HasFloat16 = true; } else if (Feature == "+avx512pf") { We should probably be setting `HasLegalHalfType` here. Comment at:

[PATCH] D114099: Enable `_Float16` type support on X86 without the avx512fp16 flag

rjmccall added inline comments. Comment at: clang/lib/Basic/Targets/X86.cpp:372 + // Turn on _float16 for x86 (feature sse+) + HasFloat16 = SSELevel >= SSE2; pengfei wrote: > rjmccall wrote: > > Out of curiosity, why SSE2? SSE2 adds double-precision, but we

[PATCH] D114099: Enable `_Float16` type support on X86 without the avx512fp16 flag

rjmccall requested changes to this revision. rjmccall added inline comments. This revision now requires changes to proceed. Comment at: clang/test/SemaCXX/Float16.cpp:4 +// RUN: %clang_cc1 -fsyntax-only -verify -triple armv7a-linux-gnu %s +// RUN: %clang_cc1 -fsyntax-only -verify

[PATCH] D113709: Don't consider `LinkageSpec` when calculating DeclContext `Encloses`

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. > We don't properly handle lookup through using declarations when there is a > linkage spec in the common chain. Pedantic note: you mean using *directives*. At some point, we should proba

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. In D108479#3140638 , @samitolvanen wrote: > In D108479#3133578 , @rjmccall > wrote: > >> If we do need to support constant expressions of this > > Yes, we need this also in constant exp

[PATCH] D114099: Enable `_Float16` type support on X86 without the avx512fp16 flag

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/D114099/new/ https://reviews.llvm.org/D114099 ___ cfe-commits mailing list c

[PATCH] D100879: [Clang] Propagate guaranteed alignment for malloc and others

rjmccall added a comment. Platforms are permitted to make stronger guarantees than the C standard requires, and it is totally reasonable for compilers to assume that `malloc` meets the target platform's documented guarantees. Arguably, libraries should not be replacing `malloc` if they fail to

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

rjmccall added a comment. For that example, yes, approach #3 would result in that exact same IR on targets that lack direct hardware support for `_Float16` operations. But getting that behavior right in general requires a different implementation than is provided by this patch, which is implem

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. In D108479#3149228 , @samitolvanen wrote: > I worked around this for now by explicitly allowing > `__builtin_function_start` in `CheckLValueConstantExpression`, but this seems > terribly hacky. What would be the correct way to

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