[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. 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. > 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#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. 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] 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] 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. 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] 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] 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] 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] 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] 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] 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 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] 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] 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 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] 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] 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] 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] 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] 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] 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] D138296: [clang] Avoid duplicating ProgramAddressSpace in TargetInfo. NFCI

rjmccall added a comment. In D138296#3937486 , @arichardson wrote: > In D138296#3937224 , @eandrews > wrote: > >> Functionally this looks ok to me. However I am not sure if CodeGenTypes is >> the 'right' place

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

rjmccall added a comment. At some point, we're going to have to figure out how this impacts `FLT_EVAL_METHOD`, but we can do that in a separate patch. Comment at: clang/docs/UsersManual.rst:1732 +.. option:: -fexcess-precision: + + By default, Clang uses excess precision to

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

rjmccall added a comment. To be clear, my opinion is just that the requested change is implementable, as I understand it. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126818/new/ https://reviews.llvm.org/D126818 ___ cfe-commits mailing list

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

rjmccall added a comment. In D126818#3954139 , @erichkeane wrote: > In D126818#3954137 , @rjmccall > wrote: > >> To be clear, my opinion is just that the requested change is implementable, >> as I understand it

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

rjmccall added a comment. In D137381#3953178 , @lebedev.ri wrote: > ping. We need to get this going, reminder, this was caught because it caused > a visible miscompilation. I'm sorry, I caught COVID-19 during the LLVM conference and have not had a lot

[PATCH] D138679: [clang][CodeGen] Add default attributes to __clang_call_terminate

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

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

rjmccall added a comment. > The more i stare at this. The more worried i am. Is the idea that, when we > are in termination/UB EH scope, we no longer have to play by the RAII rules, > and destructors, for the moment, are no-ops and are side-effect free? Termination is allowed to happen as soon

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

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:301 + enum Float16ExcessPrecisionKind { FPP_Standard, FPP_Fast, FPP_None }; + rjmccall wrote: > You can leave this named `ExcessPrecisionKind` — if we introduce excess > preci

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

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:301 + enum Float16ExcessPrecisionKind { FPP_Standard, FPP_Fast, FPP_None }; + zahiraam wrote: > rjmccall wrote: > > rjmccall wrote: > > > You can leave this named `ExcessPrecis

[PATCH] D138295: [clang][TargetInfo] Use LangAS for getPointer{Width,Align}()

rjmccall added a comment. This looks great, thanks! Skimmed through the changes pretty quickly. Comment at: clang/lib/AST/Mangle.cpp:237 } - Out << ((TI.getPointerWidth(0) / 8) * ArgWords); + Out << ((TI.getPointerWidth(LangAS::Default) / 8) * ArgWords); } --

[PATCH] D138295: [clang][TargetInfo] Use LangAS for getPointer{Width,Align}()

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1917 +EffectiveFieldSize = FieldSize = TI.Width; +FieldAlign = TI.Align; } else { arich

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:821 +(Precision == LangOptions::ExcessPrecisionKind::FPP_Standard || + Precision == LangOptions::ExcessPrecisionKind::FPP_Fast)) { if (Ty->isAnyComplexType()) {

[PATCH] D138296: [clang] Avoid duplicating ProgramAddressSpace in TargetInfo. NFCI

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I think this is fine. Most of the patch is changing calls to `getTargetAddressSpace` to be internal to IRGen, which, as mentioned, I think is the right move. I do think that if we're goi

[PATCH] D139171: Don't revisit the subexpressions of PseudoObjectExpr when building a ParentMap

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

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. Let's make this patch be purely a representation change. I wouldn't expect *any* test changes from that; if there are, we should understand why. You can then add the stuff about tracking whether there's a pragma in a separate patch. Comment at: cla

[PATCH] D76534: [clang/docs] Fix various sphinx warnings/errors in docs.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Is the option issue causing the documentation to be wrong in any way, or does it just produce a warning? If the latter, we could raise this issue with them. Repository: rG LLVM Github

[PATCH] D76534: [clang/docs] Fix various sphinx warnings/errors in docs.

rjmccall added a comment. Sure. I'm not asking you to fix this, I'm just asking your opinion about the right thing to do. Is the Sphinx warning reasonable, such that we should fix our (autogenerated, IIRC) input to avoid the "redundant" spellings, or should we ask Sphinx to handle it more gra

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. In D76384#1936769 , @mibintc wrote: > In D76384#1934785 , @rjmccall wrote: > > > Let's make this patch be purely a representation change. I wouldn't expect > > *any* test changes from tha

[PATCH] D74183: [IRGen] Add an alignment attribute to underaligned sret parameters

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Herald added a subscriber: wuzish. I'm just going to assume the test changes look good; thank you for taking the time to do this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D741

[PATCH] D74183: [IRGen] Add an alignment attribute to underaligned sret parameters

rjmccall added a comment. In D74183#1937426 , @efriedma wrote: > I think we should remove the LangRef rule that says "sret" pointers have to > be dereferenceable/naturally aligned, and let the frontend add explicit > aligned/dereferenceable markings wher

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:2573 + FPOptions FPFeatures; + Let's split the CallExpr changes out into a separate patch, so that we have one patch that's *purely* about unifying BinaryOperator and CompoundAssignOpe

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:3465 +SubExprs[RHS] = rhs; +hasFPFeatures = true; +isCompoundAssign = 1; mibintc wrote: > rjmccall wrote: > > Okay, so this is *always* adding trailing storage. Is there a com

[PATCH] D74144: [OPENMP50]Add basic support for array-shaping operation.

rjmccall added a comment. Okay, so these array-shaping expressions are only allowed as operands to specific OpenMP directives? That's a plausible interpretation of "The shape-operator can appear only in clauses where it is explicitly allowed" from the spec

[PATCH] D74144: [OPENMP50]Add basic support for array-shaping operation.

rjmccall added a comment. > We cannot build an array type since this expression does not represent a > supported array type. Sizes may be non-constant anywhere in the operation, > variable array type does not support it. I don't think that's true; the element type of a C99 VAT can still be anot

[PATCH] D74144: [OPENMP50]Add basic support for array-shaping operation.

rjmccall added a comment. Okay, if you think that suppress array decay is the right behavior here, that's fine with me. Please check the behavior of other OpenMP implementations, though. Comment at: clang/lib/Sema/SemaExpr.cpp:4848 + Expr::EvalResult EvResult; + if

[PATCH] D74183: [IRGen] Add an alignment attribute to underaligned sret parameters

rjmccall added a comment. Yeah, if emitting alignment assumptions in inlining is causing regressions when frontends provide better information, those assumptions need to be reverted until they can be fixed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D74144: [OPENMP50]Add basic support for array-shaping operation.

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

[PATCH] D74387: [SYCL] Defer __float128 type usage diagnostics

rjmccall added inline comments. Comment at: clang/include/clang/Sema/Sema.h:12248 + /// SYCLDiagIfDeviceCode(Loc, diag::err_type_unsupported) << "__float128"; + DeviceDiagBuilder SYCLDiagIfDeviceCode(SourceLocation Loc, unsigned DiagID); + Will this collect n

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:2573 + FPOptions FPFeatures; + mibintc wrote: > rjmccall wrote: > > Let's split the CallExpr changes out into a separate patch, so that we have > > one patch that's *purely* about unif

[PATCH] D76937: Fix infinite recursion in deferred diagnostic emitter

rjmccall added a comment. Can you explain what exactly the emission/semantic model is for variables? Normal code-generation absolutely triggers the emission of many variables lazily (e.g. internal-linkage globals, C++ inline variables); and any variable that's *not* being emitted lazily actual

[PATCH] D77077: [clang] CodeGen: Make getOrEmitProtocol public for Swift

rjmccall added inline comments. Comment at: clang/include/clang/CodeGen/CodeGenABITypes.h:148 + llvm::function_ref + createProtocolReference); } // end namespace CodeGen I would call this `emitObjCProtocolObject` or something

[PATCH] D77070: [SemaObjC] Add a warning for declarations of BOOL:1 when BOOL is a signed char

rjmccall added a comment. This feels like it's going to bite a lot of people, but maybe it's the best option. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77070/new/ https://reviews.llvm.org/D77070 ___ cfe-commits mailing list cfe-commits

[PATCH] D77077: [clang] CodeGen: Make getOrEmitProtocol public for Swift

rjmccall added inline comments. Comment at: clang/include/clang/CodeGen/CodeGenABITypes.h:148 + llvm::function_ref + createProtocolReference); } // end namespace CodeGen aschwaighofer wrote: > rjmccall wrote: > > I would call

[PATCH] D76937: Fix infinite recursion in deferred diagnostic emitter

rjmccall added a comment. In D76937#1950764 , @yaxunl wrote: > In D76937#1950077 , @rjmccall wrote: > > > Can you explain what exactly the emission/semantic model is for variables? > > Normal code-generation absol

[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

rjmccall added a comment. I made a suggestion in the other patch about restructuring things out of `visitUsedDecl`. I'll review this when that's done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77028/new/ https://reviews.llvm.org/D77028 __

[PATCH] D74387: [SYCL] Defer __float128 type usage diagnostics

rjmccall added inline comments. Comment at: clang/include/clang/Sema/Sema.h:12248 + /// SYCLDiagIfDeviceCode(Loc, diag::err_type_unsupported) << "__float128"; + DeviceDiagBuilder SYCLDiagIfDeviceCode(SourceLocation Loc, unsigned DiagID); + Fznamznon wrote: >

[PATCH] D77077: [clang] CodeGen: Make getOrEmitProtocol public for Swift

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjCGNU.cpp:623 +llvm_unreachable("not implemented"); + } + I think this is just the (unfortunately-named) `GenerateProtocolRef` (the one that just takes a protocol and not a CGF). ==

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:3474 + static unsigned sizeOfTrailingObjects(bool hasFP, bool isCompound) { +return (hasFP ? 1 : 0) * sizeof(unsigned) + + (isCompound ? 2 : 0) * sizeof(QualType); Sorry, I

[PATCH] D77077: [clang] CodeGen: Make getOrEmitProtocol public for Swift

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjCRuntime.h:217 + /// ProtocolPtrTy. + virtual llvm::Constant *GetOrEmitProtocol(const ObjCProtocolDecl *PD) = 0; + aschwaighofer wrote: > aschwaighofer wrote: > > rjmccall wrote: > > > Can this

[PATCH] D77168: Add a flag to debug automatic variable initialization

rjmccall added a comment. I can understand the automation benefits of just counting the number of variables initialized so far in the translation unit without having to modify source, but if you can possibly do this with the pragma, that seems like both a more flexible tool (you can isolate mul

[PATCH] D77077: [clang] CodeGen: Make getOrEmitProtocol public for Swift

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

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/AST/StmtVisitor.h:146 + RetTy VisitBin##NAME(PTR(BinaryOperator) S, ParamTys... P) { \ +DISPATCH(BinAssign, BinaryOperator); \ } erichk

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + rjmccall wrote: > erichkeane wrote: > > rjmccall wrote: > > > The problem with having both functions that take `ASTContext`s and > > > functions tha

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + erichkeane wrote: > rjmccall wrote: > > rjmccall wrote: > > > erichkeane wrote: > > > > rjmccall wrote: > > > > > The problem with having both functi

[PATCH] D77168: Add a flag to debug automatic variable initialization

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:1814 +if (StopAfter) { + static unsigned Counter = 0; + if (Counter >= StopAfter) jcai19 wrote: > rjmccall wrote: > > srhines wrote: > > > MaskRay wrote: > > > > I am a bit wor

[PATCH] D76937: Fix infinite recursion in deferred diagnostic emitter

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

[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

rjmccall added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1508 void checkFunc(SourceLocation Loc, FunctionDecl *FD) { +auto DiagsCountIt = DiagsCount.find(FD); FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back(); It makes me

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:394 + return true; + } + sepavloff wrote: > erichkeane wrote: > > rjmccall wrote: > > > erichkeane wrote: > > > > rjmccall wrote: > > > > > rjmccall wrote: > > > > > > erichk

[PATCH] D77028: [NFC] Refactor DeferredDiagsEmitter and skip redundant visit

rjmccall added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1508 void checkFunc(SourceLocation Loc, FunctionDecl *FD) { +auto DiagsCountIt = DiagsCount.find(FD); FunctionDecl *Caller = UseStack.empty() ? nullptr : UseStack.back(); yaxunl wrote

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

rjmccall added a comment. In D72231#1881797 , @nickdesaulniers wrote: > In D72231#1881784 , @rjmccall wrote: > > > In D72231#1881760 , > > @nickdesaulniers wrote: > > > > >

[PATCH] D68578: [HIP] Fix device stub name

rjmccall added inline comments. Comment at: clang/include/clang/AST/GlobalDecl.h:40 + Stub = 1, +}; + tra wrote: > rjmccall wrote: > > tra wrote: > > > rjmccall wrote: > > > > The attribute here is `CUDAGlobalAttr`; should this be named in terms > > > > of CUDA

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

rjmccall added a comment. Okay. Can we raise this with the kernel folks instead of just assuming they'll be opposed? An obvious patch to fix a few dozen places where they're hit by a warning they intentionally enabled really doesn't seem like a burden. If they push back, fine, we can enable

[PATCH] D74116: [Sema][C++] Propagate conversion type in order to specialize the diagnostics

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:14829 +if (getLangOpts().CPlusPlus) + isInvalid = true; break; Could you hoist this up to the place where we pick the diagnostic and then make it unconditional in the path that

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

rjmccall added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1514 + void visitUsedDecl(SourceLocation Loc, Decl *D) { +if (auto *TD = dyn_cast(D)) { + for (auto *DD : TD->decls()) { erichkeane wrote: > Note that when recommitting this (if you cho

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

rjmccall added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1514 + void visitUsedDecl(SourceLocation Loc, Decl *D) { +if (auto *TD = dyn_cast(D)) { + for (auto *DD : TD->decls()) { erichkeane wrote: > rjmccall wrote: > > erichkeane wrote: > > >

[PATCH] D73186: [AST] Add fixed-point multiplication constant evaluation.

rjmccall added inline comments. Comment at: clang/lib/Basic/FixedPoint.cpp:242 + } else +Overflowed = Result < Min || Result > Max; + leonardchan wrote: > ebevhan wrote: > > rjmccall wrote: > > > ebevhan wrote: > > > > rjmccall wrote: > > > > > leonardchan w

[PATCH] D71227: [cuda][hip] Fix function overload resolution in the global initiailizer.

rjmccall added inline comments. Comment at: clang/include/clang/Sema/Sema.h:11442 + ContextDecl = getCUDACurrentNonLocalVariable(); +return ContextDecl; + } This is tricky because we could be in a nested context, not just the initializer, and that cont

[PATCH] D74116: [Sema][C++] Propagate conversion type in order to specialize the diagnostics

rjmccall added a comment. Yeah, looks like returning true causes the variable to be marked as invalid (not unreasonable, since maybe its type is wrong), which causes downstream diagnostics to be suppressed. Both test cases should probably be fixed to assign into a different variable so that we

[PATCH] D74860: [Sema] Fix pointer-to-int-cast diagnostic for _Bool

rjmccall added a comment. Can we test the same thing in C++? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74860/new/ https://reviews.llvm.org/D74860 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

rjmccall added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1514 + void visitUsedDecl(SourceLocation Loc, Decl *D) { +if (auto *TD = dyn_cast(D)) { + for (auto *DD : TD->decls()) { yaxunl wrote: > rjmccall wrote: > > erichkeane wrote: > > > rjmc

[PATCH] D74116: [Sema][C++] Propagate conversion type in order to specialize the diagnostics

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

[PATCH] D71227: [cuda][hip] Fix function overload resolution in the global initiailizer.

rjmccall added inline comments. Comment at: clang/include/clang/Sema/Sema.h:11444 + return nullptr; +const Decl *ContextDecl = dyn_cast(CurContext); +if (!ContextDecl) You really want this to match whenever we're in a local context, right? How abou

[PATCH] D74860: [Sema] Fix pointer-to-int-cast diagnostic for _Bool

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. In D74860#1888024 , @Mordante wrote: > In D74860#1883415 , @rjmccall wrote: > > > Can we test the same thin

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

rjmccall added a comment. In D70172#1894036 , @yaxunl wrote: > I tried recording functions to be emitted during normal parsing and using it > as starting point for the final traversal. It is quite promising. I only get > one lit test failure for OpenMP:

[PATCH] D74387: [SYCL] Do not diagnose use of __float128

rjmccall added a comment. In D74387#1895264 , @Fznamznon wrote: > @rjmccall, Thank you very much for so detailed response, It really helps. I > started working on implementation and I have a couple of questions/problems > with this particular appoach. >

[PATCH] D75285: Mark restrict pointer or reference to const as invariant

rjmccall added a comment. Unfortunately, `const` also doesn't mean that the memory doesn't change. It does mean it can't be changed through this pointer, but `restrict` allows you to derive more pointers from it within the `restrict` scope, and those pointers can remove the `const` qualifier.

[PATCH] D75285: Mark restrict pointer or reference to const as invariant

rjmccall added a comment. Are you sure `restrict` alone isn't good enough? It doesn't directly tell you that the memory is invariant, but it's usually simple to prove that the memory isn't modified within the `restrict` scope, which might be sufficient. CHANGES SINCE LAST ACTION https://rev

[PATCH] D73186: [AST] Add fixed-point multiplication constant evaluation.

rjmccall added a comment. Since we've settled on not considering that to be overflow, yeah, I think the patch is fine. Might be worth being explicit about that at the point you do the shift: that it's known that this discards precision that could leave the true mathematical value outside of th

[PATCH] D75285: Mark restrict pointer or reference to const as invariant

rjmccall added a comment. In D75285#1897537 , @JonChesterfield wrote: > In D75285#1897247 , @yaxunl wrote: > > > If users derive a non-const pointer from the const pointer and modify it, > > doesn't that result in

[PATCH] D75423: [OpenCL] Mark pointers to constant address space as invariant

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:172 + if (llvm::Constant *C = dyn_cast(Addr)) +Cast = llvm::ConstantExpr::getBitCast(C, ObjectPtr[0]); + else This check is already done by `CreateBitCast`. Comme

[PATCH] D75387: [Sema] Look through OpaqueValueExpr when checking implicit conversions

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

[PATCH] D72841: [RFC] Add support for pragma float_control, to control precision and exception behavior at the source level

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:3055 +same spelling and syntax. For pragmas specified at file scope, a stack +is supported so that the pragma float_control settings can be pushed or popped. + `pragma float_control` s

[PATCH] D74361: [Clang] Undef attribute for global variables

rjmccall added a comment. In D74361#1896982 , @JonChesterfield wrote: > In D74361#1879559 , @rjmccall wrote: > > > This will need to impact static analysis and the AST, I think. Local > > variables can't be redec

[PATCH] D71227: [cuda][hip] Fix function overload resolution in the global initiailizer.

rjmccall added inline comments. Comment at: clang/include/clang/Sema/Sema.h:11444 + return nullptr; +const Decl *ContextDecl = dyn_cast(CurContext); +if (!ContextDecl) rjmccall wrote: > You really want this to match whenever we're in a local context,

[PATCH] D74361: [Clang] Undef attribute for global variables

rjmccall added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4367 +This is useful for variables that are always written to before use where the +default zero initialization provided by the toolchain loader is expensive. + }]; How about: > T

[PATCH] D75423: [OpenCL] Mark pointers to constant address space as invariant

rjmccall added a comment. In D75423#1901206 , @hliao wrote: > invariant checking already takes account of loading from constant address > space or memory (AA::pointsToConstantMemory), that's almost equivalent to > adding invariant attributes. Why do we m

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