[PATCH] D53046: [Sema] Fix an error-on-valid with friends and templates

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D53046 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[PATCH] D53100: clang: Add ARCTargetInfo

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Repository: rC Clang https://reviews.llvm.org/D53100 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D50616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D53295: [OpenCL] Mark load of block invoke function as invariant

rjmccall added inline comments. Comment at: lib/CodeGen/CGBlocks.cpp:1318 +CGM.getModule().getMDKindID("invariant.load"), +llvm::MDNode::get(getLLVMContext(), None)); + OpenCL blocks are still potentially function-local, right? I don't think you

[PATCH] D53308: [Fixed Point Arithmetic] Fixed Point to Boolean Cast

rjmccall added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9599 + return false; +return Success(Val.getInt().getBoolValue(), E); + } I know you haven't really done constant-evaluation yet, but I think you should at least be setting up o

[PATCH] D53308: [Fixed Point Arithmetic] Fixed Point to Boolean Cast

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2026 +return EmitScalarConversion(Visit(E), E->getType(), DestTy, +CE->getExprLoc

[PATCH] D53295: Mark store and load of block invoke function as invariant.group

rjmccall added a reviewer: Prazek. rjmccall added a comment. In https://reviews.llvm.org/D53295#1271590, @Anastasia wrote: > Btw, blocks w/o captures are already optimized into regular calls? That's a very easy optimization for the optimizer to do because the global can be marked constant.

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

rjmccall added a comment. In https://reviews.llvm.org/D52615#1266320, @filcab wrote: > In https://reviews.llvm.org/D52615#1254567, @rsmith wrote: > > > This seems like an unreasonably long flag name. Can you try to find a > > shorter name for it? > > > `-fsanitize-poison-extra-operator-new`? >

[PATCH] D53604: [AST] Widen the bit-fields of Stmt to 8 bytes

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D53604 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D53605: [AST] Pack PredefinedExpr

rjmccall added inline comments. Comment at: include/clang/AST/Expr.h:1687 + // "Stmt *" for the predefined identifier. It is present if and only if + // hasFunctionName() is true and is in fact a "StringLiteral *". + "always" would be clearer than "in fact".

[PATCH] D53610: [AST] Check that GNU range case statements are correctly imported.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D53610 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D53607: [AST] Only store the needed data in IfStmt.

rjmccall added a comment. Yeah, I agree that changing child order is problematic. Repository: rC Clang https://reviews.llvm.org/D53607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm

[PATCH] D53674: [CodeGen] Fix assertion on referencing constexpr Obj-C object with ARC.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:2480 + SuppressResultRetain); } This switch is just checking what you already computed as `SuppressResultRetain`. Please just assert in the second case that th

[PATCH] D53609: [AST] Don't store data for GNU range case statement if not needed.

rjmccall added inline comments. Comment at: include/clang/AST/Stmt.h:956 + // with a range. Present if and only if caseStmtIsGNURange() is true. + enum { LHS, SUBSTMT, RHS, ELLIPSISLOC }; + enum { NumMandatoryStmtPtr = 2 }; ELLIPSISLOC is dead. Much like th

[PATCH] D53609: [AST] Don't store data for GNU range case statement if not needed.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D53609 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

rjmccall added a comment. In https://reviews.llvm.org/D52615#1275946, @filcab wrote: > In https://reviews.llvm.org/D52615#1272725, @rjmccall wrote: > > > In https://reviews.llvm.org/D52615#1266320, @filcab wrote: > > > > > In https://reviews.llvm.org/D52615#1254567, @rsmith wrote: > > > > > > > T

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

rjmccall added a comment. Oops, sorry for the unedited comment; it's fixed on Phab. Repository: rC Clang https://reviews.llvm.org/D52615 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-co

[PATCH] D53607: [AST] Only store the needed data in IfStmt.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D53607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D53715: [AST] Only store the needed data in WhileStmt.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D53715 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

rjmccall added a comment. So, three points: - That's not class-member-specific; you can have a placement `operator new[]` at global scope that isn't the special `void*` placement operator and therefore still has a cookie, and it would have just as much flexibility as a class-member override wo

