[PATCH] D39505: [OpenMP] Show error if VLAs are not supported

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:944 + /// \brief Whether target supports variable-length arrays. + bool isVLASupported() const { return VLASupported; } + Hahnfeld wrote: > rjmccall wrote: > > Hahnfeld wrote: > > > rj

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

rjmccall added a comment. So, that change makes this very interesting, because I think the right way of looking at it is as the first in a larger family of warnings that attempt to treat typedefs as if they were a much stronger type-system feature, i.e. that warn about all sorts of conversions

[PATCH] D39627: Fix vtable not receiving hidden visibility when using push(visibility)

rjmccall added a comment. In https://reviews.llvm.org/D39627#917175, @efriedma wrote: > Adding cfe-commits. (In the future, please make sure to CC that list instead > of llvm-commits for clang changes.) > > > I'm missing some background lingo here. What is the meaning of "a > > declaration" an

[PATCH] D39505: [OpenMP] Show error if VLAs are not supported

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:944 + /// \brief Whether target supports variable-length arrays. + bool isVLASupported() const { return VLASupported; } + ABataev wrote: > ABataev wrote: > > Hahnfeld wrote: > > > rjmc

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

rjmccall added a comment. In https://reviews.llvm.org/D39462#922844, @lebedev.ri wrote: > In https://reviews.llvm.org/D39462#922826, @rjmccall wrote: > > > I don't speak for the entire project, but I'm not sure I'm interested in > > the diagnostic you're actually offering to contribute here. It

[PATCH] D39944: [Sema] Stable sort OverloadCandidates to remove non-deterministic ordering

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Hmm. It looks like the intent is for CompareOverloadCandidatesForDisplay to be a total order, but I'm sure there are cases where it isn't. Okay, LGTM. https://reviews.llvm.org/D39944

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

rjmccall added a comment. I don't speak for the entire project, but I'm not sure I'm interested in the diagnostic you're actually offering to contribute here. It may produce a warning on your specific test case, but I think it's really much too rigid and will lead to massive false positives.

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

rjmccall added a comment. I see. The problem now, though, is that things involving, say, a size_t and a numeric_limits will never warn. Repository: rL LLVM https://reviews.llvm.org/D39462 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D39947: [OpenMP] Stable sort Privates to remove non-deterministic ordering

rjmccall added a comment. In https://reviews.llvm.org/D39947#922870, @mgrang wrote: > Although this patches fixes the above unit test failures, the generated code > is very different from the one that the tests expect. As a result, these > tests need to be adjusted. Could the reviewers please c

[PATCH] D39462: [Sema] Implement -Wmaybe-tautological-constant-compare for when the tautologicalness is data model dependent

rjmccall added a comment. That's the template type-propagation problem: the template was invoked with a size_t, but that's erased down to the canonical type by the template machinery. That can't be fixed within the template, but at use sites, we could theoretically recognize that e.g. the resu

[PATCH] D39455: [CodeGen] Add initial support for union members in TBAA

rjmccall added a comment. I think the implementation looks fine. The metadata design also sounds good to me, but I'd like Hal to weigh in before you commit. Repository: rL LLVM https://reviews.llvm.org/D39455 ___ cfe-commits mailing list cfe-co

[PATCH] D39947: [OpenMP] Stable sort Privates to remove non-deterministic ordering

rjmccall added a comment. In https://reviews.llvm.org/D39947#922922, @mgrang wrote: > In https://reviews.llvm.org/D39947#922889, @rjmccall wrote: > > > In https://reviews.llvm.org/D39947#922870, @mgrang wrote: > > > > > Although this patches fixes the above unit test failures, the generated > >

[PATCH] D39953: [CodeGen] Do not lookup for cached TBAA metadata nodes twice

rjmccall added inline comments. Comment at: lib/CodeGen/CodeGenTBAA.cpp:267 + if (N) return N; The main danger with persisting this kind of reference is that DenseMap doesn't actually guarantee stability, so if there are recursive calls that can grow the

