[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

rjmccall added a comment. I have no objection to trying to always emit lifetime intrinsics in coroutines since it has a less-trivial runtime cost. I am skeptical that it's reasonable to do this for *correctness*, however; I don't think the frontend unconditionally emits lifetime intrinsics. B

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

rjmccall added a comment. In D99227#2646532 , @rjmccall wrote: > I am skeptical that it's reasonable to do this for *correctness*, however; I > don't think the frontend unconditionally emits lifetime intrinsics. Sorry, I re-read this after posting, and i

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

rjmccall added a comment. Is it feasible to outline the initial segment that you don't want to be part of the coroutine, and then have coroutine splitting force that outlined function to be inlined into the ramp function? IIUC, you were saying that the splitting patch was difficult, but maybe

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

rjmccall added a comment. In D99227#2646710 , @lxfind wrote: >> I think you just set `ShouldEmitLifetimeMarkers` correctly in the first >> place instead of adding this as an extra condition to every place that >> considers it, however. > > This was set w

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

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

[PATCH] D99005: [clang] Implement P2266 Simpler implicit move

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1059 +else if (auto *F = dyn_cast(DC)) + goto unimplemented; // FIXME: get the return type here somehow... +else mizvekov wrote: > @rjmccall Hello! Do you have

[PATCH] D99005: [clang] Implement P2266 Simpler implicit move

rjmccall added a subscriber: fhahn. rjmccall added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:1059 +else if (auto *F = dyn_cast(DC)) + goto unimplemented; // FIXME: get the return type here somehow... +else mizvekov w

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:1240 + if (auto *CE = dyn_cast(RV)) +::makeTailCallIfSwiftAsync(CE, Builder, CurFnInfo); +} What's with the explicit `::` here? Repository: rG LLVM Github Monorepo CHAN

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

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:5011 +!ValType->isFloatingType()) { + Diag(ExprRange.getBegin(), diag::err_atomic_op_needs_atomic_int_ptr_or_fp) << IsC11 << Ptr->getType() << Ptr->getSourceRange(); -

[PATCH] D98783: [CUDA][HIP] Remove unused addr space casts

rjmccall added a comment. In D98783#2642269 , @tra wrote: > We may want to add someone with more expertise with the IR as a reviewer. I'd > like an educated opinion on whether the invisible dangling IR is something > that needs fixing in general or if it

[PATCH] D99037: [Matrix] Implement explicit type conversions for matrix types

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaCast.cpp:2236 +} + } + The expected semantics for this conversion are not the ordinary bitwise-reinterpretation semantics of `reinterpret_cast`, so this really should not be allowed as a `reint

[PATCH] D99037: [Matrix] Implement explicit type conversions for matrix types

rjmccall added a comment. In D99037#2659290 , @SaurabhJha wrote: > Hey @fhahn @rjmccall , > > Thank you so much for your reviews. Apart from the rest of your comments, > here are the two principle things I am going to do next: > > 1. Replace the `reinterp

[PATCH] D99037: [Matrix] Implement explicit type conversions for matrix types

rjmccall added a comment. In D99037#2662414 , @fhahn wrote: > In D99037#2659477 , @SaurabhJha > wrote: > >>> What code do you want to get out of this? Are there e.g. vectorized >>> float->double conversions we ca

[PATCH] D99037: [Matrix] Implement explicit type conversions for matrix types

rjmccall added inline comments. Comment at: clang/test/CodeGen/matrix-cast.c:15 + // CHECK: [[C:%.*]] = load <25 x i8>, <25 x i8>* {{.*}}, align 1 + // CHECK-NEXT: [[CONV:%.*]] = zext <25 x i8> [[C]] to <25 x i32> + // CHECK-NEXT: store <25 x i32> [[CONV]], <25 x i32>*

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

rjmccall added a reviewer: varungandhi-apple. rjmccall added a comment. CC'ing Varun Gandhi. Is `musttail` actually supported generically on all LLVM backends, or does this need a target restriction? You should structure this code so it's easy to add exceptions for certain calling conventions

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

