[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] 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. 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. 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] 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] 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] D74094: Reapply: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

rjmccall added a comment. In D74094#4646975 , @xbolva00 wrote: >>> So we can start by giving these objects full-expression lifetime, chase >>> down any program bugs that that uncovers (which will all *unquestionably* >>> be program bugs under the standar

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

rjmccall added a comment. In D74094#4646297 , @tellenbach wrote: > No real comment on the issue itself but on the example as a former Eigen > maintainer (sorry for the noise if that's all obvious for you): > > auto round (Tensor m) { > return (m +

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

rjmccall added a comment. > Fuck me for trying to help, right? If x-values are part of the "basics" of > parameter passing, I'm afraid to ask about the more complicated cases. I can see how my response was somewhat aggressive, and I regret that and apologize. I imagine you're probably approach

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

rjmccall added a comment. In D74094#4643558 , @nickdesaulniers wrote: > In D74094#4643495 , @rjmccall wrote: > >> I can't easily tell you because the "standalone example" involves a million >> templates that I don

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

rjmccall added a comment. I can't easily tell you because the "standalone example" involves a million templates that I don't know anything about, and nobody seems to have done the analysis to identify the specific source of the miscompile. What I can tell you is that there is an enormous semant

[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

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

rjmccall added a comment. Parameter objects are not temporaries and have their own lifetime rules, so there's nothing wrong with this idea in principle. This seems to just be a bug, probably that we're doing a type check on `E->getType()` without considering whether E might be a gl-value. We

[PATCH] D158695: [clang] Fix missing contract flag in sqrt intrinsic

rjmccall added a comment. Code change LGTM; I'll let you hash out the test feedback. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158695/new/ https://reviews.llvm.org/D158695 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://li

[PATCH] D158695: [clang] Fix missing contract flag in sqrt intrinsic

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:501 if (CGF.Builder.getIsFPConstrained()) { CodeGenFunction::CGFPOptionsRAII FPOptsRAII(CGF, E); Function *F = CGF.CGM.getIntrinsic(ConstrainedIntrinsicID, Src0->getType());

[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] 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] 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] 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] 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] 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] 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/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] 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] 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] 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] 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 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] 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. 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] 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] 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; aaron.ballman wrote: > danix800 wrote: > > tbaeder wrote: > > > Why is there a `ConsumeToken()` call at all here? Th

[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] 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] 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] 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] 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] 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] 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] 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] 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] 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 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] 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 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 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] 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] 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] 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] 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. Thanks, that's way better. Just a couple minor requests. Comment at: clang/docs/ReleaseNotes.rst:629 +- Fixed false positive error diagnostic observed from mixing ``asm goto`` with + ``__attribute__((cleanup()))`` variables falsly warning that jumps

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

rjmccall added a comment. Wait, the whole point of this algorithm is that we have to do this whole complicated linear check against every label whose address is taken because we don't know where it's going. If we have a list of all the possible labels that the `asm goto` might jump to, why are

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

rjmccall added a comment. In D154658#4482202 , @rsmith wrote: > In D154658#4481225 , @rjmccall > wrote: > >> In D154658#4479213 , @rsmith wrote: >> >>> I think (hope?) we

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

rjmccall added inline comments. Comment at: clang/include/clang/AST/Stmt.h:603 -// These don't need to be particularly wide, because they're -// strictly limited by the forms of expressions we permit. -unsigned NumSubExprs : 8; -unsigned ResultIndex : 32 - 8 - N

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

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaLookup.cpp:542 +N = Decls.size(); + } + dexonsmith wrote: > john.brawn wrote: > > rjmccall wrote: > > > john.brawn wrote: > > > > rjmccall wrote: > > > > > This is going to fire on every single o

[PATCH] D154784: [clang] Fix crash caused by PseudoObjectExprBitfields::NumSubExprs overflow

rjmccall added a comment. We had that discussion in the bug report. Let's just increase the widths unconditionally; this is not such a common expression class that we need to worry about using an extra word. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[PATCH] D153835: [Sema] Clone VisibilityAttr for functions in template instantiations

rjmccall added a comment. I agree that the change to test51 is the right result, yes. The explicit visibility attribute on the template declaration should take precedence over the visibility of the types in the template parameter list, and then the explicit visibility attribute on the variable

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

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaLookup.cpp:542 +N = Decls.size(); + } + john.brawn wrote: > rjmccall wrote: > > This is going to fire on every single ordinary lookup that finds multiple > > declarations, right? I haven't full

[PATCH] D153835: [Sema] Clone VisibilityAttr for functions in template instantiations

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. We made a decision ten years or so ago to use a slightly different interpretation of the visibility attributes, so differences from GCC are not by themselves unexpected or proble

[PATCH] D139629: clang: Stop emitting "strictfp"

rjmccall added a comment. In D139629#4481030 , @zahiraam wrote: > According to this comment https://reviews.llvm.org/D87528#2342132, it looks > like @rjmccall had proposed the addition of the attribute. @rjmccall Do you > recall why it was added? My su

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