[PATCH] D39955: [Driver] Add command-line flags for the new TBAA metadata format

rjmccall added a comment. We don't want this to become a long-term feature; we just want to switch the implementation over to the new format. Is there a good reason not to do that in one patch? If so, adding an option is acceptable, but it should be a -cc1 option only, because it is purely a

[PATCH] D39505: [OpenMP] Show error if VLAs are not supported

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:944 + /// \brief Whether target supports variable-length arrays. + bool isVLASupported() const { return VLASupported; } + Hahnfeld wrote: > rjmccall wrote: > > ABataev wrote: > > > ABa

[PATCH] D39505: [OpenMP] Show error if VLAs are not supported

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:944 + /// \brief Whether target supports variable-length arrays. + bool isVLASupported() const { return VLASupported; } + Hahnfeld wrote: > rjmccall wrote: > > Hahnfeld wrote: > > > rj

[PATCH] D39505: [OpenMP] Show error if VLAs are not supported

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:944 + /// \brief Whether target supports variable-length arrays. + bool isVLASupported() const { return VLASupported; } + ABataev wrote: > Hahnfeld wrote: > > rjmccall wrote: > > > Hah

[PATCH] D39955: [Driver] Add command-line flags for the new TBAA metadata format

rjmccall added a comment. Okay. I'm fine with living with both formats as long as we're not exposing this as a feature. Repository: rL LLVM https://reviews.llvm.org/D39955 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llv

[PATCH] D39505: [OpenMP] Show error if VLAs are not supported

rjmccall added inline comments. Comment at: include/clang/Basic/TargetInfo.h:944 + /// \brief Whether target supports variable-length arrays. + bool isVLASupported() const { return VLASupported; } + Hahnfeld wrote: > rjmccall wrote: > > ABataev wrote: > > > Hah

[PATCH] D40115: Driver: remove `SupportsObjCGC`

rjmccall added a comment. This isn't changing the actual driver interface in any way, it's just dead-code removal. Seems fine to me. Repository: rL LLVM https://reviews.llvm.org/D40115 ___ cfe-commits mailing list cfe-commits@lists.llvm.org htt

[PATCH] D40141: [ObjC][ARC] Honor noescape attribute for -Warc-retain-cycles

rjmccall added inline comments. Comment at: lib/Sema/SemaChecking.cpp:11661 if (Expr *capturer = findCapturingExpr(*this, msg->getArg(i), owner)) return diagnoseRetainCycle(*this, capturer, owner); + } Failing to find a capturing expression will over

[PATCH] D40023: [RISCV] Implement ABI lowering

rjmccall added inline comments. Comment at: lib/CodeGen/CGCall.cpp:1937 +RetAttrs.addAttribute(llvm::Attribute::ZExt); +} // FALL THROUGH I feel like a better design would be to record whether to do a sext or a zext in the ABIArgInfo. Add getSi

[PATCH] D39953: [CodeGen] Do not lookup for cached TBAA metadata nodes twice

rjmccall added a comment. I think having a primary function that handles the caching logic makes some sense. I think there might be some cases that intentionally don't cache their normal result, though, so it might be harder than you think. Up to you whether you want to continue. Repository

[PATCH] D39505: [OpenMP] Show error if VLAs are not supported

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, seems fine. Thanks for putting up with my questions. https://reviews.llvm.org/D39505 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D39627: Fix vtable not receiving hidden visibility when using push(visibility)

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Looks great, thanks! Repository: rL LLVM https://reviews.llvm.org/D39627 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://li

[PATCH] D39955: [Driver] Add a cc1 flag for the new TBAA metadata format

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

[PATCH] D39947: [OpenMP] Stable sort Privates to remove non-deterministic ordering

rjmccall accepted this revision. rjmccall added a comment. Yes, LGTM. Repository: rL LLVM https://reviews.llvm.org/D39947 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D39953: [CodeGen] Generate TBAA type descriptors in a more reliable manner

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