[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic

rjmccall added a comment. I don't suppose there's any chance you can just tell Khronos to fix their stuff. It's a little funny to be more conservative about keywords in C++ when the C++ committee is actually much more aggressive about adding keywords in the non-reserved space than C is. Repo

[PATCH] D53725: [CodeGen] Move `emitConstant` from ScalarExprEmitter to CodeGenFunction. NFC.

rjmccall added a comment. This should at least be named `emitScalarConstant`. https://reviews.llvm.org/D53725 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D53674: [CodeGen] Fix assertion on referencing constexpr Obj-C object with ARC.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGObjC.cpp:2480 + SuppressResultRetain); } vsapsai wrote: > rjmccall wrote: > > This switch is just checking what you already computed as > > `SuppressResultRetain`. Plea

[PATCH] D53605: [AST] Refactor PredefinedExpr

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: include/clang/AST/Stmt.h:279 +/// in PredefinedExpr::IdentType. +unsigned Type : 4; + rjmccall wrote: > Since you're changing t

[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

rjmccall added inline comments. Comment at: lib/AST/Expr.cpp:1609 case CK_AddressSpaceConversion: -assert(getType()->isPointerType() || getType()->isBlockPointerType()); -assert(getSubExpr()->getType()->isPointerType() || - getSubExpr()->getType()->isBlockPoi

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

rjmccall added a comment. In https://reviews.llvm.org/D53738#1277172, @ebevhan wrote: > I want to float the idea again of adding an AST type to encapsulate an > arbitrary fixed-point semantic and using that as the 'common type' for binary > operations involving fixed-point types. This would ena

[PATCH] D53714: [AST] Only store the needed data in SwitchStmt.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Repository: rC Clang https://reviews.llvm.org/D53714 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/c

[PATCH] D53780: Fix bitcast to address space cast for coerced load/stores

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1260 llvm::ConstantInt::get(CGF.IntPtrTy, SrcSize), false); return CGF.Builder.CreateLoad(Tmp); The main reason why `llvm.memcpy` is an overloaded intrinsic is so that you can co

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

rjmccall added a comment. In https://reviews.llvm.org/D53738#1278528, @ebevhan wrote: > In https://reviews.llvm.org/D53738#1278078, @rjmccall wrote: > > > I don't think we should add *types* just for this, but if you need to make > > a different kind of `BinaryOperator` that represents that the

[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic

rjmccall added a comment. In https://reviews.llvm.org/D53705#1279068, @svenvh wrote: > Unlikely, since address spaces are provided in a different way in OpenCL C++ > vs OpenCL C. > > OpenCL C provides qualifiers such as `global` as part of the language. > OpenCL C++ provides template classes s

[PATCH] D53860: [SemaCXX] Don't check base's dtor is accessible

rjmccall added a comment. That's interesting. If you think of a list-initialization of an aggregate as effectively defining an *ad hoc* constructor for it, then yes, we clearly ought to have access to protected destructors of base classes. And that aligns with the intuition that people make t

[PATCH] D52615: Handle -fsanitize-address-poison-class-member-array-new-cookie in the driver and propagate it to cc1

rjmccall added a comment. Thanks, and I agree with renaming the existing option. Comment at: docs/ClangCommandLineReference.rst:805 -Enable poisoning array cookies when using class member operator new\[\] in AddressSanitizer +Enable poisoning array cookies when using custom

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

rjmccall added a comment. In https://reviews.llvm.org/D53738#1280193, @ebevhan wrote: > In https://reviews.llvm.org/D53738#1279069, @rjmccall wrote: > > > > The important difference would be that we separate the semantics of > > > performing the conversion and the operation. I understand that ad

[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic

rjmccall added a comment. In https://reviews.llvm.org/D53705#1280264, @Anastasia wrote: > In https://reviews.llvm.org/D53705#1279086, @rjmccall wrote: > > > In https://reviews.llvm.org/D53705#1279068, @svenvh wrote: > > > > > Unlikely, since address spaces are provided in a different way in OpenC

[PATCH] D53725: [CodeGen] Move `emitConstant` from ScalarExprEmitter to CodeGenFunction. NFC.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Well, that's the old style, but we've been slowly moving to the camelCase style instead. Very, very slowly. I won't hold up your patch over it. https://reviews.llvm.org/D53725 __

[PATCH] D53705: [OpenCL] Postpone PSV address space diagnostic

rjmccall added a comment. In https://reviews.llvm.org/D53705#1281738, @keryell wrote: > In https://reviews.llvm.org/D53705#1278066, @rjmccall wrote: > > > I don't suppose there's any chance you can just tell Khronos to fix their > > stuff. It's a little funny to be more conservative about keywo

[PATCH] D52674: [AST] Add Obj-C discriminator to MS ABI RTTI

rjmccall added a comment. In https://reviews.llvm.org/D52674#1271814, @smeenai wrote: > @rjmccall I prototyped the ForRTTI parameter approach in > https://reviews.llvm.org/D53546. It could definitely be cleaned up a bit, but > it demonstrates the problems I saw with the added parameter. Namely,

[PATCH] D56066: [OpenCL] Address space for default class members

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

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. It's pretty unfortunate that all these fields have to be individually called out like this. Can you move all these basic layout fields into a separate `struct` (which can be a secondary base class of `TargetInfo`) which can then just be normally copied? Anything that

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

rjmccall added a comment. In D53153#1353256 , @scott.linder wrote: > In D53153#1317977 , @rjmccall wrote: > > > I think `-fvisibility=hidden` isn't good enough because you want to infer > > hidden visibility even

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

rjmccall added a comment. Mostly, we don't really want the abstract visibility calculation to change whenever we see a definition. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53153/new/ https://reviews.llvm.org/D53153 ___ cfe-commits mai

[PATCH] D55394: Re-order type param children of ObjC nodes

rjmccall added a comment. I don't really have an opinion about this, sorry. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55394/new/ https://reviews.llvm.org/D55394 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D55394: Re-order type param children of ObjC nodes

rjmccall added a comment. This is the AST dumper. This is not a user feature. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55394/new/ https://reviews.llvm.org/D55394 ___ cfe-commits mailing list cfe-commits@lists.l

[PATCH] D55394: Re-order type param children of ObjC nodes

rjmccall added a comment. If I'm debugging a clang bug and calling `D->dump()`, I think it'll be just as clear as it used to be what the `ObjCTypeParamDecl` lines mean. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55394/new/ https://reviews.llvm.org/D55394 _

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

rjmccall added a comment. Yeah, I would recommend splitting the `APFixedPoint` in `APValue` changes into a separate patch. Comment at: clang/lib/AST/ExprConstant.cpp:9959 + } + llvm_unreachable("unknown cast resulting in fixed point value"); +} leonardchan w

[PATCH] D56735: [OpenCL] Fix overloading ranking rules to work correctly for address space coversions

rjmccall added a comment. Is there a reason not to just do `T1.getQualifiers() == T2.getQualifiers()`? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56735/new/ https://reviews.llvm.org/D56735 ___ cfe-commits mailing list cfe-commits@lists.l

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

rjmccall added a comment. In D53738#172 , @rjmccall wrote: > In D53738#1333276 , @leonardchan > wrote: > > > In D53738#1326071 , @rjmccall > > wrote: > > > > > I'm fine

[PATCH] D56746: [Fixed Point Arithmetic] Add APFixedPoint to APValue

rjmccall added a comment. One nit-pick, then this is good to go. Comment at: clang/include/clang/AST/APValue.h:183 +setFixedPoint(std::move(FX)); + } explicit APValue(const APValue *E, unsigned N) : Kind(Uninitialized) { I know this is the established p

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

rjmccall added a comment. Okay, so is this ready to re-review independently? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55850/new/ https://reviews.llvm.org/D55850 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.

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

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I think that's the right direction, yeah. I thought I told you it was fine to commit this patch under that assumption awhile ago. Did I just never click "accept"? Repository: rC Clang

[PATCH] D56746: [Fixed Point Arithmetic] Add APFixedPoint to APValue

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

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

rjmccall added a comment. It's an interesting idea at least, and it does seem like there ought to be some way to express it, at least internally. A `-cc1` option is fine for your purposes, right? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53153/new/ https://reviews.llvm.org/D53153

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

rjmccall added a comment. In D56411#1352602 , @yaxunl wrote: > In D56411#1352332 , @rjmccall wrote: > > > This patch still doesn't make any sense. You don't need to do any special > > validation when passing a fun

[PATCH] D56735: [OpenCL] Fix overloading ranking rules to work correctly for address space conversions

rjmccall added a comment. In D56735#1359590 , @Anastasia wrote: > In D56735#1358222 , @rjmccall wrote: > > > Is there a reason not to just do `T1.getQualifiers() == T2.getQualifiers()`? > > > I tried this but it cau

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

rjmccall added a comment. I think the diagnostic should come during instantiation when you find an evaluated use of a host function within a device function. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56411/new/ https://reviews.llvm.org/D56411

[PATCH] D56792: Rename getTypeQualifiers to getMethodQualifiers

rjmccall accepted this revision. rjmccall added a reviewer: rsmith. rjmccall added a comment. LGTM, but I'd like Richard to also sign off on the name. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56792/new/ https://reviews.llvm.org/D56792

[PATCH] D56735: [OpenCL] Fix overloading ranking rules to work correctly for address space conversions

rjmccall added a comment. Thanks, LGTM. Well, actually, could you improve the test case to verify that the right overload is called? This should be easy because C++ type-checking is bottom-up: just make the functions return something distinguishable, e.g. `struct`s that declare different memb

[PATCH] D55844: [Fixed Point Arithmetic] Fixed Point Subtraction

rjmccall added a comment. I don't pay attention to "ready to land" because I assume that you're verifying that your patch actually works as promised in practice, at least as far as the tests are concerned (and presumably my review catches deeper issues). If there are substantial changes that la

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

rjmccall added inline comments. Comment at: lib/Parse/ParseDecl.cpp:6170 + } +} + } Does this not need to diagnose redundant qualifiers? Why is this path required in addition to the path in SemaType, anyway? Comment at:

