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

rjmccall added a comment. In D75423#1901348 , @yaxunl wrote: > In D75423#1901254 , @rjmccall wrote: > > > In D75423#1901206 , @hliao wrote: > > > > > invariant checking alrea

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

rjmccall added a comment. In D75423#1901407 , @yaxunl wrote: > In D75423#1901362 , @rjmccall wrote: > > > Okay, then I have no problem taking a patch for this into IRGen. But I > > think it should be fine to do th

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

rjmccall added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + JonChesterfield wrote: > Quuxplusone wrote: >

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

rjmccall added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + Quuxplusone wrote: > rjmccall wrote: > > JonCh

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

rjmccall added inline comments. Comment at: clang/test/CodeGenCXX/attr-loader-uninitialized.cpp:23 +// CHECK: @nominally_value_init = global i32 undef +int nominally_value_init [[clang::loader_uninitialized]] = 4; + JonChesterfield wrote: > rjmccall wrote: > > Q

[PATCH] D75491: [CodeGenObjC] Privatize some ObjC metadata symbols

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

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

rjmccall added a comment. In D75285#1903284 , @yaxunl wrote: > In D75285#1902835 , > @jeroen.dobbelaere wrote: > > > In D75285#1902788 , @Anastasia > > wrote: > > > > > In

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

rjmccall added a comment. In D75285#1903611 , @yaxunl wrote: > In D75285#1903444 , @rjmccall wrote: > > > That is not true for two reasons: first, `restrict` guarantees that the > > variable is not accessed through