rjmccall added a comment. In D99517#2667088 , @rsmith wrote: > In D99517#2667025 , @rjmccall wrote: > >> You should structure this code so it's easy to add exceptions for certain >> calling conventions that can sup

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

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:5011 +!ValType->isFloatingType()) { + Diag(ExprRange.getBegin(), diag::err_atomic_op_needs_atomic_int_ptr_or_fp) << IsC11 << Ptr->getType() << Ptr->getSourceRange(); -

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

rjmccall added a comment. Sorry, it's just been a busy month for me. In D89490#2539950 , @aguinet wrote: > In D89490#2516255 , @rjmccall wrote: > >> In D89490#2514695 , @agu

[PATCH] D99893: [WIP] Replace std::forward & std::move by cast expressions during Sema

rjmccall added a comment. I think a pattern-match in IRGen is probably a much more reasonable alternative if the goal is primarily to avoid the code-generation overheads at -O0 / optimization costs at -O. You'd still have template instantiation overheads; I don't know how significant those rea

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

rjmccall added a comment. Alright, mostly looks good. Comment at: clang/lib/Sema/SemaChecking.cpp:5011 +!ValType->isFloatingType()) { + Diag(ExprRange.getBegin(), diag::err_atomic_op_needs_atomic_int_ptr_or_fp) << IsC11 << Ptr->getType() << Ptr->getSour

[PATCH] D99791: [CGCall] Annotate pointer argument with alignment

rjmccall added a comment. The last major conversation we had about this was this RFC I sent out about five years ago: https://lists.llvm.org/pipermail/llvm-dev/2016-January/094012.html In that RFC, I specifically argued that we should not do this, and that it should only be considered undefi

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

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

[PATCH] D99898: [clang, test] Fix use of undef FileCheck var

rjmccall added a comment. `NUW_RN` isn't a good name for the variable in this alternative since it's not `readnone` anymore. With an appropriate rename, LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99898/new/ https://reviews.llvm.org/D9989

[PATCH] D99893: [WIP] Replace std::forward & std::move by cast expressions during Sema

rjmccall added a comment. Our builtin logic is capable of recognizing user declarations as builtins without any sort of explicit annotation in source. That's how we recognize C library builtins, for example. As Richard points out, that logic isn't really set up to do the right thing for names

[PATCH] D99898: [clang, test] Fix use of undef FileCheck var

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

[PATCH] D99791: [CGCall] Annotate pointer argument with alignment

rjmccall added a comment. In D99791#2672528 , @lebedev.ri wrote: > @rjmccall thank you for taking a look! > > In D99791#2670333 , @rjmccall wrote: > >> The last major conversation we had about this was this RFC I se

[PATCH] D100037: [clang][UBSan] Passing underaligned pointer as a function argument is undefined behaviour

rjmccall added a comment. > Yes. We now have a motivational reason to do so, it wouldn't be "just expose > the UB for the sake of miscompiling the program". Nobody thinks you're proposing this just for the sake of miscompiling. We understand that there are optimization benefits to having this

[PATCH] D60456: [RISCV] Hard float ABI support

rjmccall added a comment. Herald added subscribers: vkmr, frasercrmck, evandro, luismarques, sameer.abuasal, s.egerton. Your use of CoerceAndExpand seems fine; thanks for pinging me on it Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60456/new/ https://reviews.ll

[PATCH] D100225: [Clang][AArch64] Coerce integer return values through an undef vector

rjmccall added a comment. Why does the ABI "require" this to be returned as an i64 if some of the bits are undefined? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100225/new/ https://reviews.llvm.org/D100225 _

[PATCH] D100225: [Clang][AArch64] Coerce integer return values through an undef vector

rjmccall added a comment. AArch64 only has 64-bit integer registers, so of course the algorithm is specified that way. LLVM could nonetheless choose to return an `i32`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100225/new/ https://reviews.llv

[PATCH] D100225: [Clang][AArch64] Coerce integer return values through an undef vector

rjmccall added a reviewer: t.p.northover. rjmccall added a comment. You should probably talk it over with AArch64 backend folks, but yes, abstractly it seems reasonable. CC'ing Tim Northover. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100225/ne

[PATCH] D100282: [Coroutines] Move CoroEarly pass to before AlwaysInliner

rjmccall added a comment. Why does this pass even exist? We should just expect the frontend to set the attribute. It's not like frontends don't have to otherwise know that they're emitting a coroutine; a ton of things about the expected entire IR pattern are different. Repository: rG LLVM

[PATCH] D100282: [Coroutines] Move CoroEarly pass to before AlwaysInliner

rjmccall added a comment. In D100282#2682269 , @lxfind wrote: > In D100282#2682251 , @rjmccall > wrote: > >> Why does this pass even exist? We should just expect the frontend to set >> the attribute. It's not

[PATCH] D100282: [Coroutines] Move CoroEarly pass to before AlwaysInliner

rjmccall added a comment. We don't have so many coroutine-emitting frontends that it's unreasonable to modify them all. Swift can make this change; it's not on you to do it. (I also work on the Swift frontend, so don't worry, I'm signing my own teammates up for work here.) Repository: rG

[PATCH] D100282: [Coroutines] Set presplit attribute in Clang instead of CoroEarly pass

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

[PATCH] D100282: [Coroutines] Set presplit attribute in Clang instead of CoroEarly pass

rjmccall added a comment. That something is an (unlowered) coroutine is an important semantic property of the function that should be represented more explicitly in IR than just whether it contains a call to an intrinsic in the `llvm.coro.id` family. Coroutines have somewhat different structur

[PATCH] D100591: [Clang][AArch64] Disable rounding of return values for AArch64

rjmccall added a comment. Hmm. I think the right thing to do here is to recognize generally that we're emitting a mandatory tail call, and so suppress *all* the normal transformations on the return value. The conditions on mandatory tail calls should make that possible, and it seems like it w

[PATCH] D100591: [Clang][AArch64] Disable rounding of return values for AArch64

rjmccall added a comment. In D100591#2692978 , @asavonic wrote: > In D100591#2692599 , @rjmccall > wrote: > >> I think the right thing to do here is to recognize generally that we're >> emitting a mandatory tail

[PATCH] D99790: [CGCall] Annotate `this` argument with alignment

rjmccall added a comment. I agree. The arrangement logic is assuming that the exact pointer type doesn't matter because it's all just a pointer in the end, but of course that breaks down when we start inferring attributes. Pointer authentication also really wants to know the original declarati

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rjmccall added a comment. Sorry for the delay. That seems like a reasonable summary of our discussion. Let me try to lay out the right architecture for this as I see it. Conceptually, we can split the translation unit into a sequence of partial translation units (PTUs). Every declaration wil

[PATCH] D96033: [clang-repl] Land initial infrastructure for incremental parsing

rjmccall added a comment. In D96033#2695622 , @v.g.vassilev wrote: > Would it make sense to have each `Decl` point to its owning PTU, similarly to > what we do for the owning module (`Decl::getOwningModule`)? I think that's the interface we want, but ac

[PATCH] D100282: [Coroutines] Set presplit attribute in Clang instead of CoroEarly pass

rjmccall added a comment. MLIR is an in-tree project that can be updated. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100282/new/ https://reviews.llvm.org/D100282 ___ cfe-commits mailing list cfe-commi

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

rjmccall added a comment. Please addd tests, including tests that we suppress this assumption under e.g. `-fno-builtin-malloc` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100879/new/ https://reviews.llvm.org/D100879 ___ cfe-commits mailing

[PATCH] D94092: [Clang] Remove unnecessary Attr.isArgIdent checks.

rjmccall added a comment. Without bothering to look it up, I would guess that the attribute-parsing code used to generically handle the ambiguity between identifier expressions and identifier attribute arguments by just always parsing simple identifiers as identifier arguments, making it Sema's

[PATCH] D92409: [AST][NFC] Silence GCC warning about multiline comments

rjmccall added a comment. People can't actually just copy/paste from the comment anyway because of the comment characters on the line, and I don't think anyone will misunderstand the example if the backslash is missing. It's silly that GCC has a problem with this, but since it does, let's just

[PATCH] D88298: Fix MaterializeTemporaryExpr's type when its an incomplete array.

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

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

rjmccall added a comment. I do feel like this is a terrible idea, sorry. It's a *very* niche use case to be dedicating a new compiler feature to, and it's a feature that demands a lot from the internal compiler architecture in ways that other features don't. If you really need to build code wi

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

rjmccall added a comment. Are you committed to the name `__ibm128`? I think a name that says something about floating point would be better than just a company name and a number. "double double" is the common name for this format pretty much everywhere, and while I certainly would *not* sugge

[PATCH] D94092: [Clang] Remove unnecessary Attr.isArgIdent checks.

rjmccall added a comment. In D94092#2481857 , @aaron.ballman wrote: > In D94092#2481276 , @rjmccall wrote: > >> Without bothering to look it up, I would guess that the attribute-parsing >> code used to generically

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

rjmccall added a comment. In D93377#2481957 , @hubert.reinterpretcast wrote: > In D93377#2481312 , @rjmccall wrote: > >> Are you committed to the name `__ibm128`? > > Insofar as that's what GCC on Power (for exampl

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

rjmccall added a comment. I'm not calling Wine a niche use-case, I'm calling this feature a niche use-case. The lack of this feature has not blocked Wine from being a successful project. The feature has to stand on its own and be more broadly useful than the momentary convenience of a few dev

[PATCH] D94196: [NFC] Move readAPValue/writeAPValue up the inheritance hierarchy

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

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

rjmccall added a comment. In D89490#2483792 , @aguinet wrote: > In D89490#2482531 , @rjmccall wrote: > >> I'm not calling Wine a niche use-case, I'm calling this feature a niche >> use-case. The lack of this featu

[PATCH] D92409: [AST][NFC] Silence GCC warning about multiline comments

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

[PATCH] D94257: [test] Move coro-retcon-unreachable.ll into llvm/test

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Yeah, that's definitely an LLVM test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94257/new/ https://reviews.llvm.org/D94257

[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

rjmccall added a comment. I'm sorry that it's taking a while to find time to review the implementation; I'll try to get to it this week. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80344/new/ https://reviews.llvm.org/D80344

[PATCH] D94854: [Clang] Fix SwiftCallingConv's aggregate lowering for _Atomic(_Bool).

rjmccall added a comment. Hmm. The way you'd test it on the Clang side would be to just write a test case that passes such an aggregate and tests that it's passed as an `i8`. But I'm not sure it's at all unreasonable to pass this as an `i1` rather than an `i8`; seems like something that Swift

[PATCH] D94854: [Clang] Fix SwiftCallingConv's aggregate lowering for _Atomic(_Bool).

rjmccall added a comment. Clang is trying to avoid using non-byte-multiple integer types for memory accesses in LLVM IR, whereas Swift isn't. If you take your test case and make the field non-atomic, you'll see that: Clang is accessing the field as an `i8` and Swift is accessing it as an `i1`.

[PATCH] D80344: [Windows SEH]: HARDWARE EXCEPTION HANDLING (MSVC -EHa) - Part 1

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCleanup.cpp:1341 + llvm::FunctionCallee SehCppScope = + CGM.CreateRuntimeFunction(FTy, "llvm.seh.scope.begin"); + EmitSehScope(*this, SehCppScope); We generally prefer to get intrinsic functio

[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. How does this new rule work if we find overlapping but non-equal sets of declarations in different subobjects? I'm sure you can get that with `using` declarations. Comment at: clang/include/clang/AST/CXXInheritance.h:77 - CXXBasePath() = default;

[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. (I didn't mean to submit the last comment before I finished the review, sorry) It looks like you've chosen to treat this as a DR that we should apply under all standards. Have you investigated whether it causes compatibility problems? It does look like it'd be painfu

[PATCH] D94977: [CodeGen] Use getCharWidth() more consistently in CGRecordLowering. NFC

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

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

rjmccall added a comment. I don't mind generally abstracting Clang to handle this better. Comment at: clang/lib/CodeGen/CodeGenModule.h:1229 + // Return the LLVM type sized as one character unit. + llvm::Type *getCharSizedType(); + Please just add a CharTy to

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

rjmccall added a reviewer: fhahn. rjmccall added inline comments. Comment at: clang/lib/AST/ItaniumMangle.cpp:4032 + } } break; The old case doesn't seem to be adding the `u8__uuidof` prefix anymore. Your patch description does not say anything ab

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

rjmccall added a comment. I think the plan should be to use this code pattern on all targets, but starting it with arm64 seems fine. Comment at: clang/lib/CodeGen/CGObjC.cpp:2342 +attrs = attrs.addAttribute(CGF.getLLVMContext(), + llvm::Attrib

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

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Patch looks good to me now, thanks. I don't think `always_inline` should strictly be necessary vs. just `static inline`, because in either case the function definition containing the non-

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

rjmccall added a comment. I assume we rebuild an AILE and OVE when we process the initializer and see it's a structured binding? In that case, this should be fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108482/new/ https://reviews.llvm.org

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

rjmccall added a comment. This patch seems to be confused. You're making two changes. In one of them, you're trying to prevent `addrspacecast`s from being interleaved with the sequence of `alloca`s in the function prologue. In the other, you're moving the store emitted by `InitTempAlloca` so

[PATCH] D111286: Add no_instrument_function attribute to Objective C methods as well

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

[PATCH] D111293: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca()

rjmccall added a comment. We should take the opportunity to add a test here for the ObjC case. It should be testable with something like this, where you should see the store inside of the loop instead of once in the prologue: // RUN: %clang_cc1 -triple x86_64-unknown-freebsd -S -emit-llvm -

[PATCH] D111324: [CFE][Codegen] Remove CodeGenFunction::InitTempAlloca()

rjmccall added a comment. Can you just do one patch for all the OpenMP changes, and then put the final removal in a separate patch? Thanks for taking this on. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111324/new/ https://reviews.llvm.org/D111

[PATCH] D111331: [ObjC][ARC] Use operand bundle "clang.arc.attachedcall" on x86-64

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:2351 - // FIXME: Do this when the target isn't aarch64. + llvm::Triple::ArchType Arch = CGF.CGM.getTriple().getArch(); + Please leave a comment saying that we should do this on all targe

[PATCH] D111293: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca()

rjmccall added a comment. In D111293#3049633 , @jdoerfert wrote: >> CodeGenFunction::InitTempAlloca() inits the static alloca within the entry >> block which may *not* necessarily be correct always. > > FWIW, for all uses this was correct. The point of t

[PATCH] D111293: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca()

rjmccall added a comment. Hmm. I decided the ObjC case needed a more complex change and went ahead and committed that fix here: https://github.com/llvm/llvm-project/commit/5ab6ee75994d. That obviates the need for the new test; sorry for the runaround. Please rebase and I'll review. Reposit

[PATCH] D111293: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca()

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Patch LGTM, but please remove the test, it's been folded into a test I added in my patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D1112

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

rjmccall added a comment. I agree. It doesn't have to be a CodeGen test, just anything that directly verifies that we get the right type, since I think those calls can succeed due to promotion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109950

[PATCH] D111324: [CFE][Codegen] Remove CodeGenFunction::InitTempAlloca()

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks. This LGTM when all the patches it depends on are checked in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111324/new/ https://revi

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

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

[PATCH] D111316: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca()

rjmccall added a reviewer: ABataev. rjmccall added a comment. This mostly looks good, but I'd like Alexey Bataev to sign off, since he authored the OpenMP support. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2124 /*Name=*/".bound

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

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

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added inline comments. Comment at: clang/docs/BitIntABI.rst:17 +This is only intended as a generic specification. The actual details for any +particular platform should be codified in that platform's ABI specification. + I think you should recommend that

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. In D108643#3027473 , @erichkeane wrote: > In D108643#3026550 , @rjmccall > wrote: > >> I think it would be better to provide a generic ABI specification that is >> designed to "lower"

[PATCH] D111316: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca()

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

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:392 + /// order immediately after all static allocas. + llvm::BasicBlock::iterator AllocaAddrSpaceInsertPt; + Please call this something like `PostAllocaInsertPt`. Instead of e

[PATCH] D111331: [ObjC][ARC] Use operand bundle "clang.arc.attachedcall" on x86-64

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

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. In D108643#3055244 , @erichkeane wrote: >>> ! In D108643#3054443 , @rjmccall >>> wrote: >>> ! In D108643#3027473 , @erichkeane wrote

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added inline comments. Comment at: clang/docs/BitIntABI.rst:90 +width. e.g., a ``signed _BitField(7)`` whose representation width is ``8`` bits +cannot store values less than ``-64`` or greater than ``63``. + It might be worthwhile to pull these definiti

[PATCH] D108643: Introduce _BitInt, deprecate _ExtInt

rjmccall added a comment. Yes, I think it would be fine to fork off a conversation about what the ABI should be; I didn't meant to completely derail this patch. To be frank, my expectation was that more of this was already settled and documented, so we just needed to add that documentation and

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

rjmccall added a comment. I don't think we currently hold very strongly to that ActOn vs. Build philosophy across all productions, but it does seem like a good idea. I don't really know why there's this much duplication between the code paths. The only semantic distinction I know of is that I

[PATCH] D112081: Define __STDC_NO_THREADS__ when targeting windows-msvc (PR48704)

rjmccall added a comment. The standard answer is that compilers are designed to work with a specific set of system headers. In the Clang-on-Windows case, that's complicated by the fact that many people acquire Clang separately from the rest of the build environment (although Microsoft does dis

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

rjmccall added inline comments. Comment at: clang/test/SemaCXX/PR51855.cpp:71 +// CHECK: call void @_ZN1S4putMEs +// CHECK: call {{(signext )?i16}} @_ZN1S4getMEv rnk wrote: > Please add test coverage for the interesting operators in the code under > edit: OO_Sub

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

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

[PATCH] D112235: [HIP][OpenMP] Fix assertion in deferred diag due to incomplete class definition

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Minor grammar nit with a comment, but otherwise LGTM. Comment at: clang/test/OpenMP/deferred-diags.cpp:40 +// Test deleting object with incomplete class definition does n

[PATCH] D108445: [clang][NFC] GetOrCreateLLVMGlobal takes LangAS

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

[PATCH] D108458: [clang][CodeGen] Add default constructor for Address. NFC.

rjmccall added a comment. You can still use a type without a default constructor in a `DenseMap`; you just have to use `insert` instead of `dict[key] = ...`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108458/new/ https://reviews.llvm.org/D10845

[PATCH] D108450: [clang][CodeGen] GetDefaultAlignTempAlloca uses preferred alignment

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Sure, I guess there's no harm in this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108450/new/ https://reviews.llvm.org/D108450 _

[PATCH] D108360: [clang][NFC] Remove dead code

rjmccall added a reviewer: Anastasia. rjmccall added a comment. Hi, Andy. This patch is obviously okay, although it makes me wonder if it was introduced by a patch collision or something similar that needs closer attention. CC'ing Anastasia. I'm not sure that I agree with your overall plan, t

[PATCH] D108407: [CodeGen][WIP] Avoid generating Record layouts for pointee types

rjmccall added a comment. I have no problem with breaking LLVM analyses that rely on record types being filled in when they don't need to be. I've been consistently telling people for years that they shouldn't be relying on IR types for things like that. I would stick with the frontend termino

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

rjmccall added a reviewer: jfb. rjmccall added a comment. + JF, who knows something about Web Assembly, or can at least drag in the right people In D108464#2959591 , @wingo wrote: > In D108464#2957276 , @wingo w

[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: > > Okay, so first off, the comme

<    27   28   29   30   31   32   33   >