[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1304 +RHSTy = ResultTy; + } + leonardchan wrote: > rjmccall wrote: > > ebevhan wrote: > > > rjmccall wrote: > > > > Hmm. So adding a signed integer to an unsigned fixed-point type alway

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1304 +RHSTy = ResultTy; + } + ebevhan wrote: > rjmccall wrote: > > leonardchan wrote: > > > rjmccall wrote: > > > > ebevhan wrote: > > > > > rjmccall wrote: > > > > > > Hmm. So adding a

[PATCH] D55127: [OpenCL] Diagnose conflicting address spaces between template definition and its instantiation

rjmccall added a comment. Okay. What you're saying is that the current OpenCL C++ compiler is implicitly adding an address-space qualifier at the top level to explicit template arguments. I agree that it should not be doing that. Can you explain why the changes to `TreeTransform` are required

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1304 +RHSTy = ResultTy; + } + leonardchan wrote: > rjmccall wrote: > > ebevhan wrote: > > > rjmccall wrote: > > > > leonardchan wrote: > > > > > rjmccall wrote: > > > > > > ebevhan wrote

[PATCH] D53153: [OpenCL] Mark kernel functions with default visibility

rjmccall added a comment. I think `-fvisibility=hidden` isn't good enough because you want to infer hidden visibility even for external symbol references, and that's just not how global visibility works. But after this discussion, I'm prepared to accept that (1) we should have some sort of sin

[PATCH] D55067: [HIP] Fix offset of kernel argument for AMDGPU target

rjmccall added a comment. In D55067#1318810 , @arsenm wrote: > In D55067#1313419 , @rjmccall wrote: > > > I understand that it's copied into a properly-aligned local variable, but > > if it affects how the function

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1304 +RHSTy = ResultTy; + } + leonardchan wrote: > rjmccall wrote: > > leonardchan wrote: > > > rjmccall wrote: > > > > ebevhan wrote: > > > > > rjmccall wrote: > > > > > > leonardchan w

[PATCH] D55127: [OpenCL] Diagnose conflicting address spaces between template definition and its instantiation

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM with minor revisions. Comment at: lib/Sema/SemaType.cpp:7206 + // Do not deduce address spaces in templates. + T->isDependentType()) return; -

[PATCH] D55067: [HIP] Fix offset of kernel argument for AMDGPU target

rjmccall added a comment. In D55067#1319047 , @arsenm wrote: > I don't understand how the alignment of the IR type isn't meaningful or > stable. The DataLayout gives us the concept of an ABI alignment, but you seem > to be saying this is meaningless (in

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1304 +RHSTy = ResultTy; + } + leonardchan wrote: > rjmccall wrote: > > leonardchan wrote: > > > rjmccall wrote: > > > > leonardchan wrote: > > > > > rjmccall wrote: > > > > > > ebevhan w

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

rjmccall added a subscriber: akyrtzi. rjmccall added a comment. I have a lot of comments about various things that need to be fixed, but I just want to take a moment to say that this is a tremendously impressive patch: given only some very high-level directions, you've done a great job figuring

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Although this also fixes bugs with unaligned pointers that you might consider adding tests for. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55262/

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

rjmccall added inline comments. Comment at: lib/AST/ItaniumMangle.cpp:1507 +Qualifiers MethodQuals = Qualifiers::fromCVRUMask( +Method->getTypeQualifiers().getCVRUQualifiers()); // We do not consider restrict a distinguishing attribute for overloading ---

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

rjmccall added a comment. The code was dropping all the original l-value information, which includes alignment. So if you start with an l-value that refers to under-aligned memory (or over-aligned), we were losing that information on the resulting l-value. The test case would be something lik

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:1304 +RHSTy = ResultTy; + } + ebevhan wrote: > rjmccall wrote: > > leonardchan wrote: > > > rjmccall wrote: > > > > leonardchan wrote: > > > > > rjmccall wrote: > > > > > > leonardchan w

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

rjmccall added a comment. Oh, sorry, that wasn't a good example because that's still the natural alignment for `A`. It should be `static_cast<__generic int &>(GPtr->X)`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55262/new/ https://reviews.llvm.org/D55262 ___

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added a comment. Okay, thanks, that makes sense to me. I'll ask around to find a way to contact the C committee. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53738/new/ https://reviews.llvm.org/D53738 ___

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

rjmccall added inline comments. Comment at: lib/AST/ItaniumMangle.cpp:1507 +Qualifiers MethodQuals = Qualifiers::fromCVRUMask( +Method->getTypeQualifiers().getCVRUQualifiers()); // We do not consider restrict a distinguishing attribute for overloading ---