[PATCH] D56802: [CodeGenObjC] Treat ivar offsets variables as constant if they refer to ivars of a direct subclass of NSObject with an @implementation

rjmccall added a comment. We've tossed around the idea of doing things like this before, but I was hoping that it wouldn't have to be specific to `NSObject` and that we'd e.g. have an attribute that guarantees that the `@interface` declares all the ivars for a class. Are we still thinking that

[PATCH] D56802: [CodeGenObjC] Treat ivar offsets variables as constant if they refer to ivars of a direct subclass of NSObject with an @implementation

rjmccall added a comment. Also, we should just go ahead and skip using the offset variable when emitting the ivar access in the frontend. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56802/new/ https://reviews.llvm.org/D56802

[PATCH] D56802: [CodeGenObjC] Treat ivar offsets variables as constant if they refer to ivars of a direct subclass of NSObject with an @implementation

rjmccall added a comment. Emitting the global as `const` is probably still a good idea; if the global actually gets mapped as a constant, we'll end up making a dynamic assertion that we got the offset right. It's possible that the section will inhibit that, though. CHANGES SINCE LAST ACTION

[PATCH] D56821: [CodeGen] Always use string computed in Sema for PredefinedExpr

rjmccall added a comment. I agree that this is the right thing to do. If we want to make those improvements, they're doable within Sema. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56821/new/ https://reviews.llvm.org/D56821