[PATCH] D40275: [CUDA] Report "unsupported VLA" errors only on device side.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:2188 + !Context.getTargetInfo().isVLASupported() && + shouldDiagnoseTargetSupportFromOpenMP()) { + // Some targets don't support VLAs. Please write this check

[PATCH] D146188: [Clang][DOC] Add documentation in for __builtin_flt_rounds and __builtin_set_flt_rounds

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:3283 +Sets the current rounding mode. Encoding of the returned values is +same as the result of FLT_ROUNDS, specified by C standard: +0 - toward zero "Sets the current floating-point r

[PATCH] D146188: [Clang][DOC] Add documentation in for __builtin_flt_rounds and __builtin_set_flt_rounds

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:3271 +4 - to nearest, ties away from zero +The effect of passing some other value to this builtin is implementation-defined. + This comment belongs with the other builtin. I would su

[PATCH] D146188: [Clang][DOC] Add documentation in for __builtin_flt_rounds and __builtin_set_flt_rounds

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:3288 +This builtin is restrcted to work on x86 and arm targets currently. When support +for the builtin is added for new targets, the manual should be updated accordingly. + This meta

[PATCH] D146386: [MS ABI] Fix mangling references to declarations.

rjmccall added inline comments. Comment at: clang/lib/AST/MicrosoftMangle.cpp:720 +const CXXRecordDecl *RD, const ValueDecl *VD) { + MSInheritanceModel IM = RD->getMSInheritanceModel(); + // ::= efriedma wrote: > It's not obvious to me why the inheritance

[PATCH] D146188: [Clang][DOC] Add documentation in for __builtin_flt_rounds and __builtin_set_flt_rounds

rjmccall added a comment. Okay. I was looking at a version of the standard that makes reading the environment UB. If that's been relaxed, then I agree that it would be much more natural to talk about *changing* the environment than just *accessing* it. And yeah, I agree it would make sense to

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added a comment. The user isn't modifying the `float_t` type definition, they're using it. I think the diagnostic should say something like `cannot use type 'float_t' within '#pragma clang fp eval_method'; type is set according to the default eval method for the translation unit`. Is

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added a comment. In D146148#4217382 , @zahiraam wrote: > In D146148#4213475 , @rjmccall > wrote: > >> The user isn't modifying the `float_t` type definition, they're using it. I >> think the diagnostic

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added a comment. Okay, so modifying `math.h` to use this attribute is acceptable? That's great, that's definitely the best outcome for the compiler. Just a minor request about the diagnostic name, but otherwise LGTM. Comment at: clang/include/clang/Basic/DiagnosticS

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added a comment. In D146148#4220591 , @zahiraam wrote: > In D146148#4220495 , @aaron.ballman > wrote: > >> In D146148#4220433 , @zahiraam >> wrote: >> >>> In D1

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added a comment. In D146148#4224094 , @aaron.ballman wrote: > In D146148#4222306 , @zahiraam > wrote: > >> In D146148#4221651 , @rjmccall >> wrote: >> >>> Zahi

[PATCH] D146188: [Clang][DOC] Add documentation in for __builtin_flt_rounds and __builtin_set_flt_rounds

rjmccall added a comment. In D146188#4223249 , @xiongji90 wrote: > Hi, @rjmccall and @sepavloff > In UserManual, we include a section `Controlling Floating Point Behavior`: > https://github.com/llvm/llvm-project/blob/main/clang/docs/UsersManual.rst#cont

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added a comment. Yeah, that was the sort of thing I was worried about. The identifier thing is also a reasonable approach, and it'll be nice infrastructure for other, similar things. The ranges of special identifier we already call out on IdentifierInfo* are for builtin functions (wh

[PATCH] D146188: [Clang][DOC] Add documentation in for __builtin_flt_rounds and __builtin_set_flt_rounds

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:3272 +3 - toward negative infinity +4 - to nearest, ties away from zero +The effect of passing some other value to ``__builtin_flt_rounds`` is I suspect this won't end up being format

[PATCH] D144454: Add builtin for llvm set rounding

rjmccall added a comment. In D144454#4157717 , @xiongji90 wrote: > In D144454#4142253 , @rjmccall > wrote: > >> New builtins should be documented in the user manual. >> >> There are standard pragmas for changing

[PATCH] D144454: Add builtin for llvm set rounding

rjmccall added a comment. I see. If we're going to take the target-independent values specified by `FLT_ROUNDS`, then the original builtin name is more appropriate. Of course, this has the disadvantage of not allowing target-specific values that might exist beyond those specified in the stand

[PATCH] D144454: Add builtin for llvm set rounding

rjmccall added a comment. `__builtin_set_flt_rounds` seems better. We should add the portability checking, yeah. Look at the checking we do for certain builtins with `CheckBuiltinTargetInSupported` in `SemaChecking.cpp`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https

[PATCH] D144454: Add builtin for llvm set rounding

rjmccall added a comment. Okay, so if we're adding this builtin, and it's not just imitating a stdlib function, we should definitely document it as a language extension. There's a section in the manual about controlling FP modes which is probably an appropriate place for this. I assume we nee

[PATCH] D144454: Add builtin for llvm set rounding

rjmccall accepted this revision. rjmccall added a comment. I have no objection to doing the documentation in a separate patch. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144454/new/ https://reviews.llvm.org/D144454 __

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

rjmccall added a comment. There are *some* properties we can still assume about `linkonce_odr` functions despite them being replaceable at link time. The high-level language guarantee we're starting from is that the source semantics of all versions of the function are identical. The version o

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

rjmccall added a comment. In D83906#4182287 , @dexonsmith wrote: > - At IRGen time, you know the LLVM attributes have not been adjusted after > the optimized refined the function's behaviour. It should be safe to have IPA > peepholes, as long as IRGen's

[PATCH] D83906: [CodeGen] Emit a call instruction instead of an invoke if the called llvm function is marked nounwind

rjmccall added a comment. So your argument is that it would not be possible to recognize that we're doing such an optimization and mark the function as having had a possible semantics change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83906/new

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:6784 + if (II->getInterestingIdentifierID() != 0) +NewTD->addAttr(AvailableOnlyInDefaultEvalMethodAttr::Create(Context)); } Please switch over the interesting identifiers he

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:6784 + if (II->getInterestingIdentifierID() != 0) +NewTD->addAttr(AvailableOnlyInDefaultEvalMethodAttr::Create(Context)); } zahiraam wrote: > rjmccall wrote: > > Please swit

