[PATCH] D44984: [HIP] Add hip input kind and codegen for kernel launching

rjmccall added a comment. Otherwise LGTM. Comment at: lib/CodeGen/CGCUDANV.cpp:51-52 llvm::Constant *getLaunchFn() const; + std::string addPrefixToName(CodeGenModule &CGM, StringRef FuncName) const; + std::string addUnderscoredPrefixToName(CodeGenModule &CGM, +

[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

rjmccall added a comment. I'm not sure that doing this in the lexer is appropriate; you should just diagnose the unsupported feature in Sema, or at best the parser. Repository: rC Clang https://reviews.llvm.org/D46022 ___ cfe-commits mailing lis

[PATCH] D45179: [libc++] Add _LIBCPP_FORCE_NODISCARD define to force-enable nodiscard in pre-C++17

rjmccall added a comment. In https://reviews.llvm.org/D45179#1077048, @mclow.lists wrote: > In https://reviews.llvm.org/D45179#1056183, @rjmccall wrote: > > > Is Marshall arguing that the standard doesn't allow compilers to warn about > > failing to use these function results prior to C++17? Be

[PATCH] D44984: [HIP] Add hip input kind and codegen for kernel launching

rjmccall accepted this revision. rjmccall added a comment. Thank you. https://reviews.llvm.org/D44984 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45601: Warn on bool* to bool conversion

rjmccall added a comment. I agree that this is one of those warnings that just can't be done in any reasonable way. https://reviews.llvm.org/D45601 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listin

[PATCH] D46042: Cap vector alignment at 16 for all Darwin platforms

rjmccall created this revision. Herald added a reviewer: javed.absar. Herald added subscribers: cfe-commits, kristof.beyls. This fixes two major problems: - We were not capping vector alignment as desired on 32-bit ARM. - We were using different alignments based on the AVX settings on Intel, so w

[PATCH] D45755: [Sema] Do not match function type with const T in template argument deduction

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

[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

rjmccall added a comment. Yeah, if there are actually differences in the set of keywords, that's a totally reasonable thing to handle in the lexer. Comment at: include/clang/Basic/TokenKinds.def:255 +// KEYNOOPENCL - This is a keyword that is not supported in OpenCL C +//

[PATCH] D46052: GNUstep Objective-C ABI version 2

rjmccall added a comment. Are you asking for a code review or a design review of the ABI? The second would be much easier to do from a design document. Comment at: lib/CodeGen/CGObjCGNU.cpp:502 +for (const auto *I : Methods) + if (I->getImplementationControl() == Obj

[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

rjmccall added inline comments. Comment at: include/clang/Basic/TokenKinds.def:255 +// KEYNOOPENCL - This is a keyword that is not supported in OpenCL C +// nor in OpenCL C++. // KEYALTIVEC - This is a keyword in AltiVec svenvh wrote: > rjm

[PATCH] D46052: GNUstep Objective-C ABI version 2

rjmccall added inline comments. Comment at: lib/CodeGen/CGObjCGNU.cpp:961 + GV->setSection(Section); +return GV; + } I'd encourage you to use ConstantBuilder whenever you would want to use this. Comment at: lib/CodeGen/CGObjCGNU.cpp:9

[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

rjmccall added inline comments. Comment at: lib/Parse/ParseStmtAsm.cpp:696 +return StmtError(); + } + svenvh wrote: > rjmccall wrote: > > You might consider parsing the statement normally and then just diagnosing > > after the fact, maybe in Sema. You'd ha

[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

rjmccall added inline comments. Comment at: lib/Parse/ParseStmtAsm.cpp:696 +return StmtError(); + } + svenvh wrote: > rjmccall wrote: > > svenvh wrote: > > > rjmccall wrote: > > > > You might consider parsing the statement normally and then just > > > > dia

[PATCH] D45382: [CodeGen] Avoid destructing a struct type that has already been destructed by a delegated constructor

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/D45382 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

[PATCH] D46218: PR37275 packed attribute should not apply to base classes

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I wonder if that was originally just an oversight that got turned into official policy. Anyway, if it's the policy, it's what we have to do; LGTM. Have you checked whether we do the right

[PATCH] D45900: CodeGen: Fix invalid bitcast for lifetime.start/end

rjmccall added a comment. Oh, I see, it's not that the lifetime intrinsics don't handle pointers in the alloca address space, it's that we might have already promoted them into `DefaultAS`. Do the LLVM uses of lifetime intrinsics actually look through these address space casts? I'm wondering

[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

rjmccall added inline comments. Comment at: lib/Parse/ParseStmtAsm.cpp:696 +return StmtError(); + } + svenvh wrote: > rjmccall wrote: > > svenvh wrote: > > > rjmccall wrote: > > > > svenvh wrote: > > > > > rjmccall wrote: > > > > > > You might consider parsi

[PATCH] D46241: [CodeGen] Recognize more cases of zero initialization

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprConstant.cpp:1395 +static bool isZeroInitializer(ConstantEmitter &CE, const Expr *Init) { + QualType InitTy = Init->getType().getCanonicalType(); You should have a comment here clarifying that this

[PATCH] D46022: [OpenCL] Restrict various keywords in OpenCL C++ mode

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

[PATCH] D46241: [CodeGen] Recognize more cases of zero initialization

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprConstant.cpp:1403 + if (auto *IL = dyn_cast_or_null(Init)) { +if (InitTy->isConstantArrayType()) { + for (auto I : IL->inits()) Do you actually care if it's an array initialization instead of

[PATCH] D46042: Cap vector alignment at 16 for all Darwin platforms

rjmccall added a comment. I think we should seriously consider making alignment attributes on typedefs (and maybe some other attributes like may_alias) actual type qualifiers that are preserved in the canonical type, mangled, and so on. It would be an ABI break, but it'd also solve a lot of pr

[PATCH] D46386: Adding __atomic_fetch_min/max intrinsics to clang

rjmccall added a comment. Given that these are overloaded builtins, why are there separate builtins for `min` and `umin`? If this is a Clang extension, it needs to be documented in our extensions documentation. The documentation should describe the exact ordering semantics, to the extent we w

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added inline comments. Comment at: lib/Sema/SemaDeclCXX.cpp:8931 + /*ConstArg*/ true, false, false, false, false); + auto *CopyCtor = cast_or_null(SMI.getMethod()); + Sorry, I didn't realize you'd go off and write this code manu

[PATCH] D45944: Disallow pointers to const in __sync_fetch_and_xxx

rjmccall added a comment. LGTM, although you might consider testing a broader set of builtins. Maybe at least one from the `__atomic_*` family? https://reviews.llvm.org/D45944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.ll

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added a comment. In https://reviews.llvm.org/D45476#1087487, @EricWF wrote: > In https://reviews.llvm.org/D45476#1087446, @cfe-commits wrote: > > > I think you and Richard agreed that you weren’t going to synthesize a whole > > expression tree at every use of the operator, and I agree w

[PATCH] D46042: Cap vector alignment at 16 for all Darwin platforms

rjmccall added a comment. In https://reviews.llvm.org/D46042#1088044, @ab wrote: > So, this makes sense to me, but on x86, should we also be worried about the > fact that the calling convention is based on which features are available? > (>128bit ext_vector_types are passed in AVX/AVX-512 regi

[PATCH] D46042: Cap vector alignment at 16 for all Darwin platforms

rjmccall added a comment. In https://reviews.llvm.org/D46042#1088049, @scanon wrote: > In https://reviews.llvm.org/D46042#1088044, @ab wrote: > > > So, this makes sense to me, but on x86, should we also be worried about the > > fact that the calling convention is based on which features are avai

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added a comment. In https://reviews.llvm.org/D45476#1088180, @EricWF wrote: > OK, As I see it, we have two choices: > > (1) Stash the expressions in Sema and import them like > > In https://reviews.llvm.org/D45476#1088047, @rjmccall wrote: > > > I'm sorry, but this is just not true. The

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added a comment. In https://reviews.llvm.org/D45476#1088189, @rsmith wrote: > This is significantly more complexity than we need. We're talking about > constexpr variables here, so we just need a `VarDecl* -- you can then ask > that declaration for its evaluated value and emit that dir

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:1986 + /// This object needs to be initialized by Sema the first time it checks + /// a three-way comparison. + ComparisonCategories CompCategories; Is this comment accurate? Because i

[PATCH] D45174: non-zero-length bit-fields should make a class non-empty

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

[PATCH] D46487: [HIP] Diagnose unsupported host triple

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

[PATCH] D46471: [HIP] Add hip offload kind

rjmccall added a comment. Otherwise LGTM. Comment at: lib/Driver/Compilation.cpp:201 + // not compiled again if there are already failures. It is OK to abort the + // CUDA pipeline on errors. + if (A->isOffloading(Action::OFK_Cuda) || A->isOffloading(Action::OFK_HIP)) --

[PATCH] D46472: [HIP] Support offloading by linkscript

rjmccall added a comment. The IRGen aspects of this seem reasonable, modulo a comment change. I don't feel qualified to judge the driver change. Comment at: lib/CodeGen/CGCUDANV.cpp:317 + if (GpuBinaryFileName.empty() && !IsHIP) return nullptr; Is thi

[PATCH] D46473: [HIP] Let clang-offload-bundler support HIP

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

[PATCH] D46475: [HIP] Set proper triple and offload kind for the toolchain

rjmccall added inline comments. Comment at: lib/Driver/Driver.cpp:554 + }) || + IsHIP) { const ToolChain *HostTC = C.getSingleOffloadToolChain(); It seems to me that it wouldn't be too hard to do your TODO here; it's basically just ch

[PATCH] D46489: [HIP] Let assembler output bitcode for amdgcn

rjmccall added a comment. I think the right solution here is to make a CompileJobAction with type TY_LLVM_BC in the first place. You should get the advice of a driver expert, though. https://reviews.llvm.org/D46489 ___ cfe-commits mailing list cf

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added inline comments. Comment at: include/clang/AST/ComparisonCategories.h:71 + /// standard library. The key is a value of ComparisonCategoryResult. + mutable llvm::DenseMap Objects; + EricWF wrote: > rjmccall wrote: > > We expect this map to have at

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:964 +RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer(); +break; + case TEK_Complex: It looks like we don't actually support any aggregate types here, which I think is fine beca

[PATCH] D46241: [CodeGen] Recognize more cases of zero initialization

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprConstant.cpp:1403 + if (auto *IL = dyn_cast_or_null(Init)) { +if (InitTy->isConstantArrayType()) { + for (auto I : IL->inits()) sepavloff wrote: > rjmccall wrote: > > Do you actually care if

[PATCH] D46471: [HIP] Add hip offload kind

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

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprAgg.cpp:964 +RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer(); +break; + case TEK_Complex: EricWF wrote: > EricWF wrote: > > rjmccall wrote: > > > It looks like we don't actually suppo

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Fine, that works. https://reviews.llvm.org/D45476 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailm

[PATCH] D46052: GNUstep Objective-C ABI version 2

rjmccall added a comment. Thanks, the comments help a lot. Comment at: lib/CodeGen/CGObjCGNU.cpp:439 + ArrayRef IvarOffsets, + ArrayRef IvarAlign, + ArrayRef IvarOwnership); the

[PATCH] D48589: [WIP] [CodeGen] Allow specifying Extend to CoerceAndExpand

rjmccall added inline comments. Comment at: include/clang/CodeGen/CGFunctionInfo.h:90 union { -unsigned DirectOffset; // isDirect() || isExtend() -unsigned IndirectAlign;// isIndirect() -unsigned AllocaFieldIndex; // isInAlloca() +llvm::StructType *Exte

[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types

rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:1954 + llvm::APInt getFixedPointMin(QualType Ty) const; + llvm::APInt getFixedPointOne(QualType Ty) const; ebevhan wrote: > rjmccall wrote: > > Are these opaque bit-patterns? I think

[PATCH] D48589: [WIP] [CodeGen] Allow specifying Extend to CoerceAndExpand

rjmccall added inline comments. Comment at: include/clang/CodeGen/CGFunctionInfo.h:90 union { -unsigned DirectOffset; // isDirect() || isExtend() -unsigned IndirectAlign;// isIndirect() -unsigned AllocaFieldIndex; // isInAlloca() +llvm::StructType *Exte

[PATCH] D48456: [Fixed Point Arithmetic] Casting between fixed point types and other arithmetic types

rjmccall added inline comments. Comment at: lib/CodeGen/CGExprScalar.cpp:768 +if (CGF.getContext().getTargetInfo().unsignedFixedPointTypesHavePadding() && +Ty->isUnsignedFixedPointType()) { + unsigned NumBits = CGF.getContext().getTypeSize(Ty);

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

rjmccall added inline comments. Comment at: include/clang/Basic/FixedPoint.h:11 +/// \file +/// Defines the fixed point number interface. +// Referencing the standard here would be good, and maybe even excerpting the key parts. Comment at: inc

[PATCH] D46915: [Fixed Point Arithmetic] Fixed Point Precision Bits and Fixed Point Literals

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:90 + // by default. + bool SameFBits; + Sorry for the extremely late review, but this really needs to be renamed. Please remember that other compiler maintainers are not experts in t

[PATCH] D48589: [WIP] [CodeGen] Allow specifying Extend to CoerceAndExpand

rjmccall added a comment. In https://reviews.llvm.org/D48589#1144976, @rogfer01 wrote: > @rjmccall because we do not want to impact the clients of `ABIArgInfo` I > thought of two possible approaches > > 1. extend `CGFunctionInfo` with a third trailing array (now it has two), with > as many elem

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

rjmccall added inline comments. Comment at: include/clang/Basic/FixedPoint.h:23 + +class FixedPointNumber { + public: ebevhan wrote: > rjmccall wrote: > > The established naming convention here — as seen in `APInt`, `APFloat`, > > `APValue`, etc. — would call th

[PATCH] D48727: [Fixed Point Arithmetic] Rename `-fsame-fbits` flag

rjmccall added a comment. This all looks reasonable except that I think the interpretation is exactly backwards, no? From the documentation on the option, `-fpadding-on-unsigned-fixed-point` causes there to be padding, i.e. the inverse of the old `SameFBits`; but the default value, comments, a

[PATCH] D48727: [Fixed Point Arithmetic] Rename `-fsame-fbits` flag

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D48727#1146873, @leonardchan wrote: > Oh, having the same number of fractional bits is what leads to unsigned types > having one bit of padding, and vice versa.

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

rjmccall added inline comments. Comment at: include/clang/Basic/FixedPoint.h:31 + SatNoPadding, +}; + I figured you'd want this to be a struct which include the scale, width, signed-ness, and saturating-ness; and then `APFixedPoint` can just store one of these

[PATCH] D47299: [CodeGenCXX] Emit strip.invariant.group with -fstrict-vtable-pointers

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1647 + } +} + Prazek wrote: > rsmith wrote: > > Prazek wrote: > > > rjmccall wrote: > > > > Prazek wrote: > > > > > rjmccall wrote: > > > > > > Incidentally, how do you protec

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

rjmccall added inline comments. Comment at: include/clang/Basic/FixedPoint.h:31 + SatNoPadding, +}; + leonardchan wrote: > ebevhan wrote: > > rjmccall wrote: > > > I figured you'd want this to be a struct which include the scale, width, > > > signed-ness, and s

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

rjmccall added a comment. LGTM, but I'd like Richard to sign off, too. Repository: rC Clang https://reviews.llvm.org/D45898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

rjmccall added a comment. Thanks, much appreciated. A couple more style changes that I noticed and this will be LGTM, although you should also make sure you have Bevin Hansson's approval. Comment at: include/clang/AST/ASTContext.h:33 #include "clang/Basic/AddressSpaces.h" +

[PATCH] D44539: [Sema][Objective-C] Add check to warn when property of objc type has assign attribute

rjmccall added inline comments. Comment at: test/SemaObjC/property-in-class-extension-1.m:18 -@property (assign, readonly) NSString* changeMemoryModel; // expected-note {{property declared here}} +@property (unsafe_unretained, readonly) NSString* changeMemoryModel; // expecte

[PATCH] D49083: [HIP] Register/unregister device fat binary only once

rjmccall added inline comments. Comment at: lib/CodeGen/CGCUDANV.cpp:444 +auto HandleValue = +CtorBuilder.CreateAlignedLoad(GpuBinaryHandle, CGM.getPointerAlign()); +llvm::Constant *Zero = llvm::Constant::getNullValue(HandleValue->getType()); Do

[PATCH] D49294: Sema: Fix explicit address space cast in C++

rjmccall added inline comments. Comment at: lib/Sema/SemaCast.cpp:2171 + ->getPointeeType() + .getAddressSpace()) { +Kind = CK_AddressSpaceConversion; Please extract t

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

rjmccall added inline comments. Comment at: include/clang/AST/ASTContext.h:1953 unsigned char getFixedPointIBits(QualType Ty) const; + FixedPointSemantics getFixedPointSema(QualType Ty) const; + APFixedPoint getFixedPointMax(QualType Ty) const; rjmccall wrot

[PATCH] D49403: More aggressively complete RecordTypes with Function Pointers

rjmccall added a comment. Can you explain why this is important for the optimizer? https://reviews.llvm.org/D49403 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47233: [CodeGen] Emit MSVC RTTI for Obj-C EH types

rjmccall added inline comments. Comment at: lib/CodeGen/CGObjCMac.cpp:7457-7460 CGObjCNonFragileABIMac::GetEHType(QualType T) { // There's a particular fixed type info for 'id'. if (T->isObjCIdType() || T->isObjCQualifiedIdType()) { +if (CGM.getTriple().isWindowsMSVCEn

[PATCH] D49085: [Sema] Emit a diagnostic for an invalid dependent function template specialization

rjmccall added a comment. This is the right basic approach. I think it would be better if the diagnostic text was more like `err_function_template_spec_no_match`, maybe "no candidate function template was found for dependent friend function template specialization". And it would be good to em

[PATCH] D49303: [CodeGen][ObjC] Treat non-escaping blocks as global blocks to make copy/dispose a no-op

rjmccall added a comment. Please test that we still copy captures correctly into the block object and destroy them when the block object is destroyed. For the sorts of ephemeral blocks that we currently mark non-escaping, we can think about optimizing that away, too, but for now please test tha

[PATCH] D49457: DR330: when determining whether a cast casts away constness, consider qualifiers from all levels matching a multidimensional array

rjmccall added inline comments. Comment at: lib/Sema/SemaCast.cpp:535 +T1 = Unwrap(T1); +T2 = Unwrap(T2).withCVRQualifiers(T2.getCVRQualifiers()); + } Hmm. Just CVR? I understand that we can have problems here with the enumerated qualifiers, so maybe

[PATCH] D49462: [ObjC] Error out when using forward-declared protocol in a @protocol expression

rjmccall added a comment. Hmm. I think this is a reasonable change to make to the language. Have you investigated to see if this causes source-compatibility problems? Comment at: include/clang/Basic/DiagnosticSemaKinds.td:849 +def err_atprotocol_protocol : Error< + "@protoc

[PATCH] D49457: DR330: when determining whether a cast casts away constness, consider qualifiers from all levels matching a multidimensional array

rjmccall added inline comments. Comment at: lib/Sema/SemaCast.cpp:535 +T1 = Unwrap(T1); +T2 = Unwrap(T2).withCVRQualifiers(T2.getCVRQualifiers()); + } rsmith wrote: > rjmccall wrote: > > Hmm. Just CVR? I understand that we can have problems here with t

[PATCH] D49294: Sema: Fix explicit address space cast in C++

rjmccall added inline comments. Comment at: lib/Sema/SemaOverload.cpp:3150 + !getLangOpts().OpenCLCPlusPlus) +return false; + yaxunl wrote: > rjmccall wrote: > > It's not really OpenCL C++ that's special here, it's the possibility of > > promotions betw

[PATCH] D49083: [HIP] Register/unregister device fat binary only once

rjmccall added inline comments. Comment at: lib/CodeGen/CGCUDANV.cpp:444 +auto HandleValue = +CtorBuilder.CreateAlignedLoad(GpuBinaryHandle, CGM.getPointerAlign()); +llvm::Constant *Zero = llvm::Constant::getNullValue(HandleValue->getType()); yax

[PATCH] D49083: [HIP] Register/unregister device fat binary only once

rjmccall added a comment. Thanks for the comment. Comment at: lib/CodeGen/CGCUDANV.cpp:444 +auto HandleValue = +CtorBuilder.CreateAlignedLoad(GpuBinaryHandle, CGM.getPointerAlign()); +llvm::Constant *Zero = llvm::Constant::getNullValue(HandleValue->getType());

[PATCH] D48661: [Fixed Point Arithmetic] Fixed Point Constant

rjmccall added inline comments. Comment at: include/clang/Basic/FixedPoint.h:64 + +class APFixedPoint { + public: rjmccall wrote: > This should get a documentation comment describing the class. You should > mention that, like `APSInt`, it carries all of the sem

[PATCH] D49508: [CodeGen] VisitMaterializeTemporaryExpr(): don't skip NoOp Casts.

rjmccall added a comment. I think it would be reasonable to set a flag on `ImplicitCastExpr`s that are actually semantically part of an explicit cast. I don't think that would be hard to get Sema to do, either by passing a flag down to the code that builds those casts or just by retroactively

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

rjmccall added a comment. Thank you, this looks very clean now. Comment at: clang/docs/UsersManual.rst:1318 + mode informs the compiler that it must not assume any particular + rounding mode. + "represent *the* corresponding IEEE rounding rules" =

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

rjmccall added inline comments. Comment at: clang/docs/UsersManual.rst:1330 + and ``fast``. + Details: + rjmccall wrote: > "provided by other, single-purpose floating point options." I don't know why you keep including "clang" as a modifier here; this is the

[PATCH] D68818: [hip][cuda] Fix the extended lambda name mangling issue.

rjmccall added a comment. I agree with Richard's suggestion of just numbering all the lambdas in both modes if that's viable. Comment at: clang/include/clang/AST/DeclCXX.h:587-590 +unsigned NumExplicitCaptures : 12; + +/// Has known `internal` linkage. +unsigned Ha

[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/include/clang/Basic/LangOptions.h:203 + // Floating point exceptions are not handled: fp exceptions are masked. + FPEB_Ignore, // This is the default + // Optimizer will avoid transformations that may raise exceptions that

[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/docs/UsersManual.rst:1318 + mode informs the compiler that it must not assume any particular + rounding mode. + rjmccall wrote: > "represent *the* corresponding IEEE rounding rules" A few points about this doc

[PATCH] D69129: [AMDGPU] Fix assertion due to initializer list

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Minor change, but otherwise LGTM. Comment at: lib/CodeGen/CodeGenModule.cpp:3898 +Entry = CE->stripPointerCasts(); } You can just make this whol

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

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:5063 const_cast(D), PID); } } aprantl wrote: > rjmccall wrote: > > Is this special treatment still necessary? Won't we encounter the getter

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

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:890 + "cannot apply to inline functions, ignoring pragma">, + InGroup; What's the purpose of this restriction? Whether `inline` really has much to do with inlining

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

rjmccall added a comment. Okay. If the optimizer cannot correctly handle a mix of constrained and unconstrained FP operations, then the optimizer should protect itself, either by refusing to inline across such boundaries or by adding constraints as necessary before inlining. If it does that,

[PATCH] D69022: [coroutines] Remove assert on CoroutineParameterMoves in Sema::buildCoroutineParameterMoves

rjmccall resigned from this revision. rjmccall added a comment. Despite generally knowing about coroutines and generally knowing about Clang, I actually don't know the Clang coroutine code and can't review this properly. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D69322: [hip][cuda] Enable extended lambda support on Windows.

rjmccall added a comment. Please don't ping for review after a single day. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69322/new/ https://reviews.llvm.org/D69322 ___ cfe-commits mailing list cfe-comm

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. This certainly seems like a more tractable representation, although I suppose it'd be a thorn in the side of (IR-level) outlining. A note on spelling: the `no` prefix seems to be used largely with verbs; it's weird to use it here with an adjective, especially since `no

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. In D69498#1723553 , @rjmccall wrote: > This certainly seems like a more tractable representation, although I suppose > it'd be a thorn in the side of (IR-level) outlining. Sorry, I mis-clicked and sent this review while I was s

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

rjmccall added a comment. We could probably do a quick check to see if the class informally conforms to the protocol. `+copyWithZone` seems to be marked unavailable in ARC; not sure if that would cause problems for such a check. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. The kind of tracking that you're doing of convergent operations is one example of a basically limitless set of "taint" analyses, of which half a dozen already exist in LLVM. I object to the idea that an arbitrary number of unrelated tests need to be changed every time

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. If you can come up with a way to make this change that doesn't require changes to non-GPU frontends or tests, I agree that treating functions as convergent by default makes sense for GPU targets. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69498/new/ https:

[PATCH] D67983: [ObjC] Diagnose implicit type coercion from ObjC 'Class' to object pointer types.

rjmccall added a comment. I was thinking that we could walk through the instance methods of the protocol and check whether the class has compatible declarations of class methods. But you're right, of course: we don't actually know what class we're working with, and we'd have to look through `[

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

rjmccall added a comment. Is the exception-strictness of `-frounding-math` actually considered to be specified behavior, or is it just a consequence of the current implementation? There are definitely some optimizations that we can't do under strict FP exceptions that we can still do in princi

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

rjmccall added a comment. Thanks. A few things about the functionality parts of the patch now. Comment at: clang/include/clang/Basic/CodeGenOptions.def:238 CODEGENOPT(UnsafeFPMath , 1, 0) ///< Allow unsafe floating point optzns. +CODEGENOPT(RoundingFPMath, 1, 0) ///<

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. In D69498#1724625 , @kariddi wrote: > One thing to probably note is that its not only a "target specific" issue, > but a language specific issue as well (IMHO). OpenCL, CUDA, SYCL are all > languages (to name a few) that have a

[PATCH] D80166: [CGCall] Annotate reference parameters with "align" attribute.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/lib/CodeGen/CGCall.cpp:2258 Attrs.addDereferenceableAttr(info.first.getQuantity()); -Attrs.addAttribute(llvm::Attribute::getWithA

[PATCH] D79903: FastMathFlags.allowContract should be init from FPFeatures.allowFPContractAcrossStatement

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. No, go ahead. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79903/new/ https://reviews.llvm.org/D79903 __

[PATCH] D76791: [Matrix] Implement matrix index expressions ([][]).

rjmccall added a comment. Sorry for the slow review; I'm getting crushed with review requests. Comment at: clang/lib/AST/Expr.cpp:3859 + + auto *SubscriptE = dyn_cast(this); + return SubscriptE You need to `IgnoreParens()` here. Comment at:

[PATCH] D71739: [AssumeBundles] Use operand bundles to encode alignment assumptions

rjmccall added a comment. Is there a good reason for this to use the same `llvm.assume` intrinsic as before? Are there restrictions about what assumptions can be combined on a single intrinsic call? There can only be one bundle of a particular name on an instruction, right? ===

[PATCH] D80295: [Sema] Diagnose static data members in classes nested in unnamed classes

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:6904 << Name << RD->getTagKind(); Invalid = true; +} else if (RD->isLocalClass()) { This diagnostic actually ignores the tag kind that passed down to it, which

<    8   9   10   11   12   13   14   15   16   17   >