[PATCH] D56802: [CodeGenObjC] Treat ivar offsets variables as constant if they refer to ivars of a direct subclass of NSObject with an @implementation

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

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

rjmccall added a comment. I don't want this to be a driver option because I don't want to design a general-purpose feature for this right now, nor do I want to gradually accrete a general-purpose feature around a random collection of needs accumulated from other features. Let's just leave it a

[PATCH] D56868: Add -fset-visibility-for-decls for -cc1

rjmccall added inline comments. Comment at: include/clang/Driver/CC1Options.td:707 +def fset_visibility_for_decls : Flag<["-"], "fset-visibility-for-decls">, + HelpText<"Apply global symbol visibility to declarations without an explicit visibility">; def ftemplate_depth : Sepa

[PATCH] D56735: [OpenCL] Fix overloading ranking rules to work correctly for address space conversions

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

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

rjmccall added inline comments. Comment at: lib/Parse/ParseDecl.cpp:6170 + } +} + } Anastasia wrote: > rjmccall wrote: > > Does this not need to diagnose redundant qualifiers? Why is this path > > required in addition to the path in SemaTy

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

rjmccall added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9839 +return Success(Result, E); + } + Can most of this reasonably be a method on `APFixedPoint`? (And likewise for `CK_IntegralToFixedPoint`.) If nothing else, I suspect you are g