[PATCH] D143241: [Clang] Reset FP options before function instantiations

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseTemplate.cpp:1736 + // point of the template definition. + Actions.resetFPOptions(LPT.FPO); + Ah, is this bug specific to the MSVC-compatible template logic? Repository: rG LLVM Github Monorep

[PATCH] D146148: [clang] Add a namespace for interesting identifiers.

rjmccall added a comment. Yes, this is very close, thank you. Comment at: clang/include/clang/Basic/TokenKinds.def:805 +INTERESTING_IDENTIFIER(float_t) +INTERESTING_IDENTIFIER(double_t) +INTERESTING_IDENTIFIER(FILE) I think it would be cleaner if you added thes

[PATCH] D146148: [clang] Add a namespace for interesting identifiers.

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

[PATCH] D143241: [Clang] Reset FP options before function instantiations

rjmccall added a comment. Hmm. Why are we clearing the FP pragma stack instead of saving the old context onto it and then restoring after instantiation? I don't think semantic analysis ever depends on enclosing members of the stack, does it? Clearing the entire stack might not matter much if

[PATCH] D152818: [Clang] Make template instantiations respect pragma FENV_ACCESS state at point-of-definition

rjmccall added a comment. Does https://reviews.llvm.org/D143241 solve the original problem here, or is there something deeper? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152818/new/ https://reviews.llvm.org/D152818