[PATCH] D55349: Convert some ObjC alloc msgSends to runtime calls

rjmccall added inline comments. Comment at: include/clang/Basic/ObjCRuntime.h:189 + /// When this method returns true, Clang will turn non-super message sends of + /// certain selectors into calls to the correspond entrypoint: + /// alloc => objc_alloc "corr

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

rjmccall added a comment. In D55262#1321048 , @romanovvlad wrote: > I'm getting the same IR for both examples you provided. And for both load of > X value is aligned on 4 bytes wo patch and on 1 byte with. That's odd, because the natural type alignment

[PATCH] D54862: [OpenCL] Add generic AS to 'this' pointer

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, but you can probably undo one of my requests. :) Comment at: lib/AST/Type.cpp:2950 + FunctionTypeBits.HasExtQuals = 0; + } } mikael wrote: > mi

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

rjmccall added a comment. In D55262#1321641 , @romanovvlad wrote: > Yes it's 1, but after addrspacecast alignment becomes not struct A but > alignment of the pointer Oh! Yes, that's even more broken than I thought. Comment at: lib/C

[PATCH] D55371: Fix thunks returning memptrs via sret by emitting also scalar return values directly in sret slot (PR39901)

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. That seems reasonable. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55371/new/ https://reviews.llvm.org/D55371 ___ cfe-commits mail

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

rjmccall added a comment. Sorry. Can you post the result of passing `-ast-dump` instead of `-emit-llvm` to the compiler? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55262/new/ https://reviews.llvm.org/D55262 ___ cfe-commits mailing list

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

rjmccall added a comment. In D55262#1321682 , @rjmccall wrote: > Sorry. Can you post the result of passing `-ast-dump` instead of > `-emit-llvm` to the compiler? Actually, nevermind, I don't need that. If you can fix the address-space issue I pointed

[PATCH] D55262: [OpenCL] Fix for TBAA information of pointer after addresspacecast

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

[PATCH] D55349: Convert some ObjC alloc msgSends to runtime calls

rjmccall added inline comments. Comment at: lib/CodeGen/CGObjC.cpp:385 +// prefix are identified as OMF_alloc but we only want to call the +// runtime for this version. +if (Sel.isUnarySelector() && Sel.getNameForSlot(0) == "alloc") Actual

[PATCH] D55349: Convert some ObjC alloc msgSends to runtime calls

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

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rjmccall added a comment. Yeah, if we weren't exercising the code in our test suite, that would be one thing, but I think a language-mode-specific test probably isn't too valuable. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45898/new/ https://reviews.llvm.org

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rjmccall added a comment. In D45898#1324625 , @orivej wrote: > I have noticed that this change breaks seemingly valid code: > > class A { protected: ~A(); }; > struct B : A {}; > B f() { return B(); } > B g() { return {}; } > > > `f` compiles, but

[PATCH] D45898: [SemaCXX] Mark destructor as referenced

rjmccall added a comment. Always worth adding more tests. Mind writing that one up as a commit? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45898/new/ https://reviews.llvm.org/D45898 ___ cfe-commits mailing list c

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added a comment. We sent the question out, but we haven't gotten a response yet. I think going forward under the idea that this just changes the type but does the operation on the original operand types is the right way to go forward in the short term. Repository: rC Clang CHANGES

[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

rjmccall added a comment. I'm fine with making this change under the assumption that we've gotten the language rule right. Even if that weren't abstractly reasonable for general language work — and I do think it's reasonable when we have a good-faith question about the right semantics — this i

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. Okay, one last minor request, then LGTM. Comment at: lib/CodeGen/CGObjC.cpp:2953 + return asImpl().visitExpr(e); +} + Oh, I'd forgotten this wasn't a normal expression visitor. Well, okay, this isn't too bad. Comm

[PATCH] D58634: [PR40778] Generate address space conversion when binding reference to a temporary value in different address space

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:4067 + IRFuncTy->getParamType(FirstIRArg)->isPointerTy()) +V = Builder.CreatePointerBitCastOrAddrSpaceCast( +V, IRFuncTy->getParamType(FirstIRArg)); Anasta

[PATCH] D56411: [CUDA][HIP][Sema] Fix template kernel with function as template parameter

rjmccall added a comment. In D56411#1406212 , @yaxunl wrote: > I would like to fix the validation issue only and leave the overload > resolution issue for future. As I understand it, the "validation issue" is just that you'd like a diagnostic to be emi

