[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:208 +if (UnaryOp->getOpcode() == UnaryOperator::Opcode::UO_AddrOf) + E = UnaryOp->getSubExpr(); + samitolvanen wrote: > rjmccall wrote: > > samitolvanen wrote: > > > rjmccall wro

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall added a comment. Your builtin is using custom type-checking (`t`), which suppresses all the normal conversions that happen on expressions. Specifically, it skips lvalue-to-rvalue conversion, so in this example the argument ends up being an l-value reference to a variable rather than a

[PATCH] D113925: [HIP] Add HIP scope atomic operations

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:5587 // the GNU atomics specification, but we enforce it, because if we didn't it // would be very confusing. FIXME: For whom? How so? Diag(ExprRange.getBegin(), diag::err_atomic_op_needs

[PATCH] D114025: [clang][NFC] Inclusive terms: replace some uses of sanity in clang

rjmccall added subscribers: rsmith, rjmccall. rjmccall added a comment. If you aren't sure what a comment means, please feel free to CC Richard or me, and we might be able to help. Comment at: clang/include/clang/Analysis/CFG.h:520 /// to keep receiving compiler warnings

[PATCH] D113925: [HIP] Add HIP scope atomic operations

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:5587 // the GNU atomics specification, but we enforce it, because if we didn't it // would be very confusing. FIXME: For whom? How so? Diag(ExprRange.getBegin(), diag::err_atomic_op_needs

[PATCH] D114728: [Coroutine] Remove the prologue data of `-fsanitize=function` for split functions

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. I agree that coroutine resumption functions have a different formal type from the ramp function and so would need different treatment from `-fsanitize=functions` if it wants to s

[PATCH] D114025: [clang][NFC] Inclusive terms: replace some uses of sanity in clang

rjmccall added a comment. Those all look good, thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114025/new/ https://reviews.llvm.org/D114025 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

[PATCH] D114902: [Attrs] Elaborate on the trivial_abi documentation

rjmccall added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3217 +are balanced, and that this attribute can safely be applied to +reference-counting smart pointers. + I would change the first sentence here to: > Arguments of ``trivial_abi``

[PATCH] D111566: [SYCL] Fix function pointer address space

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:636-638 +unsigned AS = PointeeType->isFunctionTy() + ? getDataLayout().getProgramAddressSpace() + : Context.getTargetAddressSpace(ETy); aa

[PATCH] D112626: Convert float to double on __builtin_dump_struct

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2090-2094 +// Variadic functions expect the caller to promote float to double. +if (CanonicalType == Context.FloatTy) { + FieldPtr = + CGF.Builder.CreateFPExt(FieldPtr, CGF.ConvertTy

[PATCH] D112626: Convert float to double on __builtin_dump_struct

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:2090-2094 +// Variadic functions expect the caller to promote float to double. +if (CanonicalType == Context.FloatTy) { + FieldPtr = + CGF.Builder.CreateFPExt(FieldPtr, CGF.ConvertTy

[PATCH] D112626: Convert float to double on __builtin_dump_struct

rjmccall added a comment. In D112626#3167520 , @rafaelfranco wrote: > Hey all! Thanks for taking the time to review my patch and writing the > Compiler Explorer examples and everything. I had no idea this was the > essentially the wrong approach to thi

[PATCH] D114937: [PowerPC] [Clang] Fix alignment adjustment of single-elemented float128

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

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. I'm sorry for holding this up for so long by just not responding to your pings. Yes, I have no objection to you going forward with this patch, since we're still explicitly saying that it's not yet ABI-stable. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1086

[PATCH] D108479: [Clang] Add __builtin_addressof_nocfi

rjmccall accepted this revision. rjmccall added a comment. This looks great, thanks. Please feel free to commit with the requested minor change to the docs. Comment at: clang/docs/LanguageExtensions.rst:2555 +This builtin is not supported on all targets. The returned pointer

[PATCH] D112113: [ObjC] type method metadata `_imp`, messenger routine at callsite with program address space

rjmccall accepted this revision. rjmccall added a comment. Well, I don't envy you the amount of work you're going to have to do to get ObjC working on a Harvard architecture, but this patch LGTM as progress. Is AVR really going to be using the Darwin ObjC runtime, though? Are you planning to f

[PATCH] D25844: [Sema][ObjC] Warn about implicitly autoreleasing indirect parameters that are captured by blocks

rjmccall added a comment. That's not an unreasonable idea, and we did consider it, and in fact it's still on the table as something we could do. However, it's a significantly more complex change, and we're not committed to doing it, and so we wanted to at least put this warning in place to hel

[PATCH] D133668: [HLSL] Disable int16_t to avoid promote to int for HLSL.

rjmccall added a comment. Okay, this seems fine to me. I think you accidentally removed the word "promotion" from the patch title, though. I assume HLSL also provides `uint16_t`, and you should test the behavior for it. You should also test this for whatever 8-bit types HLSL provides (`char`,

[PATCH] D133668: [HLSL] Disable integer promotion to avoid int16_t being promoted to int for HLSL.

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

[PATCH] D136639: [CodeGen][ObjC] Fix a memory leak that occurs when a non-trivial C struct property is set using dot notation

rjmccall added inline comments. Comment at: clang/test/CodeGenObjC/nontrivial-c-struct-property.m:89 + +// CHECK: call void @__destructor_8_s0(ptr %[[AGG_TMP_ENSURED]]) + It looks like we're copying `a` into a temporary and then copying out of that temporary int

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3112 +result = CGF.Builder.CreateFPExt(result, ConvertType(E->getType())); + return result; +} zahiraam wrote: > rjmccall wrote: > > The first version of the fallback path (the on

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

rjmccall added a comment. Thanks, this is looking very close now. Please handle the unary minus case in the complex emitter as well; I think that's all we need in terms of basic features. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:296 + ComplexPairTy EmitUnpromotion(Qu

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

rjmccall added a comment. In D113107#3726496 , @zahiraam wrote: > I think I introduced a bug when replacing the VisitUnaryMinus/Plus/Imag/Real > with VisitMinus/Plus/Imag/Real. Now this simple test case is failing in the > non-promotion path (with the +

[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexical order before emission

rjmccall added a comment. There's no compiler interoperation problem here; it's just a semantic concern that someone could've been relying on the old behavior. The new behavior is (AIUI) clearly required by the language standard, and I don't think we want to get into the business of providing

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. `getSubExprAsWritten`, as the name suggests, is a query for exploring the source code as written; it should generally not be used in CodeGen, which should be respecting the semantics of the AST. If Sema is applying implicit conversions that aren't desirable, we should

[PATCH] D131555: [Clang] Propagate const context info when emitting compound literal

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

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:213 TestAndClearIgnoreImag(); +PromotionType = getPromotionType(E->getSubExpr()->getType()); +if (!PromotionType.isNull()) Same problem Comment at: cla

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:609 + ComplexPairTy Op; + PromotionType = getPromotionType(E->getSubExpr()->getType()); + if (!PromotionType.isNull()) zahiraam wrote: > rjmccall wrote: > > This is overwriting the

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:613 +result = EmitUnpromotion(promotionTy, E->getSubExpr()->getType(), result); + return result; +} You should unpromote only if we're not in a promoted context, which is to say,

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:613 +result = EmitUnpromotion(promotionTy, E->getSubExpr()->getType(), result); + return result; +} zahiraam wrote: > rjmccall wrote: > > You should unpromote only if we're not i

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

rjmccall added a comment. That sounds right to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113107/new/ https://reviews.llvm.org/D113107 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listi

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

rjmccall added a comment. IRGen has a strong postcondition about what IR types it expects specific from specific evaluations. Scalar expression emission is expected to return a scalar of type `ConvertType(E->getType())`, and complex expression emission is expected to return a pair of scalars o

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

rjmccall added a comment. In D113107#3736312 , @zahiraam wrote: > This is a reduced test case from the codegen/complex-strictfp.c > > _Complex double g1, g2; > double D; > > void foo(void) { > > g1 = g1 + D; > > } > > The issue is that we are calling in

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

rjmccall added inline comments. Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:207 +// CHECK-NEXT:[[EXT:%.*]] = fpext half [[TMP0]] to float +// CHECK-NEXT:store float [[EXT]], ptr [[RETVAL]], align 2 +// CHECK-NEXT:[[TMP1:%.*]] = load half, ptr [[RETVAL]], a

[PATCH] D132352: Introduce noread_thread_id to address the thread identification problem in coroutines

rjmccall added inline comments. Comment at: llvm/docs/LangRef.rst:1931 +This attribute indicates that the function does not read thread id. +The behavior of noread_thread_id shouldn't depend on the thread it lives in. + Suggestion: > This attribute indi

[PATCH] D132352: Introduce noread_thread_id to address the thread identification problem in coroutines

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:2260 Result.addFnAttr(llvm::Attribute::ReadNone); -else if (ReadOnly) + Result.addFnAttr(llvm::Attribute::NoReadThreadID); +} else if (ReadOnly) Hmm, my comment here got

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