[PATCH] D153590: Don't use float_t and double_t with #pragma clang fp eval_method.

rjmccall added a comment. `abi-check` is a really generic name for a test case; could you rename those three tests to something more specific to this feature? Also, your tests are only testing this behavior in C++. Patch functionality LGTM. Repository: rG LLVM Github Monorepo CHANGES SINC

[PATCH] D153590: Don't use float_t and double_t with #pragma clang fp eval_method.

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

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

rjmccall added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2118 + if (Packed) +UnadjustedAlignment = std::max(UnadjustedAlignment, UnpackedFieldAlign); UpdateAlignment(FieldAlign, UnpackedFieldAlign, PreferredAlign); I've always fel

[PATCH] D146897: [clang:diagnostics] Turning off warn_self_assignment_overloaded for user-defined compound assignments

rjmccall added a comment. Yeah, I feel there are three ideas here: - Clang should warn about self simple assignment in all cases, because it's probably a mistake. We can assume it's a mistake because it's reasonable to assume that the simple assignment operator behaves like a value assignment,

[PATCH] D146897: [clang:diagnostics] Turning off warn_self_assignment_overloaded for user-defined compound assignments

rjmccall added a comment. Totally fair :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146897/new/ https://reviews.llvm.org/D146897 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.

[PATCH] D146897: [clang:diagnostics] Turning off warn_self_assignment_overloaded for user-defined compound assignments

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:15660 case BO_Assign: +// Skip diagnose on compound assignment. +DiagnoseSelfAssignment(S, LHS, RHS, OpLoc, false); Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[PATCH] D150913: [Clang][Bfloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