[PATCH] D58321: [WIP] Support for relative vtables

rjmccall added inline comments. Comment at: clang/include/clang/Driver/Options.td:1250 + Group, Flags<[CC1Option]>, + HelpText<"Use the unstable C++ class ABI for classes with hidden LTO visibility">; def flto_jobs_EQ : Joined<["-"], "flto-jobs=">, Please mak

[PATCH] D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions

rjmccall added a comment. A couple tweaks, but otherwise LGTM. Comment at: clang/include/clang/AST/OperationKinds.def:203 +/// CK_FixedPointToIntegral - Fixed point to an integral. +///(int) 2.0k This is super-picky, but please either just say "integral"

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58514/new/ https://reviews.llvm.org/D58514 ___ c

[PATCH] D58147: [CodeGen] Fix calling llvm.var.annotation outside of a basic block.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Well, it won't have a guarantee that it won't see unused annotations, but alright, I'm fine with this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58147/new/ https://reviews.ll

[PATCH] D56411: [CUDA][HIP][Sema] Fix template kernel with function as template parameter

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. In D56411#1410598 , @yaxunl wrote: > In D56411#1410153 , @rjmccall wrote: > > > In D56411#1406212

[PATCH] D57716: [CUDA][HIP] Check calling convention based on function target

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclAttr.cpp:4620 const TargetInfo &TI = Context.getTargetInfo(); - TargetInfo::CallingConvCheckResult A = TI.checkCallingConvention(CC); + auto *Aux = Context.getAuxTargetInfo(); + if (LangOpts.CUDA) { --

[PATCH] D57716: [CUDA][HIP] Check calling convention based on function target

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclAttr.cpp:4620 const TargetInfo &TI = Context.getTargetInfo(); - TargetInfo::CallingConvCheckResult A = TI.checkCallingConvention(CC); + auto *Aux = Context.getAuxTargetInfo(); + if (LangOpts.CUDA) { --

[PATCH] D58708: [PR40778] Preserve addr space in Derived to Base cast

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:2670 } else { DestType = DestRecordType; FromRecordType = FromType; This path (when the object is a gl-value) also needs an address-space qualifier, so you should probably add i

[PATCH] D58719: [PR40778][Sema] Adjust address space of operands in builtin operators

rjmccall added a comment. I think the right model is that a builtin operator candidate exists for all possible address-space qualifiers on the pointer/reference pointee, and it should be straightforward to only actually add candidates for the qualifier used by the corresponding operand. CHANG

[PATCH] D58236: Make address space conversions a bit stricter.

rjmccall added inline comments. Comment at: test/SemaOpenCL/address-spaces.cl:87 + + // FIXME: This doesn't seem right. This should be an error, not a warning. + __local int * __global * __private * lll; Anastasia wrote: > ebevhan wrote: > > Anastasia wrote: >

[PATCH] D58729: Emit boxed expression as a compile-time constant if the expression inside the parentheses is a string literal

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprConstant.cpp:1793 + "this boxed expression can't be emitted as a compile-time constant"); + return emitConstantObjCStringLiteral(cast(E->getSubExpr()), + E->getType(), CGM

[PATCH] D58729: Emit boxed expression as a compile-time constant if the expression inside the parentheses is a string literal

rjmccall added inline comments. Comment at: lib/Sema/SemaExprObjC.cpp:534 +NSStringPointer, NSStringPointer); +return new (Context) ObjCBoxedExpr(SL, BoxedType, nullptr, SR); + } ahatanak wrote: > rjmccall wrote: > > You're implicitly dro

[PATCH] D58346: [Sema] Change addr space diagnostics in casts to follow C++ style

rjmccall added a comment. Hmm. Yeah, Embedded C allows these casts, so contra my previous comment, I think we can't make them ill-formed outside of OpenCL mode. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58346/new/ https://reviews.llvm.org/D58346

[PATCH] D58729: Emit boxed expression as a compile-time constant if the expression inside the parentheses is a string literal

rjmccall added inline comments. Comment at: test/SemaObjC/boxing-illegal.m:70 + s = @(L"abc"); // expected-error {{illegal type 'int *' used in a boxed expression}} + s = @("\pabc"); // expected-error {{illegal type 'unsigned char *' used in a boxed expression}} +} --

[PATCH] D58878: [Diagnostics] Warn for assignments in bool contexts

rjmccall added a comment. This seems eminently reasonable to warn about. I think people might legitimately complain about extending an existing warning to a new set of contexts without putting it under a new warning flag, though. How annoying would it be to clone the warnings and put them in

