[PATCH] D108464: [clang][CodeGen] Refactor CreateTempAlloca function nest. NFC.

rjmccall added a comment. In D108464#2960791 , @tlively wrote: > In D108464#2960623 , @rjmccall > wrote: > >> + JF, who knows something about Web Assembly, or can at least drag in the >> right people >> >> In D1

[PATCH] D107547: [CodeGen] Align calling convention bit-width to bitcode

rjmccall added inline comments. Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:562 /// The clang::CallingConv that this was originally created with. - unsigned ASTCallingConvention : 6; + unsigned ASTCallingConvention : 10; At some point, these b

[PATCH] D105972: Fix __attribute__((annotate("")) with non-zero globals AS

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

[PATCH] D105671: [Intrinsics][ObjC] Mark objc_retain and friends as thisreturn.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, thanks, seems like a good approach. Comment at: clang/test/CodeGenObjC/convert-messages-to-runtime-calls.m:34 + // CALLS-NOT: {{call.*@llvm.objc.release}} + // CA

[PATCH] D108609: [clang] NFC: remove superfluous braces

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

[PATCH] D67429: Improve code generation for thread_local variables:

rjmccall added a comment. @rsmith, can you fix or revert? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67429/new/ https://reviews.llvm.org/D67429 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.ll

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. I agree with James; I know you've reached out to the Itanium ABI group about mangling, but ABI involvement needs to mean also reaching out and getting psABI agreement about representation. I would suggest proposing a generic ABI, getting consensus on that generic ABI

[PATCH] D108680: PR48030: Fix COMDAT-related linking problem with C++ thread_local static data members.

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

[PATCH] D107021: [Sema][ObjC] Allow conversions between pointers to ObjC pointers and pointers to structs

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

[PATCH] D108618: [CGCall] Add NoInline attr if presented for the target

rjmccall added a comment. Why do you want to add `noinline` to a function declaration that lacks a definition? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108618/new/ https://reviews.llvm.org/D108618

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. In D108643#2964740 , @aaron.ballman wrote: > In D108643#2964190 , @rjmccall > wrote: > >> I agree with James; I know you've reached out to the Itanium ABI group about >> mangling, but

[PATCH] D93377: [Clang] Add __ibm128 type to represent ppc_fp128

rjmccall added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:131 +Float128, +Ibm128 }; qiucf wrote: > rjmccall wrote: > > This is necessary because it's possible to derive this type from a mode > > attribute? > Yes, `__attribute_

[PATCH] D108618: [CGCall] Add NoInline attr if presented for the target

rjmccall added a comment. Does LLVM model `noinline` as a call-site attribute in the way that would be necessary to get that effect? Also, are you actually having a problem here, or is this just something you noticed in the code? I'm not sure we can warn about putting the attribute on a declar

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