rjmccall added a comment. In D154658#4479213 , @rsmith wrote: > In D154658#4479170 , @rjmccall > wrote: > >> I don't think it's an intended guarantee of the Itanium ABI that the v-table >> will be unique, and v-

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

rjmccall added a comment. I don't think it's an intended guarantee of the Itanium ABI that the v-table will be unique, and v-tables are frequently not unique in the presence of shared libraries. They should be unique for classes with internal linkage, but of course that's a vastly reduced doma

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

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaLookup.cpp:542 +N = Decls.size(); + } + This is going to fire on every single ordinary lookup that finds multiple declarations, right? I haven't fully internalized the issue you're solving her

[PATCH] D86993: Document Clang's expectations of the C standard library.

rjmccall added a comment. You can't just consider the declared function signatures; `memcpy`ing two non-empty objects with perfect overlap is a violation of `restrict`, and yet IIRC GCC is known to assume that that works in some places. (I can't duplicate that off-hand, though — GCC seems dete

[PATCH] D86993: Document Clang's expectations of the C standard library.

rjmccall added a comment. In D86993#4474267 , @RalfJung wrote: > The first point is important for LLVM's own memcpy/memmove intrinsics, which > are documented as NOPs on size 0 (and e.g. Rust relies on that). Right, I understand that these assumptions co

[PATCH] D86993: Document Clang's expectations of the C standard library.

rjmccall added a comment. I agree with Aaron that it would be better to change the C standard if we can. I don't know how important the first bullet is; IIRC it enables some useful middle-end transformation. I know the second is useful in the frontend so that we don't have to do explicit poin

[PATCH] D153590: Don't use float_t and double_t with #pragma clang fp eval_method.

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

[PATCH] D153590: Don't use float_t and double_t with #pragma clang fp eval_method.

rjmccall added a comment. `abi-check` is a really generic name for a test case; could you rename those three tests to something more specific to this feature? Also, your tests are only testing this behavior in C++. Patch functionality LGTM. Repository: rG LLVM Github Monorepo CHANGES SINC

[PATCH] D152818: [Clang] Make template instantiations respect pragma FENV_ACCESS state at point-of-definition

rjmccall added a comment. Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152818/new/ https://reviews.llvm.org/D152818

[PATCH] D143241: [Clang] Reset FP options before function instantiations

rjmccall added a comment. Hmm. Why are we clearing the FP pragma stack instead of saving the old context onto it and then restoring after instantiation? I don't think semantic analysis ever depends on enclosing members of the stack, does it? Clearing the entire stack might not matter much if

[PATCH] D146148: [clang] Add a namespace for interesting identifiers.

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

[PATCH] D146148: [clang] Add a namespace for interesting identifiers.

rjmccall added a comment. Yes, this is very close, thank you. Comment at: clang/include/clang/Basic/TokenKinds.def:805 +INTERESTING_IDENTIFIER(float_t) +INTERESTING_IDENTIFIER(double_t) +INTERESTING_IDENTIFIER(FILE) I think it would be cleaner if you added thes