[PATCH] D58346: [Sema] Change addr space diagnostics in casts to follow C++ style

rjmccall added a comment. In the abstract, it would be nice to warn about casts that always have UB because the address spaces don't overlap; however, I'm worried about how people might be using address spaces in strange ways today, knowing that they *do* overlap, only in ways that the compiler

[PATCH] D58634: [PR40778] Generate address space conversion when binding reference to a temporary value in different address space

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. This test file will probably grow over time, so please add a CHECK-LABEL line to the test case to make sure you're checking the body of foo(). Also, you'll probably need to allow the name

[PATCH] D58708: [PR40778] Preserve addr space in Derived to Base cast

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:2666 +? FromType->getPointeeType().getAddressSpace() +: FromType.getAddressSpace()); Please do a `getAs()` into a local and then use that h

[PATCH] D58346: [Sema] Change addr space diagnostics in casts to follow C++ style

rjmccall added inline comments. Comment at: lib/Sema/SemaCast.cpp:2293 + // define overlapping address spaces currently. + if (Self.getLangOpts().OpenCL) { +auto SrcType = SrcExpr.get()->getType(); Please structure this as an early exit. CHANGES SINCE LAS

[PATCH] D58708: [PR40778] Preserve addr space in Derived to Base cast

rjmccall added inline comments. Comment at: lib/Sema/SemaExpr.cpp:2671 DestType = Context.getPointerType(DestRecordType); FromRecordType = FromType->getPointeeType(); PointerConversions = true; And here. CHANGES SINCE LAST ACTION https://

[PATCH] D58729: Emit boxed expression as a compile-time constant if the expression inside the parentheses is a string literal

rjmccall added inline comments. Comment at: test/SemaObjC/boxing-illegal.m:70 + s = @(L"abc"); // expected-error {{illegal type 'int *' used in a boxed expression}} + s = @("\pabc"); // expected-error {{illegal type 'unsigned char *' used in a boxed expression}} +} --

[PATCH] D58708: [PR40778] Preserve addr space in Derived to Base cast

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

[PATCH] D58346: [Sema] Change addr space diagnostics in casts to follow C++ style

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

[PATCH] D58729: Emit boxed expression as a compile-time constant if the expression inside the parentheses is a string literal

rjmccall added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:322 def InvalidSourceEncoding : DiagGroup<"invalid-source-encoding">; +def InvalidUTF8String : DiagGroup<"invalid-utf8-string">; def KNRPromotedParameter : DiagGroup<"knr-promoted-parameter">; -

[PATCH] D58719: [PR40778][Sema] Adjust address space of operands in builtin operators

rjmccall added a comment. Looks great, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58719/new/ https://reviews.llvm.org/D58719 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D58729: Emit boxed expression as a compile-time constant if the expression inside the parentheses is a string literal

rjmccall added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:407 def ObjCFlexibleArray : DiagGroup<"objc-flexible-array">; +def ObjCBoxing : DiagGroup<"objc-boxing">; def OpenCLUnsupportedRGBA: DiagGroup<"opencl-unsupported-rgba">; Sure.

[PATCH] D58878: [Diagnostics] Warn for assignments in bool contexts

rjmccall added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:699 def ParenthesesOnEquality : DiagGroup<"parentheses-equality">; +def AssigmentInBoolContext : DiagGroup<"assigment-in-bool-context">; def Parentheses : DiagGroup<"parentheses", --

[PATCH] D58729: Emit boxed expression as a compile-time constant if the expression inside the parentheses is a string literal

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:922 +def warn_objc_boxing_invalid_utf8_string : Warning< + "string is ill-formed as UTF-8 and will become a nu

[PATCH] D58878: [Diagnostics] Warn for assignments in bool contexts

rjmccall added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:6738 + "assignment to bool">, + InGroup; // Completely identical except off by default. xbolva00 wrote: > rjmccall wrote: > > Sorry, I didn't mean to suggest that you should