[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/include/clang/AST/Stmt.h:1104 +static_assert(sizeof(*this) <= 16, "changing bitfields changed sizeof(Stmt)"); static_assert(sizeof(*this) % alignof(void *) == 0, mibintc wrote: > rjmcca

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. As I've said before, I absolutely view this polarity flip as good for GPU compilers, and I don't mind Clang needing to take some patches to update how we operate when targeting the GPU and to update some GPU-specific tests. I do not think we should view GPU targets as

[PATCH] D62731: Add support for options -frounding-math, ftrapping-math, -fp-model=, and -fp-exception-behavior=, : Specify floating point behavior

rjmccall added a comment. Yes, thanks, looks a lot better. Just a few tweaks now. Comment at: clang/include/clang/Basic/LangOptions.h:348 VersionTuple getOpenCLVersionTuple() const; + }; Spurious change. Comment at: clang/include/clang/D

[PATCH] D69678: [CodeGen] Fix invalid llvm.linker.options about pragma detect_mismatch

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

[PATCH] D62731: Add support for options -frounding-math, ftrapping-math, -fp-model=, and -fp-exception-behavior=, : Specify floating point behavior

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:133 + +llvm::ConstrainedFPIntrinsic::ExceptionBehavior +CodeGenFunction::ToConstrainedExceptMD(LangOptions::FPExceptionModeKind Kind) { mibintc wrote: > I added these 2 functions, i

[PATCH] D62731: Add support for options -frounding-math, ftrapping-math, -fp-model=, and -fp-exception-behavior=, : Specify floating point behavior

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:137 + llvm_unreachable("Unsupported FP Exception Behavior"); +} + Sorry for dragging this out, but is there a reason these need to be member functions on `CodeGenFunction` rather

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. In D69498#1731265 , @mehdi_amini wrote: > In D69498#1727650 , @dexonsmith > wrote: > > > In D69498#1723606 , @rjmccall > > wrote: > > > > > Perhap

[PATCH] D62731: Add support for options -frounding-math, ftrapping-math, -fp-model=, and -fp-exception-behavior=, : Specify floating point behavior

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM; thanks for your patience during all the rounds of review. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62731/new/ https://reviews.llvm.org/D62731

[PATCH] D51200: Introduce per-callsite inline intrinsics

rjmccall added inline comments. Comment at: include/clang/CodeGen/CGFunctionInfo.h:517 + unsigned InlineCall : 2; + xbolva00 wrote: > rjmccall wrote: > > Please don't propagate this information through the `CGFunctionInfo`. I > > really want this type to be

[PATCH] D68108: Redeclare Objective-C property accessors inside the ObjCImplDecl in which they are synthesized.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, thanks for patiently working through all this review. I'm happy with this now. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5063

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

rjmccall added a comment. Looks okay to me. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62731/new/ https://reviews.llvm.org/D62731 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-b

[PATCH] D69272: Restricted variant of '#pragma STDC FENV_ACCESS'

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1068 + Warning<"in functions '#pragma STDC FENV_ACCESS ON' is supported only " + "in topmost block, ignoring pragma">, InGroup; "'#pragma STDC FENV_ACCESS ON' is

[PATCH] D69938: [OpenCL] Use __generic addr space when generating internal representation of lambda

rjmccall added a comment. It does make logical sense to be able to qualify the call operator of a lambda. The lambda always starts as a temporary, so presumably we want the default address space qualifier on lambdas to be compatible with the address space of temporaries. However, that's proba

[PATCH] D69810: [OpenCL] Fix address space for base method call (PR43145)

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:4091 +V = Builder.CreatePointerBitCastOrAddrSpaceCast( +V, IRFuncTy->getParamType(FirstIRArg)); + else Anastasia wrote: > svenvh wrote: > > @Anastasia po

[PATCH] D65744: [PR42707][OpenCL] Fix addr space deduction for auto

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaDecl.cpp:6721 + if (getLangOpts().OpenCL) { + Anastasia wrote: > rjmccall wrote: > > Anastasia wrote: > > > rjmccal

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

rjmccall added a comment. Distinguishing between multiple symbols associated with the same source-level declaration is the purpose of the GlobalDecl abstraction. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68578/new/ https://reviews.llvm.org/D68578

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

rjmccall added a comment. In D68578#1737415 , @yaxunl wrote: > In D68578#1737351 , @rjmccall wrote: > > > Distinguishing between multiple symbols associated with the same > > source-level declaration is the purpose

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

rjmccall added a comment. There are a number of places in IRGen that pass around `GlobalDecl`s with the expectation that that's sufficient to uniquely identify a symbol. The fact that IRGen breaks down the GD at the last second before passing it to the mangler, rather than passing it to the ma

[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

rjmccall added a comment. In D69991#1739456 , @aprantl wrote: > Why doesn't this need an update in lib/Serialization, is there generic code > that handles all attributes? The serialization logic for attributes is tblgen'ed from Attr.td. Repository:

[PATCH] D69938: [OpenCL] Use __generic addr space when generating internal representation of lambda

rjmccall added a comment. In D69938#1741080 , @Anastasia wrote: > In D69938#1737196 , @rjmccall wrote: > > > It does make logical sense to be able to qualify the call operator of a > > lambda. The lambda always st

[PATCH] D69938: [OpenCL] Use __generic addr space when generating internal representation of lambda

rjmccall added a comment. In D69938#1742026 , @Anastasia wrote: > In D69938#1741713 , @rjmccall wrote: > > > In D69938#1741080 , @Anastasia > > wrote: > > > > > In D69938#17

[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

rjmccall added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3905 +categories, or extensions (for the latter two it applies the attribute +to all methods declared in the category/implementation), + Editing error? Comment at

[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4095 +Builder.CreateStore(result.getScalarVal(), +CGF.GetAddrOfLocalVar(OMD->getSelfDecl())); + MadCoder wrote: > rjmccall wrote: > > We should also be changin

[PATCH] D70172: [CUDA][HIP] Fix assertion due to dtor check on windows

rjmccall added a reviewer: rsmith. rjmccall added a subscriber: rsmith. rjmccall added a comment. This seems like the wrong approach; @rsmith should take a look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70172/new/ https://reviews.llvm.org/D70172 _

[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

rjmccall added a comment. Could you add tests for the following cases: 1. A direct property that names a getter that is non-direct and explicitly declared in the current interface. 2. #1, but explicitly declared elsewhere, maybe in a super class or in the primary interface. 3. #1 and #2, but wh

[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

rjmccall added a comment. The symbols probably won't confuse the debugger; after all, the only difference here is that they're actually external. Of course the debugger will need work to support direct methods in general, since they won't be in method tables. CHANGES SINCE LAST ACTION https

[PATCH] D69938: [OpenCL] Use __generic addr space when generating internal representation of lambda

rjmccall added a comment. In D69938#1745737 , @Anastasia wrote: > In D69938#1742354 , @rjmccall wrote: > > > In D69938#1742026 , @Anastasia > > wrote: > > > > > In D69938#17

[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

rjmccall added a comment. Thanks, the tests look great. Comment at: clang/include/clang/Basic/AttrDocs.td:3919 +in particular, this means that it cannot override a superclass method or satisfy +a protocol requirement. + Please add a new paragraph here: Beca

[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExprObjC.cpp:1129 + } +} + Is this a good idea to do for arbitrary diagnostics? I feel like saying "direct" in the note when that has nothing to do with the conflict could be a little misleading.

[PATCH] D69938: [OpenCL] Use __generic addr space when generating internal representation of lambda

rjmccall added a comment. > I was already planning to add this. I could look into it next and maybe just > a add FIXME in the test for now. Sure. > Btw global lambda objects are in `__global` address space in OpenCL. Just based on being written outside of a function body or in a static-local

[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. This seems reasonable to me, then. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69991/new/ https://reviews.llvm.org/D69991 _

[PATCH] D70242: [OpenCL] Allow addr space qualifiers on lambda call expressions

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Well that was easy. Do we accept the address-space attribute in this position, or is that TBD? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70242/new/ https://reviews.llvm.org/D

[PATCH] D70256: [FPEnv] clang support for constrained FP builtins

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:357 + + if (ConstrainedIntrinsicID != Intrinsic::not_intrinsic && + CGF.Builder.getIsFPConstrained()) { I don't see any situation where `not_intrinsic` is passed in here; why all t

[PATCH] D69991: Implement __attribute__((objc_direct)), __attribute__((objc_direct_members))

rjmccall added a comment. Okay. There's actually a good reason to emit the selector reference as close to the call as possible, but I agree that we shouldn't do that in this patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69991/new/ https://reviews.llvm.org/D69991

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

rjmccall added inline comments. Comment at: llvm/include/llvm/IR/IRBuilder.h:262 Function *F = BB->getParent(); -if (!F->hasFnAttribute(Attribute::StrictFP)) { +if (F && !F->hasFnAttribute(Attribute::StrictFP)) { F->addFnAttr(Attribute::StrictFP);

[PATCH] D70256: [FPEnv] clang support for constrained FP builtins

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:357 + + if (ConstrainedIntrinsicID != Intrinsic::not_intrinsic && + CGF.Builder.getIsFPConstrained()) { kpn wrote: > rjmccall wrote: > > I don't see any situation where `not_intrins

[PATCH] D69938: [OpenCL] Use __generic addr space when generating internal representation of lambda

rjmccall added a comment. Yes, in that case copy-elision into the global variable is guaranteed. You can write arbitrary expressions in global initializers, however, and those can use temporary lambdas. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69938/new/ https://reviews.llvm.org

[PATCH] D69810: [OpenCL] Fix address space for base method call (PR43145)

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:2721 + From = ImpCastExprToType(From, DestASTy, CK_AddressSpaceConversion, + From->getValueKind()).get(); +} Both the source and dest types here are off

[PATCH] D69810: [OpenCL] Fix address space for base method call (PR43145)

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

[PATCH] D69938: [OpenCL] Use __generic addr space when generating internal representation of lambda

rjmccall added a reviewer: rsmith. rjmccall added a comment. In D69938#1754894 , @Anastasia wrote: > In D69938#1752024 , @rjmccall wrote: > > > Yes, in that case copy-elision into the global variable is guaranteed.

[PATCH] D69938: [OpenCL] Use __generic addr space when generating internal representation of lambda

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaLambda.cpp:891 +if (getLangOpts().OpenCL) + EPI.TypeQuals.addAddressSpace(LangAS::opencl_generic); + This should probably check that there isn't already an address space, right? CHANGES SIN

[PATCH] D70605: [OpenCL] Fix address space for implicit conversion (PR43145)

rjmccall added a comment. Is there a similar problem with reference parameters? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70605/new/ https://reviews.llvm.org/D70605 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.l

[PATCH] D70671: [clang][CodeGen] Fix wrong memcpy size of no_unique_address in FieldMemcpyizer

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

[PATCH] D70605: [OpenCL] Fix address space for implicit conversion (PR43145)

rjmccall added a comment. Great. Could you add that to the test, then, just to make sure we don't regress it? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70605/new/ https://reviews.llvm.org/D70605 ___ cfe-commits mailing list cfe-commit

[PATCH] D69938: [OpenCL] Use __generic addr space when generating internal representation of lambda

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaLambda.cpp:891 +if (getLangOpts().OpenCL) + EPI.TypeQuals.addAddressSpace(LangAS::opencl_generic); + Anastasia wrote: > rjmccall wrote: > > This should probably check that there isn't already

[PATCH] D69938: [OpenCL] Use __generic addr space when generating internal representation of lambda

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaLambda.cpp:892 +if (AS != LangAS::Default) + EPI.TypeQuals.addAddressSpace(getDefaultCXXMethodAddrSpace()); + This should just be `AS`. Could you go ahead and change the other, existing plac

[PATCH] D70605: [OpenCL] Fix address space for implicit conversion (PR43145)

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExprCXX.cpp:4108 + NewToType = Context.getPointerType(NewToType); +} + Hmm, sorry, just noticed this. Could you write this so that it preserves the original kind of pointer of `ToType`? I

[PATCH] D69938: [OpenCL] Use __generic addr space when generating internal representation of lambda

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, that looks great. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69938/new/ https://reviews.llvm.org/D69938 ___ cfe-commits m

[PATCH] D70605: [OpenCL] Fix address space for implicit conversion (PR43145)

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

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

rjmccall added a comment. Let me try to restate what's happening here so that I can see if I understand it. There are two concepts of "section" in play here: - a unit in an object file with a particular section name, which need not be unique within the object file, and which can have interesti

[PATCH] D70923: Fix comment to more accurately describe C++ language requirements around tail padding.

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

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

rjmccall added a comment. > So I don't think we should even be trying to mark sections as mergeable > unless we walk all elements in the section and make sure it's even safe to > apply these. That's the conservative fix, yes. You can either: 1. emit global objects with the same section name i

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

rjmccall added a comment. I can't speak for the pragma authors, but I strongly doubt the pragma is intended to force all affected globals to go into a single section unit, since the division into section units is purely an object-file representation issue. Looks to me like `MCContext::getELFSec

[PATCH] D70935: [CodeGen][ObjC] Emit a primitive store to store a __strong field in ExpandTypeFromArgs

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:1054 + "pointer to __strong expected"); + EmitStoreOfScalar(*AI++, LV); +} else This definitely deserves a comment. I don't think the assertion is right; the condition

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

rjmccall added a comment. In D68101#1766670 , @bd1976llvm wrote: > In D68101#1766359 , @rjmccall wrote: > > > I can't speak for the pragma authors, but I strongly doubt the pragma is > > intended to force all affec

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

rjmccall added a comment. We're not talking about putting objects in a different section *in the image*. But ELF allows object files to contain an arbitrary number of what I've been calling "section units" that will be assembled into a single section in the image. In theory, we could even emi

[PATCH] D68101: [MC][ELF] Prevent globals with an explicit section from being mergeable

rjmccall added a comment. In D68101#1767732 , @nickdesaulniers wrote: > > But ELF allows object files to contain an arbitrary number of what I've > > been calling "section units" that will be assembled into a single section > > in the image. > > More pr

[PATCH] D70935: [CodeGen][ObjC] Emit a primitive store to store a __strong field in ExpandTypeFromArgs

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Yes, that's great, thank you. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70935/new/ https://reviews.llvm.org/D70935 __

[PATCH] D69088: [Lex] #pragma clang transform

rjmccall added a comment. This is a major new language feature, and code review is probably not the right venue for reviewing it; there should be a thread on cfe-dev. My apologies if that's already been discussed and I missed it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

[PATCH] D70172: [CUDA][HIP] Fix assertion due to dtor check on windows

rjmccall added a comment. Richard is definitely our main expert in the implicit synthesis of special members. It seems to me that if we need the destructor declaration at some point, we should be forcing it to exist at that point. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70172/ne

[PATCH] D71111: [Sema] Improve diagnostic about addr spaces for overload candidates

rjmccall added a comment. Hmm. How about: // For the `this` argument candidate function not viable: 'this' object is in '__private' address space, but method expects object in '__global' address space // For pointer arguments candidate function not viable: cannot pass pointer to '__p

[PATCH] D71226: Also synthesize _cmd and self for properties

rjmccall added a comment. It'd be nice if we didn't have to synthesize parameters for all declarations when we're just handling an `@interface`, but we can improve that in follow-ups. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71226/new/ https:

[PATCH] D71226: Also synthesize _cmd and self for properties

rjmccall added a comment. Alternatively, we could make IRGen not depend on those parameters being set just to call a function, which seems reasonable since the parameter variables are logically internal to the implementation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D71208: CodeGen: Allow annotations on globals in non-zero address space

rjmccall added a comment. The questions I'd like to have answered before I can approve this are: - whether there are clients of `@llvm.global.annotations` that will have problems with non-0 address spaces and - whether this will interfere with someday having an invariant that `addrspacecast` is

[PATCH] D71208: CodeGen: Allow annotations on globals in non-zero address space

rjmccall added a comment. In D71208#1776633 , @nhaehnle wrote: > In D71208#1776202 , @rjmccall wrote: > > > The questions I'd like to have answered before I can approve this are: > > > > - whether there are clients

[PATCH] D71282: Fix bug 44190 - wrong code with #pragma pack(1)

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:4008 + if ((!ArgInfo.getIndirectByVal() || + (LV.getAlignment() < getContext().getTypeAlignInChars(I->Ty { NeedCopy = true; Thanks for the fix. Plea

[PATCH] D70256: [FPEnv] clang support for constrained FP builtins

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

[PATCH] D71208: CodeGen: Allow annotations on globals in non-zero address space

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. In D71208#1776667 , @nhaehnle wrote: > > My concern is that there's something that's going to blow up or miscompile > > if we start passing in cons

[PATCH] D71111: [Sema] Improve diagnostic about addr spaces for overload candidates

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3838 +"'this' object is in address space %3, but method expects object in " +"address space %4">; def note_ovl_candidate_bad_gc : Note< I think you need to move

[PATCH] D71111: [Sema] Improve diagnostic about addr spaces for overload candidates

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, that looks much better. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D7/new/ https://reviews.llvm.org/D7 ___ cfe-com

[PATCH] D71431: Call objc_retainBlock before passing a block as a variadic argument

rjmccall added a comment. Patch looks good, but could you describe this behavior in the ARC documentation? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71431/new/ https://reviews.llvm.org/D71431 ___ c

[PATCH] D71397: [clang] Improve LLVM-style RTTI support in ExternalASTSource/ExternalSemaSource

rjmccall added inline comments. Comment at: clang/include/clang/AST/ExternalASTSource.h:69 - /// Whether this AST source also provides information for - /// semantic analysis. - bool SemaSource = false; + // LLVM-style RTTI. + static char ID; aprantl wrote

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

rjmccall added a comment. Currently we emit a warning if you use `-frounding-math`, saying that the option is ignored. That should have alerted users that they're not getting the correct behavior now. This patch removes the warning and (IIUC) implements the correct behavior but is over-conser

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

rjmccall added a comment. In D62731#1782897 , @andrew.w.kaylor wrote: > In D62731#1782762 , @rjmccall wrote: > > > Currently we emit a warning if you use `-frounding-math`, saying that the > > option is ignored.

[PATCH] D71431: Call objc_retainBlock before passing a block as a variadic argument

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/docs/AutomaticReferenceCounting.rst:1866 +When a block pointer type is converted to type ``id``, ``Block_copy`` is called. +This is necessary bec

[PATCH] D62731: Add support for options -frounding-math, -ftrapping-math, -ffp-model=, and -ffp-exception-behavior=, : Specify floating point behavior

rjmccall added a comment. I believe your analysis on the second point is unfortunately missing half the problem. Functions like `fesetround` will be treated as having arbitrary side-effects by default, which mean arbitrary code can't be reordered with calls to them. It'd be good to model that

[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

rjmccall added a comment. The bug with `__builtin_isless` should be a really easy fix; the builtin just needs to be flagged as having custom type-checking, and then we need to make sure we do appropriate promotions on the arguments (but we probably do). Repository: rG LLVM Github Monorepo C

[PATCH] D71397: [clang] Improve LLVM-style RTTI support in ExternalASTSource/ExternalSemaSource

rjmccall added a comment. If this is commonly used, it would make sense to introduce those types into some common header in LLVM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71397/new/ https://reviews.llvm.org/D71397

[PATCH] D71397: [clang] Improve LLVM-style RTTI support in ExternalASTSource/ExternalSemaSource

rjmccall added a comment. If we decide as a community that D39111 is better, we can go back and adopt it at any point; I don't see that as a reason to reject modest refinements, especially given that there is no sign whatsoever of movement on D39111

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

rjmccall resigned from this revision. rjmccall added a comment. I opposed the creation of this library on these terms in the first place, so I'm pretty sure I'm not on the hook to review it. Introducing an IRBuilder-level finalization stack sounds like it's going to be a huge mess if your goal

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

rjmccall added a comment. In D70258#1788396 , @jdoerfert wrote: > In D70258#1788305 , @rjmccall wrote: > > > Introducing an IRBuilder-level finalization stack sounds like it's going to > > be a huge mess if your go

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

rjmccall added a comment. So it's never that OpenMP has things that need to be finalized before exiting (e.g. if something in frontend-emitted code throws an exception), it's just that OpenMP might need to generate its own control flow that breaks through arbitrary scopes in the frontend? I wo

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

rjmccall added a comment. That's what I figured. The thing is that that necessarily interacts correctly with everything in Clang's IRGen in ways that making a second stack that Clang's IRGen doesn't directly know about doesn't. I think you either need to extract Clang's entire cleanup-stack co

[PATCH] D71650: [AST] Fix compilation with GCC < 8 for MinGW

rjmccall added a comment. Wow, that's novel. Please add a comment explaining that this is a compiler workaround, but otherwise LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71650/new/ https://reviews.llvm.org/D71650 _

[PATCH] D71650: [AST] Fix compilation with GCC < 8 for MinGW

rjmccall added a comment. In D71650#1790067 , @mstorsjo wrote: > In D71650#1789897 , @rjmccall wrote: > > > Wow, that's novel. Please add a comment explaining that this is a compiler > > workaround, but otherwise

[PATCH] D71650: [AST] Fix compilation with GCC < 8 for MinGW

rjmccall added a comment. We do have a static assert. I won't insist on the comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71650/new/ https://reviews.llvm.org/D71650 ___ cfe-commits mailing li

[PATCH] D71708: [OPENMP50]Codegen for nontemporal clause.

rjmccall added inline comments. Comment at: clang/lib/AST/StmtProfile.cpp:777 +if (E) + Profiler->VisitStmt(E); + } Can `E` actually be null here? Comment at: clang/lib/CodeGen/CGExpr.cpp:3952 + CGM.getOpenMPRuntime().isNont

[PATCH] D70258: [OpenMP][IR-Builder] Introduce the finalization stack

rjmccall added a comment. Okay, if this is just handling OpenMP-specific control flow and doesn't need to interact with what a reasonable frontend would do with cleanups, I'm appeased. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70258/new/ http

[PATCH] D71708: [OPENMP50]Codegen for nontemporal clause.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3952 + CGM.getOpenMPRuntime().isNontemporalDecl(Field)) || + (!E->isArrow() && BaseLV.isNontemporal())) +LV.setNontemporal(/*Value=*/true); ABataev wrote: > ABatae

[PATCH] D69272: Restricted variant of '#pragma STDC FENV_ACCESS'

rjmccall added a comment. In D69272#1792928 , @sepavloff wrote: > In D69272#1792877 , @kpn wrote: > > > My understanding of this patch is that it only allows the #pragma at the > > top of each function. It doesn't

[PATCH] D71708: [OPENMP50]Codegen for nontemporal clause.

rjmccall added a comment. Thanks. Could you add some tests that `nontemporal` directives aren't disturbed by either (1) nested `nontemporal` directives or (2) nested directives of some other kind? Comment at: clang/lib/CodeGen/CGExpr.cpp:3952 + CGM.getOpenMPRuntime

[PATCH] D71708: [OPENMP50]Codegen for nontemporal clause.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3952 + CGM.getOpenMPRuntime().isNontemporalDecl(Field)) || + (!E->isArrow() && BaseLV.isNontemporal())) +LV.setNontemporal(/*Value=*/true); rjmccall wrote: > ABata

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