rjmccall added inline comments. Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:207 +// CHECK-NEXT:[[EXT:%.*]] = fpext half [[TMP0]] to float +// CHECK-NEXT:store float [[EXT]], ptr [[RETVAL]], align 2 +// CHECK-NEXT:[[TMP1:%.*]] = load half, ptr [[RETVAL]], a

[PATCH] D132352: Introduce noread_thread_id to address the thread identification problem in coroutines

rjmccall added a comment. Okay. Doc parts LGTM, but I don't Comment at: llvm/include/llvm/IR/InstrTypes.h:1855 + } + void setNoReadThreadID() { addFnAttr(Attribute::NoReadThreadID); } + The closest existing precedent would suggest `doesNotReadThreadID` and

[PATCH] D132352: Introduce noread_thread_id to address the thread identification problem in coroutines

rjmccall added inline comments. Comment at: llvm/include/llvm/IR/InstrTypes.h:1863 + /// not access or only reads memory. + bool doesNotReadThreadIDNorLivesInPresplitCoroutine() const { +return doesNoReadThreadID() || !getFunction() || rjmccall wrote: > Thi

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. In D131979#3742399 , @yihanaa wrote: > As far as I know, turning on the -fsanitizer=alignment options when calling > __builtin_assume_aligned in C code, if > the 1st arg has volatile qualifier, Clang will emit "call void > @__u

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