[PATCH] D59234: [CodeGen][ObjC] Remove the leading 'l' from symbols for protocol metadata and protocol list

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Using internal linkage instead of private makes sense to me. Even if we wanted to use private linkage, it never made sense to be doing this with `\01l` instead of just setting the linkage

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. I remember this coming up 7-8 years ago. I intentionally decided against doing a copy/release when assigning to `__weak` because if the block wasn't already guaranteed to be copied then it was probably better to crash than to silently assign a value that's about to be

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. In D58514#1429514 , @wuhao5 wrote: > Hey guys, this is Hao, I am working with Chrome team to sort this issue out. > > In D58514#1428606 , @rjmccall wrote: > > > I remember this coming up 7-

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. In D58514#1429662 , @wuhao5 wrote: > > The specified user model of blocks is that they stay on the stack until > > they get copied, and there can be multiple such copies. ARC just automates > > that process. So the address of

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. In D58514#1429652 , @erik.pilkington wrote: > In D58514#1429606 , @rjmccall wrote: > > > There is no way in the existing ABI for copying a block to cause other > > references to the block

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. In D58514#1429758 , @wuhao5 wrote: > > Can I ask why you want a weak reference to a block in the first place? It > > seems basically useless — blocks can certainly appear in reference cycles, > > but I don't know why you'd ever

[PATCH] D57464: Generalize method overloading on addr spaces to C++

rjmccall added a comment. Sorry, I've gotten trapped under a backlog and haven't had time to think much about this. I don't want to block progress here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57464/new/ https://reviews.llvm.org/D57464

[PATCH] D59367: [Sema] Adjust address space of operands in remaining builtin operators

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. This patch LGTM. You might consider adding tests for weird cases like class types with conversion operators to reference types. To a certain extent that sort of thing is unimplementable

[PATCH] D59529: Refactor cast<>'s in if conditionals, which can only assert on failure.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59529/new/ https://reviews.llvm.org/D59529 ___ c

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. In D58514#1435228 , @wuhao5 wrote: > > Okay, so really just a block self-reference. We could really just add a > > feature for that that would avoid both the complexity and the expense of > > the self-capture dance. > > Is ther

[PATCH] D58514: Avoid needlessly copying blocks that initialize or are assigned to local auto variables to the heap

rjmccall added a comment. In D58514#1435431 , @wuhao5 wrote: > In D58514#1435296 , @rjmccall wrote: > > > In D58514#1435228 , @wuhao5 wrote: > > > > > > Okay, so really just

[PATCH] D59628: Add support for __attribute__((objc_class_stub))