rjmccall added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1984 +auto checkTypeIsTypedef = [](auto &&checkTypeIsTypedef, + const auto Ty) -> bool { + bool isTypedef = false; Hah. The knot-tying here

[PATCH] D108618: [CGCall] Add NoInline attr if presented for the target

rjmccall added a comment. Alright, no worries. It was reasonable to ask if this was something we should fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108618/new/ https://reviews.llvm.org/D108618 _

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

rjmccall added a comment. Thanks, style suggestion aside, this looks great. Would you mind doing the TypeInfo change in a separate patch? The functional change for AIX can be a really small improvement on top of that. Comment at: clang/include/clang/AST/ASTContext.h:167 +

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

rjmccall added a comment. Please add me to the review for the other patch and I'll approve it pretty quickly, and then we can get back to this one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107394/new/ https://reviews.llvm.org/D107394 ___

[PATCH] D108858: TypeInfo records more information about align requirement

rjmccall added a comment. Thanks. Generally looks good; feel free to commit after adding a `RequiredByEnum` case and making sure it compiles. Comment at: clang/lib/AST/ASTContext.cpp:2300 Info.Align = AttrAlign; -Info.AlignIsRequired = true; +Info.Ali

[PATCH] D108858: TypeInfo records more information about align requirement

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

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

rjmccall added a comment. Thanks, this looks a lot cleaner. Minor requests only. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107394/new/ https://reviews.llvm.org/D107394 ___ cfe-commits mailing list c

[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); + stevewan wrote: > rjmccall wrote: > > stevewan wrote: > > > rjmccal

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

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

[PATCH] D108905: [ItaniumCXXABI] Set nounwind on __cxa_begin_catch/__cxa_end_catch

rjmccall added a reviewer: rsmith. rjmccall added a comment. Yes, this seems extremely reasonable; it was really just an oversight. Patch is semantically fine, although what we do in several other places is use `EmitNounwindRuntimeCall` to emit the call, and I think I'd like to do that here: I

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

rjmccall added a comment. Well, I'm saying two things. First, it is not clear to me what the expected behavior of that code is under the standard. The fact that it appears to work in one particular implementation is not in any way conclusive; we have to look at the specification. Second, I t

[PATCH] D108482: [Clang] Fix instantiation of OpaqueValueExprs (Bug #45964)

rjmccall added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:10515-10525 + // Note that SourceExpr can be nullptr. + ExprResult SourceExpr = TransformExpr(E->getSourceExpr()); + if (SourceExpr.isInvalid()) +return ExprError(); + if (SourceExpr.get() == E->ge

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

rjmccall added a comment. In D108905#2973066 , @modimo wrote: > In D108905#2971861 , @rjmccall > wrote: > >> I'm not really sure what the standard expects to happen if an exception >> destructor throws. The sta

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

rjmccall added a comment. In D108905#2973172 , @modimo wrote: > In D108905#2973099 , @rjmccall > wrote: > >> Yeah, I think this is the most natural interpretation of the current >> standard. But that would be a

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

rjmccall added a comment. Thanks, Richard. Fangrui, I think we can't do anything on this patch without a CWG decision about the right semantics, so this will have to sit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108905/new/ https://reviews.l

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

rjmccall added a comment. In D108905#2975712 , @rsmith wrote: > No decision as yet, but so far it looks very likely that we'll settle on the > rule that exceptions cannot have potentially-throwing destructors, and that > we should reject `throw`s of suc

[PATCH] D108482: [Clang] Fix instantiation of OpaqueValueExprs (Bug #45964)

rjmccall added a comment. Yes, that's right. OVE allows us to logically place a single expression in multiple positions in the expression hierarchy. This can be useful for either source or semantic fidelity. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D107589: [CMake] Support setting default fused FP contract via CMake

rjmccall closed this revision. rjmccall added a comment. I agree. This isn't the right approach. The patch in question is currently reverted; if you'd like to argue to not restore it, that conversation should be part of that review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D105671: [Intrinsics][ObjC] Mark objc_retain and friends as thisreturn.

rjmccall added a comment. How does `stripPointerCasts` "see through" an attribute on the underlying call instruction? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105671/new/ https://reviews.llvm.org/D105671 _

[PATCH] D105671: [Intrinsics][ObjC] Mark objc_retain and friends as thisreturn.

rjmccall added a comment. Whoa. That does not seem like it's in the contract for `stripPointerCasts()` generically. At best that should be enabled by the strip kind. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105671/new/ https://reviews.llvm.

[PATCH] D93377: [Clang] Add __ibm128 type to represent ppc_fp128

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

[PATCH] D107547: [CodeGen] More bits for calling convention

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

[PATCH] D109387: [AIX] Check for typedef properly when getting preferred type align

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, except it's missing tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109387/new/ https://reviews.llvm.org/D109387 ___

[PATCH] D109387: [NFC][AIX] Check for typedef properly when getting preferred type align

rjmccall added a comment. Oh, no, I don't think this is NFC. It seems very likely that there's some case where the behavior is distinguishable. Maybe a typedef of a record with an alignment attribute? __attribute__((aligned(2), packed)) struct float4 { float x, y, z, w; }; typedef struct

[PATCH] D109386: Fix use-after-free from GlobalCtors associated data

rjmccall added a comment. I agree with Reid. IRGen should never be holding naked references (across different invocations as an `ASTConsumer`) to a replaceable entity. There are some entities that are known not to be replaceable, but in general the associated data of a global constructor does

[PATCH] D109387: [AIX] Check for typedef properly when getting preferred type align

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

[PATCH] D109386: Change to use ValueHandle for associated data of GlobalCtors

rjmccall added a comment. It shouldn't need a large program to duplicate, but the test will need to exercise some unusual, arguably invalid conditions. We need to add an entry with associated data to `GlobalCtors` or `GlobalDtors` and then replace the associated data. We only actually use asso

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. In D108643#2999776 , @erichkeane wrote: >> ! In D108643#2965852 , @rjmccall >> wrote: >> >> The choice that high bits are unspecified rather than extended is an >> interesting one. Ca

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. In D108643#3000223 , @erichkeane wrote: > In D108643#3000193 , @rjmccall > wrote: > >> In D108643#2999776 , @erichkeane >> wrote: >> ! In

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. In D108643#3000556 , @erichkeane wrote: > In D108643#3000540 , @rjmccall > wrote: > >> > > I don't work on the microcode, it is just what I was told when we asked about > this. SO un

[PATCH] D103835: [CUDA][HIP] Fix store of vtbl in ctor

rjmccall added a comment. Hmm. I think "v-tables are in the address space of the object pointer" is not a good assumption. Probably this ought to be determined by the C++ ABI for the target. In principle it could even be class-specific, but I think we can start by assuming it's universal.

[PATCH] D103835: [CUDA][HIP] Fix store of vtbl in ctor

rjmccall added a comment. I see. Yes, in that case, that sounds right. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103835/new/ https://reviews.llvm.org/D103835 ___ cfe-commits mailing list cfe-commits

[PATCH] D109841: Fix vtbl field addr space

rjmccall added a comment. The following line is always just making a bitcast, then. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109841/new/ https://reviews.llvm.org/D109841 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lis

[PATCH] D109841: Fix vtbl field addr space

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

<    28   29   30   31   32   33