rjmccall added a comment. Somehow we've taken a huge step back on unpromotion, and I'm worried you're now doing the exact thing I didn't want us doing and forcing all the downstream clients to handle the possibility of a promoted result. Comment at: clang/lib/CodeGen/CGExprCo

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

rjmccall added a comment. I'm not sure what optimization you mean. Because the ABI returns 16-bit and 32-bit FP values differently, there really isn't a way that we can return a value without going through a truncation/extension cycle. There's potential to eliminate those with IPO, but we sho

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:972 +} + } + auto result = Visit(const_cast(E)); The unary operator cases seem to have disappeared from this function. Comment at: clang/lib/CodeGen/CGExprCom

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

rjmccall added a comment. In D113107#3744505 , @pengfei wrote: >> I'm not sure what optimization you mean. Because the ABI returns 16-bit and >> 32-bit FP values differently, there really isn't a way that we can return a >> value without going through a

[PATCH] D132352: Introduce noread_thread_id to address the thread identification problem in coroutines

rjmccall added a comment. Seems okay to me, but like I said, it'd be good to get AA eyes on it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132352/new/ https://reviews.llvm.org/D132352 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

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

rjmccall added a comment. Thank you! I think all the invariants look right here; just some minor comments now, and then it should be ready to commit. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:403 + Builder.CreateStore(Val.second, ImagPtr, isVolatile); +} +

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

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

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. I'm a little worried about the shift to emit an unconditional error on `volatile`. Can we at least separate that into its own patch and just fix the bug in this one? And please try to reach out whoever originally added this feature to see if that restriction is okay