[PATCH] D143241: [Clang] Reset FP options before function instantiations

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseTemplate.cpp:1736 + // point of the template definition. + Actions.resetFPOptions(LPT.FPO); + Ah, is this bug specific to the MSVC-compatible template logic? Repository: rG LLVM Github Monorep

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:6784 + if (II->getInterestingIdentifierID() != 0) +NewTD->addAttr(AvailableOnlyInDefaultEvalMethodAttr::Create(Context)); } zahiraam wrote: > rjmccall wrote: > > Please swit

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:6784 + if (II->getInterestingIdentifierID() != 0) +NewTD->addAttr(AvailableOnlyInDefaultEvalMethodAttr::Create(Context)); } Please switch over the interesting identifiers he

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/include/clang/Basic/IdentifierTable.h:341 + void setInterestingIdentifierID(unsigned ID) { +assert(ID != FirstBuiltinID); +ObjCOrBuiltinID = FirstInterestingIdentifierID + (ID - 1); Similarly, this assert

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/include/clang/Basic/IdentifierTable.h:325 void setBuiltinID(unsigned ID) { -ObjCOrBuiltinID = ID + tok::NUM_OBJC_KEYWORDS; -assert(ObjCOrBuiltinID - unsigned(tok::NUM_OBJC_KEYWORDS) == ID - && "ID too large fo

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/include/clang/Basic/IdentifierTable.h:85 +/// (including not_interesting). +/// - The rest of the values represent builtin IDs (including not_builtin). +static constexpr int FirstObjCKeywordID = 1; The code

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/lib/Basic/Builtins.cpp:33 static constexpr Builtin::Info BuiltinInfo[] = { -{"not a builtin function", nullptr, nullptr, nullptr, HeaderDesc::NO_HEADER, +#define INTERESTING_IDENTIFIER(ID)

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/include/clang/Basic/Builtins.h:65 enum ID { +#define INTERESTING_IDENTIER(ID, TYPE, ATTRS) ID, NotBuiltin = 0, // This is not a builtin function. You shouldn't muddle this into `Builtin::ID`. =

[PATCH] D152818: [Clang] Fix assertion when pragma FENV_ACCESS is used with a throw function.

rjmccall added a comment. I'd guess we're not resetting the pragma state appropriately when instantiating templates. Expressions should be governed by the pragmas in scope at the point of definition, not at the point of instantiation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D152689: [NFC] Remove dead conditionals

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/CodeGen/CGObjCMac.cpp:3812 llvm::GlobalVariable *GV; - if (ForClass) -GV = aaron.ballman wrote: > I agree this is dead

[PATCH] D152096: [Clang] Check for abstract parameters only when functions are defined.

rjmccall added inline comments. Comment at: clang/test/SemaObjCXX/parameters.mm:14-17 +struct test2 { virtual void foo() = 0; }; @interface Test2 -- (void) foo: (test2) foo; // expected-error {{parameter type 'test2' is an abstract class}} +- (void) foo: (test2) foo ; @end ---

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

rjmccall added a comment. > I would like to clarify the second point to make sure that I'm incorporating > that suggestion - the checks on lines 25, 26 & 27 are for actual calls - > since now the signature matches the VTT type there's no arg setup prior to > the call, it's a direct passing of t

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

rjmccall added a comment. The process is pretty lightweight; I'd recommend just doing it if you expect to make more than one patch. But we don't have a policy against committing patches for other people as long as there's clear attribution in the commit. I do think you could successfully test

[PATCH] D151515: [CodeGen] add additional cast when checking call arguments

rjmccall added a comment. Yeah, I agree with Eli, there should be a cast here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151515/new/ https://reviews.llvm.org/D151515 ___ cfe-commits mailing list cfe-

[PATCH] D150913: [Clang][BFloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

rjmccall added a comment. One slight miscommunication. Otherwise this LGTM, thank you. Comment at: clang/docs/LanguageExtensions.rst:824 +provide native architectural support for arithmetic on these formats. These +targets are noted in the lists of supported targets above. + -

[PATCH] D150913: [Clang][BFloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:852 ``double`` when passed to ``printf``, so the programmer must explicitly cast it to ``double`` before using it with an ``%f`` or similar specifier. codemzs wrote: > pengfei wrot

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

rjmccall added a comment. Right, that's been my thinking, too. The CVR qualifiers are all "view" qualifiers: both the underlying object and the reference to it are assumed to be the same independent of qualifiers, and the qualifiers just affect the rules around accesses. C++'s rules around qu

[PATCH] D151298: [clang][LoongArch] Fix the calling convention for empty struct in C++ mode

rjmccall added a comment. As far as I'm concerned as editor of the Itanium ABI, the ABI treatment of trivial-for-the-purposes-of-calls classes is purely a psABI matter, and the Itanium ABI's wording around empty classes is merely a suggestion if the psABI doesn't have more specific rules (becau

[PATCH] D151076: [IRGen] Handle infinite cycles in findDominatingStoreToReturnValue.

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

[PATCH] D150913: [Clang][Bfloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:852 ``double`` when passed to ``printf``, so the programmer must explicitly cast it to ``double`` before using it with an ``%f`` or similar specifier. pengfei wrote: > rjmccall wro

[PATCH] D150913: [Clang][Bfloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

rjmccall added a comment. Apologies for misunderstanding what this patch was doing. This all seems reasonable, and the code changes look good. I think the documentation needs significant reorganization; I've attached a draft. Please review for correctness. Comment at: cla

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

rjmccall added a comment. It's weird to have C-style casts that can't be done with any combination of named casts, so I think this makes some sense. I do think it should be limited to numbered address spaces of the same size, though, rather than basing it on whether we're in OpenCL mode. Targ

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

rjmccall added a comment. I see. Yes, in that case I think you're right that we can't test this yet — only base-subobject ctors and dtors get VTT parameters, we only emit calls to those from other ctors and dtors, and those ctors and dtors will always have their own stores to the v-table slot

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

rjmccall added a comment. It's well-formed as IR, just not semantically valid, right? As long as it's not actually crashing in the verifier, please test as much as you reasonably can that's correct, like that the constructor and destructor functions take something in the right address space, a

[PATCH] D148089: [clang][CodeGen] Break up TargetInfo.cpp [1/8]

rjmccall added a comment. Sergei, feel free to start landing patches like this one that were already approved. You don't need the entire sequence to be approved first. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148089/new/ https://reviews.llvm

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/include/clang/Basic/IdentifierTable.h:316 + unsigned getInterestingIdentifierID() { +if (ObjCOrBuiltinID >= 1341 && ObjCOrBuiltinID < 1343) + return ObjCOrBuiltinID; This is closer to the right approach

[PATCH] D150913: [Clang][Bfloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

rjmccall added inline comments. Comment at: clang/lib/Basic/Targets/X86.cpp:381 HasBFloat16 = SSELevel >= SSE2; pengfei wrote: > I'm not sure if I understand the meaning of `HasFullBFloat16`. If it is used > for target that supports arithmetic `__bf16`,

  1   2   3   4   5   6   7   8   9   10   >