[PATCH] D138137: [CodeGen][ARM] Fix ARMABIInfo::EmitVAAarg crash with empty record type variadic arg

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

[PATCH] D137719: [clang] Missed rounding mode use in constant evaluation

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

[PATCH] D136176: Implement support for option 'fexcess-precision'.

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.def:320 BENIGN_ENUM_LANGOPT(FPEvalMethod, FPEvalMethodKind, 2, FEM_UnsetOnCommandLine, "FP type used for floating point arithmetic") +BENIGN_ENUM_LANGOPT(FPPrecisionMode, FPExcessPrecisionModeKind

[PATCH] D126818: Itanium ABI: Implement mangling for constrained friends

rjmccall added a comment. I'm too often slow to actually apply edits to the ABI document. There's been plenty of time for feedback on this one; go ahead and act like it's accepted. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126818/new/ https://reviews.llvm.org/D126818

[PATCH] D142584: [CodeGen] Add a boolean flag to `Address::getPointer` and `Lvalue::getPointer` that indicates whether the pointer is known not to be null

rjmccall added a comment. In D142584#4081345 , @efriedma wrote: > I'm having a little trouble following what the meaning of an "Address" is > with this patch. The `Pointer` member refers to an encoded value, and > calling getPointer() emits IR to decod

[PATCH] D129008: [Clang][OpenMP] Fix the issue that globalization doesn't work with byval struct function argument

rjmccall added a comment. Right. C structs can require more than just `memcpy` to copy in several different situations (all involving language extensions), but it doesn't require Sema to be involved: it doesn't introduce uses of user declarations, and the compiler can figure out the work it nee

[PATCH] D142584: [CodeGen] Add a boolean flag to `Address::getPointer` and `Lvalue::getPointer` that indicates whether the pointer is known not to be null

rjmccall added inline comments. Comment at: clang/lib/CodeGen/Address.h:97 + llvm::Value * + getPointer(KnownNonNull_t KnownNonNull = KnownNonNull_t::False) const { assert(isValid()); ahatanak wrote: > rjmccall wrote: > > Apologies if this rehashes a conve

[PATCH] D142584: [CodeGen] Add a boolean flag to `Address::getPointer` and `Lvalue::getPointer` that indicates whether the pointer is known not to be null

rjmccall added a comment. This seems reasonable to me. Eli, are your questions answered? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142584/new/ https://reviews.llvm.org/D142584 ___ cfe-commits mailin

[PATCH] D79378: PR34581: Don't remove an 'if (p)' guarding a call to 'operator delete(p)' under -Oz.

rjmccall added a comment. In D79378#4101613 , @shiva0217 wrote: > Hi, > > I have a question for the delete function call sinking in -Oz. > > https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3690.pdf. > According to 3.7.4.2/3 > ` The value of the f

[PATCH] D79378: PR34581: Don't remove an 'if (p)' guarding a call to 'operator delete(p)' under -Oz.

rjmccall added a comment. In D79378#4101935 , @shiva0217 wrote: > In D79378#4101829 , @rjmccall wrote: > >> In D79378#4101613 , @shiva0217 >> wrote: >> >>> Hi, >>> >>> I hav

[PATCH] D142948: [OpenCL] Disable vector to scalar types coercion for OpenCL

rjmccall added a comment. Does the x86 backend lower these small vectors to the same ABI? If so, I think we could just teach Clang unconditionally that it doesn't need this coercion on x86 targets. If not, this is an ABI break; are you sure that's okay? Repository: rG LLVM Github Monorepo

[PATCH] D141381: [codegen] Store address of indirect arguments on the stack

rjmccall added a comment. That's about what I would expect. One or two extra instructions per argument that are trivially removed in debug builds. I don't really see a need to post about this on Discourse, but it might be worth a release note. Repository: rG LLVM Github Monorepo CHANGES S

[PATCH] D142001: [clang] Use FP options from AST for emitting code for casts

rjmccall added a comment. Overall looks reasonable. One question about testing. Comment at: clang/test/CodeGen/X86/avx512dq-builtins-constrained.c:7 -// FIXME: Every instance of "fpexcept.maytrap" is wrong. +// Any cases of "fpexcept.maytrap" in this test are clang bugs. + -

[PATCH] D142001: [clang] Use FP options from AST for emitting code for casts

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

[PATCH] D141765: [FPEnv] Fix complex operations in strictfp mode

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

[PATCH] D127545: Remove Itanium EH ABI workaround to allow modifying a pointer caught by-reference.

rjmccall added a comment. Should we just diagnose this and report an error that catching pointers by non-const reference is unsupported on Itanium? We don't support it properly for class pointers anyway. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

rjmccall added a comment. The cleanups arise from expressions in the constraints, right? There are a number of places where `ExprWithCleanups` does not mean the cleanups are independently nested within that expression; it's notably not how it works ever for something as basic as a variable ini

[PATCH] D127471: [Coroutines] Convert coroutine.presplit to enum attr

rjmccall accepted this revision. rjmccall added a comment. LGTM. Don't worry about Swift, we'll handle it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127471/new/ https://reviews.llvm.org/D127471 ___

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

rjmccall added a comment. Minor suggestion for a cleanup, then LGTM. Comment at: clang/lib/Sema/SemaOverload.cpp:9048 +ParamTypes[0] = S.Context.getVolatileType(ParamTypes[0]); + ParamTypes[0] = S.Context.getLValueReferenceType(ParamTypes[0]); +

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

rjmccall added a comment. Yeah, I think the appropriate thing is to just treat the entire asm statement like it's a single full-expression. As always, that implies an annoying lifetime for blocks and C compound literals. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:2291 + // statement. + CodeGenFunction::RunCleanupsScope Cleanups(*this); + Do we need to do anything special when entering the full expressions to make sure that enclosing-block features

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

rjmccall added a comment. In D113107#3585672 , @zahiraam wrote: > @rjmccall > The comma && ternary cases for EmitPromoted, I am not sure what needs to > happen there? Basically just the same thing that the normal paths do, except you recurse with `Emi

[PATCH] D125936: [Sema] Relax an assertion in BuildStmtExpr

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

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:896 + +ComplexPairTy ComplexExprEmitter::EmitPromoted(const Expr *E) { + if (auto *BinOp = dyn_cast(E->IgnoreParens())) { zahiraam wrote: > rjmccall wrote: > > `EmitPromoted` should

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:896 + +ComplexPairTy ComplexExprEmitter::EmitPromoted(const Expr *E) { + if (auto *BinOp = dyn_cast(E->IgnoreParens())) { pengfei wrote: > rjmccall wrote: > > zahiraam wrote: > > > r

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

rjmccall added a comment. Supporting the lowering in the backend is sensible in order to support `-fexcess-precision=16`, because I agree that the most reasonable IR output in that configuration is to simply generate `half` operations. But under `-fexcess-precision=32`, I do not want the front

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

rjmccall added a comment. In D113107#3602478 , @zahiraam wrote: > Do we want to add an -fexcess-precision option? Well, generally we try to offer the same options GCC does. However, it looks like GCC's `-fexcess-precision` does not line up with the opt

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

rjmccall added a comment. I think on balance the right thing to do is probably to add an alternative to `-fexcess-precision`, like `-fexcess-precision=none`. We can default to `-fexcess-precision=standard` and treat `-fexcess-precision=fast` as an alias for `standard` for now. =

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

rjmccall added a comment. In D113107#3606094 , @zahiraam wrote: > In D113107#3605797 , @rjmccall > wrote: > >> I think on balance the right thing to do is probably to add an alternative >> to `-fexcess-precision

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

rjmccall added a comment. In D113107#3606107 , @zahiraam wrote: > For this test case: > _Float16 add_half_cr(_Float16 a, _Float16 b) { > > return a > b ? a : b; > > } > > are we expecting the phi node to be > > cond.true: > > %2 = load float , ptr >

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprComplex.cpp:934 + return ComplexExprEmitter(*this).EmitPromoted(E, DstTy); +} + rjmccall wrote: > `EmitPromotedComplexExpr` should look like `EmitPromotedScalarExpr`, i.e. it > should start wit

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3075 +ConvertType(DstType), ScalarConversionOpts()); +} + ...I wrote out what this function should look like, and Phabricator just threw it away. Let me try t

[PATCH] D128501: [CodeGen] Make uninitialized Lvalue bit-field stores poison compatible

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

[PATCH] D128482: [clang codegen] Add dso_local/hidden/etc. markings to VTT declarations

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

[PATCH] D123952: [FPEnv] Allow CompoundStmt to keep FP options

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:452 + : getCurCompoundScope().FPFeatures; + FPOptionsOverride FPDiff = getCurFPFeatures().diffWith(FPO); + How cheap is this? Is this something it would be worthwhile t

[PATCH] D128571: [X86] Support `_Float16` on SSE2 and up

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:746 * SPIR -* X86 (Only available under feature AVX512-FP16) +* X86 (Enabled with feature SSE2 and up) Could you take the wording I suggested from the other patch? You'll need to d

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

rjmccall added a comment. In D125919#3554078 , @delcypher wrote: > @aaron.ballman Hey I just saw this change and had questions about it. For > others looking I think the resolution to DR423 is in > https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1863.

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

rjmccall added a comment. In D125919#3554195 , @aaron.ballman wrote: > However, I reverted the changes in this patch in > c745f2ce6c03bc6d1e59cac69cc15923d4400191 > as I > don't thi

[PATCH] D126781: [CodeGen] Correctly handle weak symbols in the codegen

rjmccall added a comment. You haven't really described the issue you're having. The code looks like it's maintaining a map from symbol names to the `GlobalDecl` from which it'll be emitted; can you explain what Cling wants this for? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

rjmccall added a comment. Ah, yeah, that seems unrelated (and surprising). You think we're *not* supposed to drop `_Atomic` in a return type per C2x? That seems surprising; I thought it was generally treated as a qualifier and would expect that we'd be supposed to drop it. Repository: rG L

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

rjmccall added a comment. Hmm. I'm sorry, seeing the impact of this, I think it's not great to have this `PromotionTy` field; it's too hard to get the invariants right. Let's do this instead: 1. Add an `EmitPromoted` method to `ScalarExprEmitter`. The general rule for this method is that it

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

rjmccall added a comment. Ah, true, the standard doesn't describe it as a normal qualifier. From the DR, it sounds like the committee certainly expected that this reasoning would also apply to `_Atomic`, even if that's not quite what they've written, but that the DR submitter seems to not want

[PATCH] D126781: [CodeGen] Correctly handle weak symbols in the codegen

rjmccall added a comment. Okay, I understand. So, first off, I wouldn't really call that a "weak" symbol rather than, say, a lazily-emitted symbol; "weak" already has plenty of different senses, and we should try to avoid coining more. Also, your patch description makes it sound like there's

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

rjmccall added a comment. Are you now arguing that C was wrong to drop qualifiers in return types and so we shouldn't implement that rule, or...? Because your argument is much broader than just `_Atomic`. I don't understand the analogy you're making to `_Complex` at all. Values of type `_Com

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

rjmccall added a comment. In D113107#3556886 , @zahiraam wrote: > In D113107#3554538 , @rjmccall > wrote: > >> Hmm. I'm sorry, seeing the impact of this, I think it's not great to have >> this `PromotionTy` fie

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

rjmccall added a comment. > I disagree with the assessment that _Atomic behaves exactly like a qualifier. > It *should* (IMHO), but it doesn't. C allows the size and alignment of an > atomic type be *different* from its unqualified version, to allow for > embedded locks and such. Because the si

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

rjmccall added a comment. I completely lost track of this, but if you'd like to rebase it, I can try to make time. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62574/new/ https://reviews.llvm.org/D62574 __

[PATCH] D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.h:1482 + + struct KeepLazyEmiitedSymRAII { +std::unique_ptr OldBuilder; I think a RAII object is an odd way to express this API; there's not really a natural reason you would scope

[PATCH] D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.h:1482 + + struct KeepLazyEmiitedSymRAII { +std::unique_ptr OldBuilder; junaire wrote: > rjmccall wrote: > > I think a RAII object is an odd way to express this API; there's not real

[PATCH] D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.h:1482 + + struct KeepLazyEmiitedSymRAII { +std::unique_ptr OldBuilder; rjmccall wrote: > junaire wrote: > > rjmccall wrote: > > > I think a RAII object is an odd way to express this

[PATCH] D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder

rjmccall added a comment. In D126781#3565173 , @junaire wrote: > I believe that `OldBuilder` is always valid in the normal cases, so > remove the if condition and replace it with an assertion. Is `StartModule` not called for the first time we start emitt

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

rjmccall added a comment. This seems like the right idea to me, yeah. It doesn't look like the patch handles `volatile _Atomic` correctly, though. I know we do a lot of candidate prefiltering, and that that can be difficult because of uesr-defined conversions. How do those things interact wit

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

rjmccall added a comment. In D125919#3569928 , @aaron.ballman wrote: > In D125919#3560523 , @aaron.ballman > wrote: > >> All that said, I think you can see why I'm hoping to get an answer from WG14 >> as to wha

[PATCH] D126781: [CodeGen] Keep track info of lazy-emitted symbols in ModuleBuilder

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

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:8978 + } } } Can we do this in a way that's a little more general if we want to include other qualifiers in this combinatoric explosion in the future? I think y

[PATCH] D125919: Drop qualifiers from return types in C (DR423)

rjmccall added a comment. Yeah, that makes sense. Thanks for following that up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125919/new/ https://reviews.llvm.org/D125919 ___ cfe-commits mailing list cf

[PATCH] D125349: [Sema] Fix crash for C11 atomic in gnu++ mode

rjmccall added inline comments. Comment at: clang/include/clang/AST/Type.h:277 +return qs; + } Could you go ahead and add this for `const` and `restrict` as well? I think that will give us the full set. Comment at: clang/lib/Sema/SemaO

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

rjmccall added a comment. > That's better than the status quo (no lifetime markers at all) but still > suboptimal, and I don't think it will solve the particular case I care about > (a particular Linux kernel driver written in C which is passing many > aggregates by value). Ah, all in the same

[PATCH] D37192: [clang-format] Add support for generic Obj-C categories

rjmccall added a comment. Is that really the way we want to recommend formatting that, with the generics clause separated from the class name but not separated from the category clause? I'd expect the opposite: write the generics clause with no separation after the class name but with separati

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rjmccall added a comment. Hey, Richard. It looks like your new tests are failing on Apple's buildbots, I'm guessing because the default ABI compatibility mode is older. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147655/new/ https://reviews.llv

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rjmccall added a comment. I've been informed that Apple's change to the default ABI compatibility mode is in our private fork, so this is not your problem; sorry for the noise. I'm not sure why we decided to do that privately; I'll see if we can fix that. Repository: rG LLVM Github Monorepo

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rjmccall added a comment. Yeah, the more I think about this, the more I think that while (1) Apple should upstream its use of an older default, regardless (2) the existence of any targets at all with an older default means that tests like this always need to be using `-fclang-abi-compat=latest`

[PATCH] D147655: Implement mangling rules for C++20 concepts and requires-expressions.

rjmccall added a comment. In D147655#4650964 , @probinson wrote: > Hi @rsmith, > >> these two different templates would have the same mangling: > > template T returnit() {return I;}; > template T returnit() { return I; } > > I tried compiling `long

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:361 + if (!GS->isAsmGoto()) +break; // Remember both what scope a goto is in as well as the fact that we have nickdesaulniers wrote: > rjmccall wrote: > > You can pul

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added a comment. I do like the look of https://reviews.llvm.org/D155522, yeah, since we have so many of these that aren't top-level in a case anyway. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155342/new/ https://reviews.llvm.org/D1553

[PATCH] D155525: WIP

rjmccall added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:362 // it. This makes the second scan not have to walk the AST again. +RecordJumpScope: LabelAndGotoScopes[S] = ParentScope; I would indent this at the same level as the `ca

[PATCH] D155506: [clang][JumpDiagnostics] use StmtClass rather than dyn_cast chain NFC

rjmccall added a comment. In D155506#4508176 , @gribozavr2 wrote: > I'm not sure it is actually an anti-pattern. `dyn_cast` will transparently > handle subclasses, should any be added in the future, but a switch wouldn't. Yeah, I don't think we have a

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { I think it would be good to leave a comment here like this: > We know the possible destination

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added inline comments. Comment at: clang/lib/Sema/JumpDiagnostics.cpp:658 +if (auto *G = dyn_cast(Jump)) { + for (AddrLabelExpr *L : G->labels()) { nickdesaulniers wrote: > rjmccall wrote: > > I think it would be good to leave a comment here l

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added a comment. > We need to check the scopes like we would for direct goto, because we don't > want to bypass non-trivial destructors. I think this is the basic misunderstanding here. Direct `goto` is allowed to jump out of scopes; it just runs destructors on the way. It is only a

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

rjmccall added a comment. Yes, this diagnostic implements a core semantic rule and must be emitted. There are some situations where Clang will actually generate malformed IR (violating dominance rules) if you fail to diagnose jump violations correctly. The IR you get when there's a jump over

[PATCH] D154503: [Sema] Fix handling of functions that hide classes

rjmccall added a comment. I'm suggesting: llvm::BitVector declsToRemove; while (I < N) { if (shouldHideTag()) declsToRemove.add(I); if (notUnique()) declsToRemove.add(I); } Decls.removeAll(declsToRemove) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154503/new/ ht

[PATCH] D155342: [clang][JumpDiagnostics] ignore non-asm goto target scopes

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

[PATCH] D155759: [Clang][CodeGen] Follow-up for `vtable`, `typeinfo` et al. are globals

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

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

rjmccall added a comment. LGTM, except, should we have a way to turn this optimization off specifically? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154658/new/ https://reviews.llvm.org/D154658 ___ cfe

[PATCH] D153092: [Clang][CodeGen]`vtable`, `typeinfo` et al. are globals

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:693 return Int32Ty; - return Int8PtrTy; + return GlobalsInt8PtrTy; } bjope wrote: > I noticed that we have some old fixes downstream that conflicts with the > changes you've made

[PATCH] D154503: [Sema] Fix handling of functions that hide classes

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thank you, this looks great, and I really appreciate that you found a way to make it work with just the single loop (in the typical case). CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs passed as function arguments

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

[PATCH] D154658: Optimize emission of `dynamic_cast` to final classes.

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

[PATCH] D155396: [Sema][ObjC] Invalidate BlockDecl with invalid return expr & its parent BlockExpr

rjmccall added a comment. Dropping the entire block from the AST on error would be unfortunate for some kinds of tooling, but as long as we maintain it with a `RecoveryExpr`, that seems like a fine approach to me. As a general rule, I think we should be tracking basically everything on the `Bl

[PATCH] D154774: [Sema] Respect instantiated-from template's VisibilityAttr for two implicit/explicit instantiation cases

rjmccall added inline comments. Comment at: clang/lib/AST/Decl.cpp:380 +->getTemplatedDecl() +->hasAttr(); } Okay, this change seems wrong. A visibility attribute on

[PATCH] D156277: [Parser][ObjC] Stop parsing on eof

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseObjc.cpp:749 + if (!Tok.is(tok::eof)) +ConsumeToken(); break; aaron.ballman wrote: > danix800 wrote: > > tbaeder wrote: > > > Why is there a `ConsumeToken()` call at all here? Th

[PATCH] D156277: [Parser][ObjC] Stop parsing on eof

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseObjc.cpp:749 + if (!Tok.is(tok::eof)) +ConsumeToken(); break; aaron.ballman wrote: > rjmccall wrote: > > aaron.ballman wrote: > > > aaron.ballman wrote: > > > > danix800 wrote: >

[PATCH] D156277: [Parser][ObjC] Stop parsing on eof

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseObjc.cpp:749 + if (!Tok.is(tok::eof)) +ConsumeToken(); break; danix800 wrote: > rjmccall wrote: > > aaron.ballman wrote: > > > rjmccall wrote: > > > > aaron.ballman wrote: > > >

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:5496 + // execution of the await_suspend. To achieve this, we need to prevent the + // await_suspend get inlined before CoroSplit pass. + // Suggestion: > The `await_suspend` call perfor

[PATCH] D157452: [RFC][Clang][Codegen] `std::type_info` needs special care with explicit address spaces

rjmccall added a comment. The path of least resistance here is that IRGen should just insert conversions from the global AS to the default AS as part of evaluating `typeid`. I haven't looked at it closely, but that seems to be what this patch is doing. However, `std::type_info` is an interesti

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

rjmccall added inline comments. Comment at: clang/docs/ReleaseNotes.rst:143 + after leaking the coroutine handle in the await_suspend may be converted to + unconditional access incorrectly. + (`#56301 `_) Sugg

[PATCH] D156897: [CodeGen] Clean up access to EmittedDeferredDecls, NFCI.

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

[PATCH] D156537: [CodeGen] Keep track of eagerly emitted globals

rjmccall added a comment. If we're avoiding the expense here when we're not emitting incremental extensions, this seems fine to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156537/new/ https://reviews.llvm.org/D156537 ___

[PATCH] D157379: [CodeGen] Restrict addEmittedDeferredDecl to incremental extensions

rjmccall accepted this revision. rjmccall added a comment. Thank you, this seems reasonable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157379/new/ https://reviews.llvm.org/D157379 ___ cfe-commits mai

[PATCH] D158242: [Clang][Attribute] Introduce linkage attribute to specify the exact LLVM linkage type for functions or variables

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. I understand that you're adding this for your project's internal purposes, but it's a permanent language feature that I don't think we want. Repository: rG LLVM Github Monorep

[PATCH] D157833: [C++20] [Coroutines] Mark await_suspend as noinline if the awaiter is not empty

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

[PATCH] D156539: [Clang][CodeGen] `__builtin_alloca`s should care about address spaces too

rjmccall added a comment. We should probably write this code to work properly in case we add a target that makes `__builtin_alloca` return a pointer in the private address space. Could you recover the target AS from the type of the expression instead of assuming `LangAS::Default`? Repository

[PATCH] D156277: [Parser][ObjC] Fix parser crash on nested top-level block with better recovery path

rjmccall added inline comments. Comment at: clang/docs/ReleaseNotes.rst:125 + out early in case next top-level block occurs as if the previous one is + properly finished. + `Issue 64065 `_ "Fixed a crash when

[PATCH] D156539: [Clang][CodeGen] `__builtin_alloca`s should care about address spaces too

rjmccall added a comment. In D156539#4546834 , @AlexVlx wrote: > In D156539#4542836 , @rjmccall > wrote: > >> We should probably write this code to work properly in case we add a target >> that makes `__builtin_

[PATCH] D156537: [CodeGen] Keep track of eagerly emitted globals

rjmccall added a comment. I would like to avoid the overhead of this when we're not emitting for a REPL. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156537/new/ https://reviews.llvm.org/D156537 ___ cfe

[PATCH] D156539: [Clang][CodeGen] `__builtin_alloca`s should care about address spaces too

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3542 +LangAS AAS = getASTAllocaAddressSpace(); +LangAS EAS = E->getType().getAddressSpace(); +if (AAS != EAS) { `E->getType().getAddressSpace()` is actually the top-level qual

[PATCH] D156539: [Clang][CodeGen] `__builtin_alloca`s should care about address spaces too

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

[PATCH] D156277: [Parser][ObjC] Fix parser crash on nested top-level block with better recovery path

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Small wording suggestion for the comment, but feel free to commit with that change. Comment at: clang/lib/Parse/ParseObjc.cpp:742 -// Eat the identifier. +

[PATCH] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

rjmccall added a comment. Under [expr.call]p6 (https://eel.is/c++draft/expr.call#6), it is implementation-defined whether the lifetime of a parameter ends at the end of the containing full expression or at the end of the function. If the implementation chooses the latter, a reference to the pa

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