[PATCH] D56868: Add -fextern-visibility for -cc1

rjmccall added a comment. I'm sorry to keep jerking you around, but let's spell out the flag a bit more: `-fapply-global-visibility-to-externs`. No reason not to be totally clear here. Thank you for updating the fields and descriptions, those all look good now. CHANGES SINCE LAST ACTION h

[PATCH] D55868: [Fixed Point Arithmetic] Fixed Point Addition Constant Expression Evaluation

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

[PATCH] D56868: Add -fapply-global-visibility-to-externs for -cc1

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, I appreciate you bearing with me on this. LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56868/new/ https://reviews.llvm.org/D56868 __

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

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

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

rjmccall added a comment. In D56411#1365727 , @yaxunl wrote: > In D56411#1360010 , @rjmccall wrote: > > > I think the diagnostic should come during instantiation when you find an > > evaluated use of a host functio

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

rjmccall added a comment. I'll just add that we've been trying to not put things behind OpenCL guards as much as possible. Most of the remaining OpenCL checks are for OpenCL-specific logic like inferring the default address space, and yeah, we could probably make that a target option or someth

[PATCH] D57101: [AST] Fix addr space of result type for dereference operator

rjmccall added a comment. What? No, the l-value should still be qualified like the pointee type. An l-value-to-r-value conversion should remove the qualifier, but not just a dereference. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57101/new/ https://reviews.llvm.org/D57101

[PATCH] D57072: Don't codegen an unreachable return block

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Sure, I have no objection to doing this at this point. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57072/new/ https://reviews.llvm.org/D57072

[PATCH] D55850: [OpenCL] Allow address spaces as method qualifiers

rjmccall added a comment. In D55850#1368767 , @Anastasia wrote: > In D55850#1366709 , @rjmccall wrote: > > > Most of the remaining OpenCL checks are for OpenCL-specific logic like > > inferring the default address

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:52 -/// Exposes information about the current target. -/// -class TargetInfo : public RefCountedBase { - std::shared_ptr TargetOpts; - llvm::Triple Triple; -protected: - // Target values set by the

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

rjmccall added a comment. In D56411#1369906 , @yaxunl wrote: > In D56411#1365745 , @rjmccall wrote: > > > In D56411#1365727 , @yaxunl wrote: > > > > > In D56411#1360010

[PATCH] D57188: Disable _Float16 for non ARM/SPIR Targets

rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:497 defined by the C standards committee, so using ``_Float16`` will not prevent -code from being ported to architectures other than Arm. Also, ``_Float16`` -arithmetic and operations will directly map on h

[PATCH] D57188: Disable _Float16 for non ARM/SPIR Targets

rjmccall added a subscriber: scanon. rjmccall added a comment. This LGTM with one minor revision; feel free to commit with that. For follow-up commit consideration: @scanon, do we want to support `_Float16` anywhere else? Do we need to lock down an ABI here for i386/x86_64 in advance of those

[PATCH] D57226: [Fixed Point] [AST] Add an AST serialization code for fixed-point literals.

rjmccall added inline comments. Comment at: include/clang/Serialization/ASTBitCodes.h:1637 + /// A FixedPointLiteral record. + EXPR_FIXEDPOINT_LITERAL, + ebevhan wrote: > I'm unsure if this is the right location for a new code. This will bump down > al