rjmccall requested changes to this revision. rjmccall added inline comments. This revision now requires changes to proceed. Comment at: include/clang/Basic/AttrDocs.td:1118 +let Content = [{ +This attribute specifies that the Objective-C class to which it applies has dynamic

[PATCH] D59603: [PR40707][PR41011][OpenCL] Allow addr space spelling without double underscore in C++ mode

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

[PATCH] D59656: [CodeGen][ObjC] Annotate calls to objc_retainAutoreleasedReturnValue with notail on x86-64

rjmccall added inline comments. Comment at: lib/CodeGen/TargetInfo.h:161 + /// marked as 'notail'. + virtual bool noTailCallARCRetainRV() const { +return false; `shouldSuppressTailCallsOfRetainAutoreleasedReturnValue()`? Repository: rC Clang CHANGES SI

[PATCH] D59656: [CodeGen][ObjC] Annotate calls to objc_retainAutoreleasedReturnValue with notail on x86-64

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59656/new/ https://reviews.llvm.org/D59656 ___ cfe-commi

[PATCH] D59724: IRGen: Remove StructorType; thread GlobalDecl through more code. NFCI.

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

[PATCH] D59670: [Sema] Fix an assert when a block captures a constexpr local

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:15692 + MaybeODRUseExprSet LocalMaybeODRUseExprs; + std::swap(LocalMaybeODRUseExprs, MaybeODRUseExprs); + It looks like `SmallPtrSet`'s move constructor does actually guarantee to leave th

[PATCH] D59426: [PR41010][OpenCL] Allow OpenCL C style vector initialization in C++

rjmccall added a reviewer: rsmith. rjmccall added inline comments. Comment at: lib/Sema/SemaInit.cpp:1295 +? InitializedEntity::InitializeTemporary(ElemType) +: Entity; + There should at least be a comment here explaining this, but mostly

[PATCH] D58236: Make address space conversions a bit stricter.

rjmccall added a comment. I think C probably requires us to allow this under an explicit cast, but we can at least warn about it. It does not require us to allow this as an implicit conversion, and that should be an error. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58236/new/ http

[PATCH] D59670: [Sema] Fix an assert when a block captures a constexpr local

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaExpr.cpp:15692 + MaybeODRUseExprSet LocalMaybeODRUseExprs; + std::swap(LocalMaybeODRUseExprs, MaybeODRUseExprs); + e

[PATCH] D59646: [PR40778][PR41157][OpenCL] Prevent implicit initialization of local address space objects

rjmccall added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11648-11651 +// In OpenCL we don't allow to initialize objects in local address space. +if (getLangOpts().OpenCL && +Var->getType().getAddressSpace() == LangAS::opencl_local) + return;

[PATCH] D58236: Make address space conversions a bit stricter.

rjmccall added a comment. In D58236#1443556 , @ebevhan wrote: > In D58236#1443082 , @rjmccall wrote: > > > I think C probably requires us to allow this under an explicit cast, but we > > can at least warn about it.

[PATCH] D59628: Add support for __attribute__((objc_class_stub))

rjmccall added inline comments. Comment at: lib/CodeGen/CGObjCMac.cpp:7274 +// Classrefs pointing at Objective-C stub classes have the least +// significant bit set to 1. +auto *Tag = llvm::ConstantInt::get(CGM.IntPtrTy, 1); slavapestov wr

[PATCH] D59874: [PR41247] Fixed parsing of private keyword in C++

rjmccall added a comment. Otherwise LGTM. Comment at: test/SemaOpenCLCXX/private-access-specifier.cpp:2 +// RUN: %clang_cc1 %s -pedantic -verify -fsyntax-only + +struct B { Please include a comment explaining why this test file is in test/SemaOpenCLCXX/ (becau

[PATCH] D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++

rjmccall added inline comments. Comment at: lib/CodeGen/CGClass.cpp:2025 +ThisPtr = +Builder.CreatePointerBitCastOrAddrSpaceCast(This.getPointer(), NewType); } Anastasia wrote: > I am a bit unsure if `performAddrSpaceCast` should be used, but cons

[PATCH] D59806: [clang-tidy] Add a check for [super self] in initializers 🔍

rjmccall added a comment. In D59806#1448537 , @stephanemoore wrote: > In D59806#1447929 , @jordan_rose > wrote: > > > I don't think there's ever a reason to call `[super self]`, and doing so > > through a macro c

[PATCH] D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++

rjmccall added inline comments. Comment at: lib/CodeGen/CGClass.cpp:2025 +ThisPtr = +Builder.CreatePointerBitCastOrAddrSpaceCast(This.getPointer(), NewType); } brunodf wrote: > rjmccall wrote: > > Anastasia wrote: > > > I am a bit unsure if `perfo

[PATCH] D59628: Add support for __attribute__((objc_class_stub))

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclObjC.cpp:4131-4133 + if (!getLangOpts().ObjCRuntime.allowsClassStubs()) { +Diag(IntfDecl->getLocation(), diag::err_class_stub_not_supported); + } aaron.ballman wrote: > This should be

[PATCH] D60099: [CodeGen] Fix a regression by emitting lambda expressions in EmitLValue

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60099/new/ https://reviews.llvm.org/D60099 ___ cfe-commit

[PATCH] D59646: [PR40778][PR41157][OpenCL] Prevent implicit initialization of local address space objects

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

[PATCH] D59988: [PR41276] Generate address space cast of 'this' for objects attributed by an address space in C++

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

[PATCH] D60161: Expose non-trivial struct helpers for Swift.

rjmccall added inline comments. Comment at: clang/include/clang/CodeGen/CodeGenABITypes.h:93 +llvm::BasicBlock::iterator InsertPoint, llvm::Value *Dst, +CharUnits Alignment, QualType QT); + Hmm. I think it might be better to just have this return a funct

[PATCH] D59628: Add support for __attribute__((objc_class_stub))

rjmccall added inline comments. Comment at: include/clang/Basic/Attr.td:290 -class LangOpt { +class LangOpt { string Name = name; I think there's a grand total of one use of `negated`, so you might as well rewrite it to use `customCode`; see below. ==

[PATCH] D60302: [CodeGen][ObjC] Emit the retainRV marker as a module flag instead of named metadata.

rjmccall added a comment. Will the ARC-contract pass recognize both, or are you adding a BC upgrader that rewrites the global metadata into a module flag? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60302/new/ https://reviews.llvm.org/D60302 ___

[PATCH] D60456: [RISCV][WIP/RFC] Hard float ABI support

rjmccall added inline comments. Comment at: lib/Basic/Targets/RISCV.h:102 // TODO: support lp64f and lp64d ABIs. -if (Name == "lp64") { +if (Name == "lp64" || Name == "lp64f" || Name == "lp64d") { ABI = Name; You can remove the TODO here, assu

<    2   3   4   5   6   7   8   9   10   11   >