rjmccall added inline comments. Comment at: clang/lib/Basic/Targets/X86.cpp:381 HasBFloat16 = SSELevel >= SSE2; pengfei wrote: > I'm not sure if I understand the meaning of `HasFullBFloat16`. If it is used > for target that supports arithmetic `__bf16`,

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/include/clang/Basic/IdentifierTable.h:316 + unsigned getInterestingIdentifierID() { +if (ObjCOrBuiltinID >= 1341 && ObjCOrBuiltinID < 1343) + return ObjCOrBuiltinID; This is closer to the right approach

[PATCH] D148089: [clang][CodeGen] Break up TargetInfo.cpp [1/8]

rjmccall added a comment. Sergei, feel free to start landing patches like this one that were already approved. You don't need the entire sequence to be approved first. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148089/new/ https://reviews.llvm

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

rjmccall added a comment. It's well-formed as IR, just not semantically valid, right? As long as it's not actually crashing in the verifier, please test as much as you reasonably can that's correct, like that the constructor and destructor functions take something in the right address space, a

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

rjmccall added a comment. I see. Yes, in that case I think you're right that we can't test this yet — only base-subobject ctors and dtors get VTT parameters, we only emit calls to those from other ctors and dtors, and those ctors and dtors will always have their own stores to the v-table slot

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

rjmccall added a comment. It's weird to have C-style casts that can't be done with any combination of named casts, so I think this makes some sense. I do think it should be limited to numbered address spaces of the same size, though, rather than basing it on whether we're in OpenCL mode. Targ

[PATCH] D150913: [Clang][Bfloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

rjmccall added a comment. Apologies for misunderstanding what this patch was doing. This all seems reasonable, and the code changes look good. I think the documentation needs significant reorganization; I've attached a draft. Please review for correctness. Comment at: cla

[PATCH] D150913: [Clang][Bfloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:852 ``double`` when passed to ``printf``, so the programmer must explicitly cast it to ``double`` before using it with an ``%f`` or similar specifier. pengfei wrote: > rjmccall wro

[PATCH] D151076: [IRGen] Handle infinite cycles in findDominatingStoreToReturnValue.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Nice catch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151076/new/ https://reviews.llvm.org/D151076

[PATCH] D151298: [clang][LoongArch] Fix the calling convention for empty struct in C++ mode

rjmccall added a comment. As far as I'm concerned as editor of the Itanium ABI, the ABI treatment of trivial-for-the-purposes-of-calls classes is purely a psABI matter, and the Itanium ABI's wording around empty classes is merely a suggestion if the psABI doesn't have more specific rules (becau

[PATCH] D151087: [Clang] Permit address space casts with 'reinterpret_cast' in C++

rjmccall added a comment. Right, that's been my thinking, too. The CVR qualifiers are all "view" qualifiers: both the underlying object and the reference to it are assumed to be the same independent of qualifiers, and the qualifiers just affect the rules around accesses. C++'s rules around qu

[PATCH] D150913: [Clang][BFloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:852 ``double`` when passed to ``printf``, so the programmer must explicitly cast it to ``double`` before using it with an ``%f`` or similar specifier. codemzs wrote: > pengfei wrot

[PATCH] D150913: [Clang][BFloat16] Upgrade __bf16 to arithmetic type, change mangling, and extend excess precision support.

rjmccall added a comment. One slight miscommunication. Otherwise this LGTM, thank you. Comment at: clang/docs/LanguageExtensions.rst:824 +provide native architectural support for arithmetic on these formats. These +targets are noted in the lists of supported targets above. + -

[PATCH] D151515: [CodeGen] add additional cast when checking call arguments

rjmccall added a comment. Yeah, I agree with Eli, there should be a cast here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151515/new/ https://reviews.llvm.org/D151515 ___ cfe-commits mailing list cfe-

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

rjmccall added a comment. The process is pretty lightweight; I'd recommend just doing it if you expect to make more than one patch. But we don't have a policy against committing patches for other people as long as there's clear attribution in the commit. I do think you could successfully test

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

rjmccall added a comment. > I would like to clarify the second point to make sure that I'm incorporating > that suggestion - the checks on lines 25, 26 & 27 are for actual calls - > since now the signature matches the VTT type there's no arg setup prior to > the call, it's a direct passing of t

[PATCH] D143919: [Clang] Copy strictfp attribute from pattern to instantiation

rjmccall added a comment. In D143919#4123712 , @sepavloff wrote: > In D143919#4123616 , @efriedma > wrote: > >> We have code somewhere to generically copy attributes from function >> templates to instantiations,

[PATCH] D144454: Add builtin for llvm set rounding

rjmccall added a comment. New builtins should be documented in the user manual. There are standard pragmas for changing the rounding mode, right? What's the interaction between the ability to set this dynamically with a builtin call and those pragmas? Comment at: clang/test

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:4371 +incorrect code, leading to an ABI mismatch. This case is prevented by emitting a +diagnostic. + Suggestion: ``` ``math.h`` defines the typedefs ``float_t`` and ``double_t`` base

[PATCH] D146188: [Clang][DOC] Add documentation in for __builtin_flt_rounds and __builtin_set_flt_rounds

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146188/new/ https://reviews.llvm.org/D146188

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added a comment. In D146148#4330611 , @zahiraam wrote: > In D146148#4221651 , @rjmccall > wrote: > >> In D146148#4220591 , @zahiraam >> wrote: >> >>> In D146148

[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

rjmccall added a comment. > Okay, so I did add that in TokenKinds.h. Isn't that the right place for it? > The same way it's done for the other builtins? And in TokenKinds.def I added > the lines for the interesting identifiers? What you're doing in `TokenKinds.{def,h}` seems fine. What I'm obj

[PATCH] D150499: [AST] Initialized data after TypeSourceInfo

rjmccall added a comment. This memory is supposed to be initialized when we copy the `TypeLoc`. Are you observing that not happening? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150499/new/ https://reviews.llvm.org/D150499

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

rjmccall added a comment. I agree with Eli; if the address space is wrong coming into this, we're doing something wrong at a more basic level. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150746/new/ https://reviews.llvm.org/D150746 ___ cfe

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

rjmccall added a comment. Passing the VTT in the appropriate AS for global variables seems like the right way to go — we do know that that argument will always be such a pointer, so there's no point in converting to the generic address space. For that matter, v-table pointer slots within classe

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

rjmccall added a comment. Hmm. Preserving unused global variables in case a hot patch might use them doesn't seem very compelling to me — hot patches need to be able to introduce globals of their own anyway. What I find more convincing is related to the *other* meaning of `__attribute__((used

[PATCH] D150221: Add option -fkeep-static-variables to emit all static variables

rjmccall added a comment. The hot-patch use case actually shouldn't care if the compiler/linker throws away unused things because it can't possibly affect how the patch interoperates with existing code. This does rely on the unanalyzable-use part of the semantics, though, to stop the compiler

[PATCH] D150746: [CodeGen]Translating pointer arguments can require an address space cast

rjmccall added a comment. Can you add a test? I think we have some in-tree targets which put globals in a non-default address space. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:1679 CGF.GetVTTParameter(GlobalDecl(D, Type), ForVirtualBase, Delegating); - QualType V

[PATCH] D145088: [RISCV] Add attribute(riscv_rvv_vector_bits(N)) based on AArch64 arm_sve_vector_bits.

rjmccall added a comment. The CodeGen change looks fine. I'm surprised you didn't need any code in argument/parameter/call/return emission to do the actual fixed<->scalable coercion; do we already have that for other reasons? Comment at: clang/include/clang/Basic/AttrDocs.td

[PATCH] D145088: [RISCV] Add attribute(riscv_rvv_vector_bits(N)) based on AArch64 arm_sve_vector_bits.

rjmccall added a comment. > You mean RISC-V specific code or generic code? If generic, I assume we got it > from SVE's earlier implementation. Ah, if SVE has a similar feature then that makes sense. Comment at: clang/include/clang/Basic/AttrDocs.td:2332 + + #if __RISCV_RVV_V

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. Yeah. To be clear, though, I'm not asking for the overall data flow of the function to be fixed in this patch; I'm just pointing out problems in the new logic being added by thi

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

rjmccall added a comment. Thank you. Per my comment here: > Also, AAPCS64 seems to define UnadjustedAlignment as the "natural alignment", > and there's a doc comment saying it's the max of the type alignments. That > makes me wonder if we should really be considering either the aligned > attr

[PATCH] D145088: [RISCV] Add attribute(riscv_rvv_vector_bits(N)) based on AArch64 arm_sve_vector_bits.

rjmccall added a comment. Thanks, a couple very minor fixes / requests, but feel free to commit afterwards. Comment at: clang/include/clang/Basic/AttrDocs.td:2323 + let Content = [{ +The ``riscv_rvv_vector_bits(N)`` attribute is used to define fixed-length +variants of sizele

[PATCH] D147626: [clang] Reject flexible array member in a union in C++

rjmccall added a comment. I have no specific objections. I do worry about removing support for something that's apparently been accepted for a long time, just on general source-compatibility grounds, but I don't think there's an ObjC-specific problem with it. Repository: rG LLVM Github Mon

[PATCH] D152096: [Clang] Check for abstract parameters only when functions are defined.

rjmccall added inline comments. Comment at: clang/test/SemaObjCXX/parameters.mm:14-17 +struct test2 { virtual void foo() = 0; }; @interface Test2 -- (void) foo: (test2) foo; // expected-error {{parameter type 'test2' is an abstract class}} +- (void) foo: (test2) foo ; @end ---

[PATCH] D152689: [NFC] Remove dead conditionals

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/CodeGen/CGObjCMac.cpp:3812 llvm::GlobalVariable *GV; - if (ForClass) -GV = aaron.ballman wrote: > I agree this is dead

[PATCH] D152818: [Clang] Fix assertion when pragma FENV_ACCESS is used with a throw function.

rjmccall added a comment. I'd guess we're not resetting the pragma state appropriately when instantiating templates. Expressions should be governed by the pragmas in scope at the point of definition, not at the point of instantiation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

<    23   24   25   26   27   28   29   30   31   32   >