[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

rjmccall added a comment. From the test case, it looks like the builtin just ignores pointer to volatile, which should be preserved by the conversions you're now doing in Sema. That is, you should be able to just check `Ptr->getType()->castAs()->isVolatile()`. Repository: rG LLVM Github Mo

[PATCH] D133583: [clang][ubsan] Fix __builtin_assume_aligned incorrect type descriptor and C++ object polymorphic address

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

[PATCH] D134441: [ObjC][ARC] Don't use operand bundle "clang.arc.attachedcall" in codegen for Windows

rjmccall added a comment. Having some sort of reliable marker here emitted after the call is currently a correctness condition for using the ObjC ARC autorelease optimization. Apple's x86_64 ObjC runtime is constrained by ABI limitations, so we sniff for a particular, fairly reasonable code pa

[PATCH] D134507: [Clang] add missing ClangABICompat check

rjmccall added a comment. I don't think I agree that changes to template partial ordering rules are "ABI-breaking changes". They're breaking changes to *language semantics*, and the ABI break is downstream of that, just like any other semantic change to overload resolution would be. I think t

[PATCH] D135011: Add sin and cos llvm builtins

rjmccall added a comment. The titled of this patch should be something like "Add __builtin_elementwise_sin and __builtin_elementwise_cos". Can you explain why this uses a new builtin name instead of overloading the existing builtins to work on vectors? I can imagine reasons why, but I think i

[PATCH] D134507: [Clang] add missing ClangABICompat check

rjmccall added a comment. DRs do not modify existing language versions. We can decide as a vendor whether to apply DRs to existing language versions. The fact that this is a language semantics change makes me think that we should not in this case. Repository: rG LLVM Github Monorepo CHANG

[PATCH] D135011: Add builtin_elementwise_sin and builtin_elementwise_cos

rjmccall added a comment. Ah, I'd forgotten we had a bunch of elementwise builtins already. I wouldn't be surprised if I asked for that in that patch, actually, especially because the existing ones include some functions like `max` and `min` where you could easily imagine them being cross-lane

[PATCH] D128745: [c++] implements DR692, DR1395 and tentatively DR1432, about partial ordering of variadic template partial specialization or function template

rjmccall added a comment. Pinging this thread because it has more reviewers who probably have opinions about this. https://reviews.llvm.org/D134507 is a patch that adds `-fclang-abi-compat` support around the breaking change here, which basically means that targets using old `-fclang-abi-compa

[PATCH] D134507: [Clang] add missing ClangABICompat check

rjmccall added a comment. I left a comment on the old review thread; probably best to center the conversation there. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134507/new/ https://reviews.llvm.org/D134507 __

[PATCH] D128745: [c++] implements DR692, DR1395 and tentatively DR1432, about partial ordering of variadic template partial specialization or function template

rjmccall added a comment. In D128745#3833393 , @royjacobson wrote: > Is there consensus that applying this DR resolution is distruptive enough > that it's actually an issue? If clang-abi-compat is not a good way to offer > backwards compatibility in th

[PATCH] D133668: [HLSL] Use _BitInt(16) for int16_t to avoid promote to int.

rjmccall added a comment. If your goal is just to pass 16-bit types in some specific way without promotion, you can just do that in the ABI. C only requires `short` arguments to be promoted to `int` in variadic or unprototyped positions, and it's not legal under the C type compatibility rules

[PATCH] D133668: [HLSL] Use _BitInt(16) for int16_t to avoid promote to int.

rjmccall added a comment. Sure, but it's extremely easy to unpromote that arithmetic for most operators, and I'm sure LLVM already has a pass that will do that. The only thing that blocks unpromotion is when you feed a promoted result into something that's sensitive to working on a wider value

[PATCH] D133668: [HLSL] Use _BitInt(16) for int16_t to avoid promote to int.

rjmccall added a comment. In D133668#3847807 , @beanz wrote: > In D133668#3847163 , @rjmccall > wrote: > >> Sure, but it's extremely easy to unpromote that arithmetic for most >> operators, and I'm sure LLVM alr

[PATCH] D133668: [HLSL] Use _BitInt(16) for int16_t to avoid promote to int.

rjmccall added a comment. If you have `char`, would you want it to promote? Because turning `char` to `_BitInt(8)` is breaking with C on other grounds (like aliasing), for better or worse. So if you just don't want promotion, maybe you really should just disable promotion. Repository: rG

[PATCH] D131698: [Sema] Fix `ExtVectorElementExpr` tree transform for the `isArrow` case.

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

[PATCH] D135628: [clang][codegen] Don't emit atomic loads for threadsafe init if they aren't inline

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Makes sense. Nice code-size optimization at any rate. Hmm, actually, along those lines, should we do the same thing at `-Oz`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register

rjmccall added a comment. This seems be to a genuine target difference, right? PPC64 has this ABI rule: > An aggregate or union smaller than one doubleword in size is padded so that > it appears in the least significant bits of the doubleword. which overrides the standard rule for passing aggr

[PATCH] D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register

rjmccall added a reviewer: t.p.northover. rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:5461 // Otherwise, just use the general rule. + // TODO: a better approach may refer to SystemZABI use same logic for caller Please add th

[PATCH] D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register

rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:327 +/// if the argument is smaller than a slot, set this flag will force +/// right-adjust the argument in its slot irrespective of the type. static Address emitVoidPtrDirectVAArg(CodeGenFunction &

[PATCH] D128745: [c++] implements DR692, DR1395 and tentatively DR1432, about partial ordering of variadic template partial specialization or function template

rjmccall added a comment. I think that's fair. I don't know how much concrete information we'll get about this in a short period of time, but it's worth trying to collect it. Was this change in LLVM 15, or is it still unreleased? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

[PATCH] D133338: [clang][PowerPC] PPC64 VAArg use coerced integer type for direct aggregate fits in register

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/CodeGen/TargetInfo.cpp:327 +/// if the argument is smaller than a slot, set this flag will force +/// right-adjust the argument in its slot

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

rjmccall added a comment. In D135326#3851678 , @dblaikie wrote: > In D135326#3851672 , @dblaikie > wrote: > >> (abandoned this, but still kind of curious) >> >> @rjmccall - any background/history of having the CX

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

rjmccall added a comment. I don't remember the history of the `-fcxx-abi` flag at all, so if that's a driver flag, you'll probably need to investigate it more to remove it. Maybe it was added for testing purposes and only made a driver flag accidentally. Repository: rG LLVM Github Monorepo

[PATCH] D131701: [CodeGen][ObjC] Call synthesized copy constructor/assignment operator functions in getter/setter functions of non-trivial C struct properties

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

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

rjmccall added a comment. Does Fuchsia still need this? If those experiments have stabilized, maybe we can just remove it. Also, it should probably have been a -cc1 flag in the first place. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85802/new

[PATCH] D135919: [Clang] Set thread_local Itanium ABI guard variables before calling constructors.

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. We can't set the flag if initialization is aborted by an exception, which is not ruled out by the use of thread-unsafe statics. So this is not a correct change. More basically,

[PATCH] D135919: [Clang] Set thread_local Itanium ABI guard variables before calling constructors.

rjmccall added a comment. Sorry, I responded as if you were modifying the guards of static constructors, but you're actually modifying the guards of thread locals. I apologize for the confusion. However, my substantive points actually all still hold: - we have to generate code that behaves co

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

rjmccall added a comment. If it comes down to it, we can make this a Fuchsia-specific flag, so that Fuchsia + alternative C++ ABI is essentially a sort of subtarget. That way we don't have to support the arbitrary Cartesian product of OS + ABI. With that said, while I don't know the details of

[PATCH] D135919: [Clang] Set thread_local Itanium ABI guard variables before calling constructors.

rjmccall added a comment. In D135919#3859520 , @hubert.reinterpretcast wrote: > In D135919#3859491 , @tahonermann > wrote: > >>> the case from https://github.com/llvm/llvm-project/issues/57828 is not for >>> a

[PATCH] D135919: [Clang] Set thread_local Itanium ABI guard variables before calling constructors.

rjmccall added a comment. Please add a comment something like this: The semantics of dynamic initialization of thread-local variables depends subtly on whether they are block-scope. The initialization of block-scope thread locals can be aborted with an exception and later retried (), and recur

[PATCH] D127695: [clang] Implement Template Specialization Resugaring

rjmccall added a comment. What is this work about? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127695/new/ https://reviews.llvm.org/D127695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https

[PATCH] D127695: [clang] Implement Template Specialization Resugaring

rjmccall added a comment. I see, thank you. I know you've marked this a WIP, but it's always good to describe these things in the patch description; that's what it's for. This is quite exciting! Thank you for looking into this. I think you could usefully be much more incremental here. Obvious

[PATCH] D136084: Fix LIT CodeGen/Func-attr.c

rjmccall added a comment. With these changes in place, this looks fine to me. The underlying problem here is that portable IR generation has gotten too difficult to test textually. FileCheck mostly works fine for testing IR passes, where we rarely worry about target dependencies and carefully

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

rjmccall added a comment. I suspect the Fuchsia project is not in fact volunteering to maintain a port of every imaginable C++ ABI to Fuchsia. (Many of these "ABIs" are specifically stuff like "the Itanium ABI as modified for iOS ARM64" and are not really portable anyway.) You may be interest

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

rjmccall added a comment. Okay. So Fuchsia multilib support uses a lot of more fine-grained options rather than being arch-driven? Can you elaborate about "each target"? Were you anticipating this being a broader feature outside of Fuchsia, or does Fuchsia actually encompass multiple OS targ

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

rjmccall added a comment. It's certainly possible that other targets will want to support multiple C++ ABIs in the future, but it's okay for us to design things around the situation we've got today. A lot of these "ABIs" are just target-specific variants; if it simplifies code to just make ABI

[PATCH] D136639: [CodeGen][ObjC] Fix a memory leak that occurs when a non-trivial C struct property is set using dot notation

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Oh, I see. That's a really unfortunate way to end up emitting this code pattern, since ignoring the result is so common. To fix that, we'd have to either figure out the result was unused

[PATCH] D137343: [clang] add -Wvla-stack-allocation

rjmccall added a comment. Aaron's suggested design makes the most sense to me, of all the designs I've seen here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137343/new/ https://reviews.llvm.org/D137343 _

[PATCH] D140868: [C] Make (c ? e1 : e2) noreturn only if both e1 and e2 are noreturn

rjmccall added a comment. LGTM, but I have a suggested elaboration for the comment. Comment at: clang/lib/AST/ASTContext.cpp:10258 - // FIXME: some uses, e.g. conditional exprs, really want this to be 'both'. - bool NoReturn = lbaseInfo.getNoReturn() || rbaseInfo.getNoRetur

[PATCH] D140868: [C] Make (c ? e1 : e2) noreturn only if both e1 and e2 are noreturn

rjmccall added a comment. Isn't the C feature not technically part of the type? I thought Clang was fairly unique in modeling `noreturn` the way we do. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140868/new/ https://reviews.llvm.org/D140868 __

[PATCH] D139564: clang: Don't emit "frame-pointer"="none"

rjmccall added a comment. Seems fine. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139564/new/ https://reviews.llvm.org/D139564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commi

[PATCH] D140868: [C] Make (c ? e1 : e2) noreturn only if both e1 and e2 are noreturn

rjmccall added a comment. In D140868#4027480 , @MaskRay wrote: > I am not a C language lawyer :) I wonder what else should be done to move > this patch forward. > The https://github.com/llvm/llvm-project/issues/59792 has got some traction > and has been

[PATCH] D137263: add boundary check for ASTUnresolvedSet::erase

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Good catch! Easy to see how this escaped notice for a decade, but still, it'll be good to fix. LGTM with a minor improvement. Comment at: clang/include/clang/AST/ASTUn

[PATCH] D136919: [X86][RFC] Change mangle name of __bf16 from u6__bf16 to DF16b

rjmccall added a comment. We talked about this on the Itanium list, and as currently specified, it is absolutely not correct for `__bf16` to have the same mangling as `std::bfloat16_t`, because these types do not have the same semantics. If GCC did that, it is a bug. That discussion was here:

[PATCH] D136919: [X86][RFC] Change mangle name of __bf16 from u6__bf16 to DF16b

rjmccall added a comment. In D136919#3906133 , @rjmccall wrote: > We talked about this on the Itanium list, and as currently specified, it is > absolutely not correct for `__bf16` to have the same mangling as > `std::bfloat16_t`, because `__bf16` does n

[PATCH] D136919: [X86][RFC] Change mangle name of __bf16 from u6__bf16 to DF16b

rjmccall added a comment. On the topic of supporting BF16 arithmetic, note my comment here: https://github.com/itanium-cxx-abi/cxx-abi/pull/147#issuecomment-1254078916. To summarize, according to Steve Canon, we really shouldn't implement arithmetic directly in the BF16 format, because the pre

[PATCH] D136919: [X86][RFC] Change mangle name of __bf16 from u6__bf16 to DF16b

rjmccall added a comment. Okay. That raises the question of what the default semantics should be for `std::bfloat16_t`, i.e. whether we should semantically default the type to using excess-precision `float` arithmetic. If we did, we'd still be able to demote solitary `float` operations to BF1

[PATCH] D134445: [PR57881][OpenCL] Fix incorrect diagnostics with templated types in kernel arguments

rjmccall added a comment. You really can't ask whether a class template pattern is standard layout; it's not meaningful. How are pointers and references passed to kernels? Does the pointee get copied or something? If so, you may have a requirement that pointee types be complete, in which cas

[PATCH] D134445: [PR57881][OpenCL] Fix incorrect diagnostics with templated types in kernel arguments

rjmccall added a comment. In D134445#3920257 , @Anastasia wrote: > In D134445#3920188 , @rjmccall > wrote: > >> You really can't ask whether a class template pattern is standard layout; >> it's not meaningful. >

[PATCH] D137381: [clang][compiler-rt] Exception escape out of an non-unwinding function is an undefined behaviour

rjmccall added a subscriber: jyknight. rjmccall added a comment. I don't have a problem with adding a sanitizer like this. IIRC, though, there's a review process for adding new sanitizers; I don't remember if this is the normal RFC process or something more streamlined. Also, I know @jyknight

[PATCH] D135919: [Clang] Correct when Itanium ABI guard variables are set for non-block variables with static or thread storage duration.

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

[PATCH] D138058: [Sema] Use the value category of the base expression when creating an ExtVectorElementExpr

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

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