[PATCH] D57188: Disable _Float16 for non ARM/SPIR Targets

rjmccall added inline comments. Comment at: docs/LanguageExtensions.rst:484 +- SPIR +``_Float16`` will be supported on more targets as they define ABIs for them. + rjmccall wrote: > "them" should be "it" here. Sorry, apparently Phabricator was sitting on a commen

[PATCH] D57188: Disable _Float16 for non ARM/SPIR Targets

rjmccall added a comment. "it" is the grammatically correct word. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57188/new/ https://reviews.llvm.org/D57188 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:

[PATCH] D57188: Disable _Float16 for non ARM/SPIR Targets

rjmccall added a comment. In D57188#1374890 , @yaxunl wrote: > This change causes regressions for CUDA/HIP. As single-source language, > CUDA/HIP code contains both device and host code. It has separate compilation > for host and device. > In host compi

[PATCH] D57188: Disable _Float16 for non ARM/SPIR Targets

rjmccall added a comment. Regardless, I think it would be fine if you wanted to add a CUDA/HIP exception to this check in the short term. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57188/new/ https://reviews.llvm.org/D57188 _

[PATCH] D57369: [CUDA][HIP] Do not diagnose use of _Float16

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

[PATCH] D54604: Automatic variable initialization

rjmccall added a comment. I think it's absolutely fair game to file bugs for this. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54604/new/ https://reviews.llvm.org/D54604 ___ cfe-commits mailing list cfe-commits@list

[PATCH] D57101: [OpenCL] Add generic addr space to the return of implicit assignment

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

[PATCH] D57390: OpenCL: Don't promote vector args to printf

rjmccall added inline comments. Comment at: include/clang/AST/FormatString.h:70 AsShort, // 'h' +AsInt,// 'hl' (OpenCL float/int vector element) AsLong, // 'l' I think giving this a weird name like `AsShortLong` might help make it

[PATCH] D57390: OpenCL: Use length modifier for warning on vector printf arguments

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

[PATCH] D57405: Revert "OpenCL: Extend argument promotion rules to vector types"

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

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

rjmccall added a comment. One minor change and then LGTM. Comment at: include/clang/Basic/TargetInfo.h:1352 + /// Copy type and layout related info. + void copyAuxTarget(TargetInfo *Aux); virtual uint64_t getPointerWidthV(unsigned AddrSpace) const { This c

[PATCH] D56318: [HIP] Fix size_t for MSVC environment

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

[PATCH] D56555: Add Attribute to define nonlazy objc classes

rjmccall added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:3506 +add the class to the list of non-lazily initialized classes. These are classes +that are realized at objc_init() time rather than when first referenced. + }]; aaron.ballman

[PATCH] D57524: Fix ICE on attempt to add an addr space qual to a type qualified by an addr space

rjmccall added inline comments. Comment at: lib/Sema/SemaInit.cpp:4693 +T2Quals.addAddressSpace(AS2); + QualType WithAScv1T4 = S.Context.getQualifiedType(IgnoreAScv2T2, T1Quals); + Sequence.AddQualificationConversionStep(WithAScv1T4, ValueKind); ---

[PATCH] D57527: Do not copy floating pointer format from aux target

rjmccall added a comment. Okay, so you silently have an incompatible ABI for anything in the system headers that mentions `long double`. Do you have any plans to address or work around that, or is the hope that it just doesn't matter? I feel like this should be a special case for AMDGPU rather

[PATCH] D57527: Do not copy floating pointer format from aux target

rjmccall added a comment. In D57527#1379088 , @yaxunl wrote: > In D57527#1379065 , @rjmccall wrote: > > > Okay, so you silently have an incompatible ABI for anything in the system > > headers that mentions `long do

[PATCH] D57527: Do not copy long double and 128-bit fp format from aux target for AMDGPU

rjmccall added a comment. Explanatory comment, please. Otherwise LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57527/new/ https://reviews.llvm.org/D57527 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/

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