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

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

[PATCH] D71805: [clang] [ast] CXXRecordDecl::getVisibleConversionFunctions() could be const

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

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

rjmccall added a comment. In D69272#1794630 , @sepavloff wrote: > In D69272#1793234 , @rjmccall wrote: > > > In D69272#1792928 , @sepavloff > > wrote: > > > > > @hfinkel pro

[PATCH] D75700: [NFC] Let mangler accept GlobalDecl

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1028 else - MC.mangleName(ND, Out); + MC.mangleName(GD, Out); } else { What would be necessary in order for this to turn into just `mangleName(GD, Out)`? I suspec

[PATCH] D64464: [CodeGen] Emit destructor calls to destruct compound literals

rjmccall added a comment. This patch looks good except for that C/C++ semantic difference. A compound literal temporary can be lifetime-extended in C++, but only in the standard way of binding a reference to it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D64464: [CodeGen] Emit destructor calls to destruct compound literals

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:4100 + if (E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) +pushDestroy(QualType::DK_nontrivial_c_struct, DeclPtr, E->getType()); + ahatanak wrote: > rjmccall wrote: > >

[PATCH] D64464: [CodeGen] Emit destructor calls to destruct compound literals

rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:4100 + if (E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) +pushDestroy(QualType::DK_nontrivial_c_struct, DeclPtr, E->getType()); + ahatanak wrote: > rjmccall wrote: > >

[PATCH] D75700: [NFC] Let mangler accept GlobalDecl

rjmccall added inline comments. Comment at: clang/lib/AST/Expr.cpp:693 else - MC->mangleName(ND, Out); + GD = GlobalDecl(ND); +MC->mangleName(GD, Out); This will need an extension for your case, right? Maybe there should be co

[PATCH] D75700: [NFC] Let mangler accept GlobalDecl

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/AST/ItaniumMangle.cpp:1563 + else +GD = GlobalDecl(dyn_cast(DC)); + return GD; rjmccall wrote: > `cast`? But I'm not sur

[PATCH] D58164: Block+lambda: allow reference capture

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, I think this is a reasonable fix. If @rsmith has thoughts about this, I guess they'll have to happen post-review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D72841: [RFC] Add support for pragma float_control, to control precision and exception behavior at the source level

rjmccall added a comment. In D72841#1908084 , @mibintc wrote: > @rjmccall suggested that I needed to remove FPOptions from the Stmt class > since the sizeof assertion failed. I moved that information into the relevant > expression nodes and fixed a few s

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

rjmccall added inline comments. Comment at: clang/include/clang/Sema/Sema.h:1470 +/// diagnostics should be emitted. +SmallVector DeclsToCheckForDeferredDiags; + This needs to be saved and restored in modules / PCH. Comment at: clang/li

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a comment. I'm fine with people developing a proposal for this openly, but it needs to be said that language changes cannot just be made in open-source; they have to go through the official language review process, which for Objective-C is an internal committee within Apple. The

[PATCH] D64464: [CodeGen] Emit destructor calls to destruct compound literals

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBlocks.cpp:869 + if (auto *BD = C.dyn_cast()) +enterBlockScope(*this, BD); } I wonder if we could just switch blocks to the same thing. Comment at: clang/lib/Sema/S

[PATCH] D66094: [CodeGen] Emit destructor calls for non-trivial C structs returned by function calls and loaded from volatile objects

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprAgg.cpp:822 +Dest.setExternallyDestructed(); + } + I don't think `setExternallyDestructed` can be used to communicate outwards like this; the code isn't set up to just do modificati

[PATCH] D72841: [RFC] Add support for pragma float_control, to control precision and exception behavior at the source level

rjmccall added a comment. In D72841#1912416 , @mibintc wrote: > In D72841#1911330 , @rjmccall wrote: > > > In D72841#1908084 , @mibintc wrote: > > > > > @rjmccall suggested t

[PATCH] D64464: [CodeGen] Emit destructor calls to destruct compound literals

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Minor comments; otherwise LGTM. Comment at: clang/lib/Sema/SemaExpr.cpp:6256 +// Compound literals that have automatic storage duration are destroyed at +// the e

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

rjmccall added a comment. If the bit is unused except for propagation, please remove it. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59214/new/ https://reviews.llvm.org/D59214 ___ cfe-commits mailing list cfe-commit

[PATCH] D59214: [clang][OpeMP] Model OpenMP structured-block in AST (PR40563)

rjmccall added a comment. In D59214#1917149 , @lebedev.ri wrote: > In D59214#1916596 , @sammccall wrote: > > > So if I understand the history here: > > > > - the `IsOMPStructuredBlock` bit on `Stmt` was added to imp

[PATCH] D72841: [RFC] Add support for pragma float_control, to control precision and exception behavior at the source level

rjmccall added a comment. In D72841#1917340 , @mibintc wrote: > @rjmccall Since CompoundAssignmentOperator derives from BinaryOperator, it's > not simple to add Trailing storage here. I think I will have to fold > CompoundAssignmentOperator into BinaryO

[PATCH] D76040: [TableGen] Move generated *Attr class methods out of line

rjmccall added a comment. Are there any classes of methods which are worth continuing to generate inline? Surely some of the accessors are trivial. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76040/new/ https://reviews.llvm.org/D76040 __

[PATCH] D72841: [RFC] Add support for pragma float_control, to control precision and exception behavior at the source level

rjmccall added a comment. In D72841#1917459 , @rjmccall wrote: > In D72841#1917340 , @mibintc wrote: > > > @rjmccall Since CompoundAssignmentOperator derives from BinaryOperator, > > it's not simple to add Trailing

[PATCH] D76040: [TableGen] Move generated *Attr class methods out of line

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

[PATCH] D76077: [ARM] Add __bf16 as new Bfloat16 C Type

rjmccall added a comment. I don't understand why you wouldn't add a new IR type for this; doing so should be totally mechanical. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76077/new/ https://reviews.llvm.org/D76077 __

[PATCH] D76077: [ARM] Add __bf16 as new Bfloat16 C Type

rjmccall added a comment. Note that we have an IR type for the PPC double-double format, which isn't even hardware-supported. This is literally just an IEEE floating-point format with non-standard parameters, right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D76077: [ARM] Add __bf16 as new Bfloat16 C Type

rjmccall added a comment. In D76077#1921490 , @LukeGeeson wrote: > In D76077#1919861 , @rjmccall wrote: > > > I don't understand why you wouldn't add a new IR type for this; doing so > > should be totally mechanica

[PATCH] D60748: Fix i386 struct and union parameter alignment

rjmccall added a comment. Oh, I see you just updated your patch months ago without ever mentioning that it was ready for review. It sounds to me like GCC retroactively added a switch specifying which version of the ABI to follow on this point, somewhat confusingly called `-malign-data`. That'

[PATCH] D76262: [NFC] Add UsedDeclVisitor

rjmccall added a comment. Thanks, looks good except for one oversight. Comment at: clang/lib/Sema/UsedDeclVisitor.h:43 + asImpl().visitUsedDecl(E->getMemberLoc(), D); +} + } You need to recurse on the base expression here. (And that's a good test cas

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

rjmccall added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1514 + void visitUsedDecl(SourceLocation Loc, Decl *D) { +if (auto *TD = dyn_cast(D)) { + for (auto *DD : TD->decls()) { bader wrote: > yaxunl wrote: > > rjmccall wrote: > > > yaxunl wr

[PATCH] D75574: RFC: Implement objc_direct_protocol attribute to remove protocol metadata

rjmccall added a subscriber: manmanren. rjmccall added a comment. This might also be interesting to @manmanren. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574

[PATCH] D66094: [CodeGen] Emit destructor calls for non-trivial C structs returned by function calls and loaded from volatile objects

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:677 + E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) +Cleanup.setExprNeedsCleanups(true); + Why only when the l-value is volatile? Comment at

[PATCH] D75169: [ARM] Enforcing calling convention for half-precision FP arguments and returns for big-endian AArch32

rjmccall added a comment. Ugh. I'd hate to introduce yet another weird little tweak to `ABIArgInfo` that's used on exactly one target. For 16-bit composite types, we seem to coerce to a type like `[1 x i32]`; would that be okay here? You don't have a test that checks that you get the IR you w

[PATCH] D76262: [NFC] Add UsedDeclVisitor

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

[PATCH] D60748: Fix i386 struct and union parameter alignment

rjmccall added a comment. In D60748#1925907 , @LiuChen3 wrote: > In D60748#1924794 , @rjmccall wrote: > > > Oh, I see you just updated your patch months ago without ever mentioning > > that it was ready for review.

[PATCH] D75169: [ARM] Enforcing calling convention for half-precision FP arguments and returns for big-endian AArch32

rjmccall added a comment. In D75169#1926545 , @pratlucas wrote: > Hi @rjmccall, > I agree those kind of tweaks do not look good. The issue here, though, is > that argument coercion currently ignores the target's endian information when > performing coer

[PATCH] D76283: [IRBuilder] Use preferred target type for len argument of memory intrinsic functions

rjmccall added a comment. I guess the logical rule would be that the bit-width of the size has to match the minimum of the bit-widths of the address spaces used with the intrinsic. (Note that `llvm.memcpy` etc. can take pointers into two different address spaces, potentially of different width

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

rjmccall accepted this revision. rjmccall added a comment. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74361/new/ https://reviews.llvm.org/D74361 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D66094: [CodeGen] Emit destructor calls for non-trivial C structs returned by function calls and loaded from volatile objects

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:677 + E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) +Cleanup.setExprNeedsCleanups(true); + ahatanak wrote: > rjmccall wrote: > > Why only when the l-value is v

[PATCH] D75169: [ARM] Enforcing calling convention for half-precision FP arguments and returns for big-endian AArch32

rjmccall added a comment. In D75169#1928696 , @pratlucas wrote: > > Oh, wait, AAPCS wants half values to be passed in the *least* significant > > bits of a GPR, even on big-endian machines? That's certainly more > > convenient, but it's a weird inconsist

[PATCH] D66094: [CodeGen] Emit destructor calls for non-trivial C structs returned by function calls and loaded from volatile objects

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:677 + E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) +Cleanup.setExprNeedsCleanups(true); + ahatanak wrote: > rjmccall wrote: > > ahatanak wrote: > > > rjmccall

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

rjmccall added inline comments. Comment at: clang/lib/Sema/Sema.cpp:1514 + void visitUsedDecl(SourceLocation Loc, Decl *D) { +if (auto *TD = dyn_cast(D)) { + for (auto *DD : TD->decls()) { yaxunl wrote: > rjmccall wrote: > > bader wrote: > > > yaxunl wr

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

rjmccall added a comment. This looks good, assuming there's either no issue with the lazy emission of variables or that you just intend to tackle that later. Comment at: clang/lib/Sema/Sema.cpp:1540 +} else if (auto *VD = dyn_cast(D)) { + if (auto *Init = VD->getInit(

[PATCH] D66094: [CodeGen] Emit destructor calls for non-trivial C structs returned by function calls and loaded from volatile objects

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:677 + E->getType().isDestructedType() == QualType::DK_nontrivial_c_struct) +Cleanup.setExprNeedsCleanups(true); + ahatanak wrote: > rjmccall wrote: > > ahatanak wrote: > > > rjmccall

[PATCH] D66094: [CodeGen] Emit destructor calls for non-trivial C structs returned by function calls and loaded from volatile objects

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thank, this looks great. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66094/new/ https://reviews.llvm.org/D66094 ___

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

rjmccall added a comment. If it's reasonable to persist the failure kind, that would probably be good, but it might be a lot of work. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73360/new/ https://reviews.llvm.org/D73360 ___ cfe-commits

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:17127 + template + class UsedDeclVisitor : public EvaluatedExprVisitor>{ + protected: This should inherit from `EvaluatedExprVisitor`, or else calls from `EvaluatedExprVisitor` and above

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:17254 +SourceLocation(), Context.getTranslationUnitDecl()); + } Thanks, this looks a lot better. Should this be moved to SemaOpenMP.cpp (and renamed to be OpenMP-specific), or do

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay, we can go with this for now, since it does fix a bug. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73360/new/ https://reviews.llvm.org/D73360 ___

[PATCH] D70172: [CUDA][HIP][OpenMP] Emit deferred diagnostics by a post-parsing AST travese

rjmccall added a comment. One minor request, but otherwise LGTM; feel free to commit with that change. Comment at: clang/lib/Sema/SemaExpr.cpp:17254 +SourceLocation(), Context.getTranslationUnitDecl()); + } yaxunl wrote: > rjmccall wrote: > > Thanks,

[PATCH] D72742: Don't assume promotable integers are zero/sign-extended already in x86-64 ABI.

rjmccall added a comment. In D72742#1854594 , @emilio wrote: > Could anyone update me with how do they want me to proceed here? Is fixing > the coercions enough to allow this to land? Do I need to make it > target-specific? It needs to be target-specif

[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

rjmccall added a comment. Oh thank goodness, they finally fixed this. Comment at: clang/lib/Sema/SemaDecl.cpp:4365 + // C++ [dcl.typedef]p9: [P1766R1] + // An unnamed class with a typedef name for linkage purposes shall not + if (RD->isInvalidDecl()) Start

[PATCH] D74094: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:3697 + + args.add(EmitAnyExpr(E, ArgSlot), type); } If the argument type has a C++ destructor, will we end its lifetime before we call destructors at the end of the full-expression? CHA

[PATCH] D74094: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:3697 + + args.add(EmitAnyExpr(E, ArgSlot), type); } erik.pilkington wrote: > rjmccall wrote: > > If the argument type has a C++ destructor, will we end its lifetime before > > we call des

[PATCH] D69272: Enable '#pragma STDC FENV_ACCESS' in frontend

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaStmt.cpp:386 + if (FPFeatures.allowFEnvAccess()) { +FunctionDecl *F = getCurFunctionDecl(); +F->setUsesFPIntrin(true); There isn't necessarily a current function declaration; this is usable i

[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:4394 +isa(D)) + continue; + rsmith wrote: > rjmccall wrote: > > This is essentially part of the next paragraph. > > > > I believe `friend` declarations also do not declare "memb

[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM, assuming that we do indeed want to ban friends. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74103/new/ https://reviews.llvm.

[PATCH] D74103: Implement P1766R1: diagnose giving non-C-compatible classes a typedef name for linkage purposes.

rjmccall added a comment. Okay, if the basic idea is to avoid anything that would need the linkage to be computed, that seems like exactly the right goal. I know one of the awful ways to get an early reference to the anonymous type is to define a typedef of like a pointer to it, then use that i

[PATCH] D74183: [IRGen] Add an alignment attribute to underaligned sret parameters

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2077 +getDataLayout().getABITypeAlignment(getTypes().ConvertType(RetTy))) + SRETAttrs.addAlignmentAttr(Align); ArgAttrs[IRFunctionArgs.getSRetArgNo()] = Why only when under

[PATCH] D74116: [Sema][C++] Strawman patch to propagate conversion type in order to specialize the diagnostics

rjmccall added a comment. Hmm. The alternative approach, I suppose, would be to recognize that we're about to emit a generic error and just recheck assignment constraints to recompute the AssignConvertType. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74116/new/ https://reviews.llvm

[PATCH] D74116: [Sema][C++] Strawman patch to propagate conversion type in order to specialize the diagnostics

rjmccall added a comment. This shouldn't be an expensive function call, and being a little inefficient on the error path to get better diagnostics is a good trade-off. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74116/new/ https://reviews.llvm.org/D74116 __

[PATCH] D74183: [IRGen] Add an alignment attribute to underaligned sret parameters

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2077 +getDataLayout().getABITypeAlignment(getTypes().ConvertType(RetTy))) + SRETAttrs.addAlignmentAttr(Align); ArgAttrs[IRFunctionArgs.getSRetArgNo()] = dexonsmith wrote: >

[PATCH] D74183: [IRGen] Add an alignment attribute to underaligned sret parameters

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2077 +getDataLayout().getABITypeAlignment(getTypes().ConvertType(RetTy))) + SRETAttrs.addAlignmentAttr(Align); ArgAttrs[IRFunctionArgs.getSRetArgNo()] = rjmccall wrote: > d

[PATCH] D74094: [IRGen] Emit lifetime intrinsics around temporary aggregate argument allocas

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

[PATCH] D69272: Enable '#pragma STDC FENV_ACCESS' in frontend

rjmccall added a comment. In D69272#1865266 , @rsmith wrote: > I don't see any changes to the constant evaluator here. You need to properly > handle constant evaluation within `FENV_ACCESS ON` contexts, somehow, or > you'll miscompile floating-point oper

[PATCH] D74387: [SYCL] Do not diagnose use of __float128

rjmccall added a comment. The right approach here is probably what we do in ObjC ARC when we see types that are illegal in ARC: in system headers, we allow the code but add a special `UnavailableAttr` to the declaration so that it can't be directly used. That is straightforward enough that I th

[PATCH] D74436: Change clang option -ffp-model=precise to select ffp-contract=on

rjmccall added a comment. I agree that the default mode should be standards-compliant, which I believe permits `fp-contract=on` (contraction within expressions) but not `fp-contract=fast` (arbitrary contraction). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://review

[PATCH] D74116: [Sema][C++] Strawman patch to propagate conversion type in order to specialize the diagnostics

rjmccall added a comment. Ah, yes, we're laxer about these things in C than we are in C++. If we do this, we'll need to go through `DiagnoseAssignmentResult` and make sure that everything (except `Compatible`, which we'll never pass) triggers an error in C++ mode. Is there a good reason to ju

[PATCH] D74116: [Sema][C++] Strawman patch to propagate conversion type in order to specialize the diagnostics

rjmccall added a comment. The C diagnostic changes seem like improvements, yeah. The virtue of re-using the same infrastructure across language modes. Comment at: clang/lib/Sema/SemaExprCXX.cpp:3880 +ToType, From->getType(), From, Action); +// assert(Diagnosed &&

[PATCH] D74436: Change clang option -ffp-model=precise to select ffp-contract=on

rjmccall added a comment. Yeah, I agree with Steve here. The great virtue of `-ffp-contract=on` over the more aggressive modes is that FMA formation is purely local to a single source expression, which means there's really no obstacle to treating it as implementation-guaranteed semantics. Suc

[PATCH] D74116: [Sema][C++] Strawman patch to propagate conversion type in order to specialize the diagnostics

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaExprCXX.cpp:3880 +ToType, From->getType(), From, Action); +// assert(Diagnosed && "failed to diagnose bad conversion"); +(void)Diagnosed; Anastasia wrote: > rjmccall wrote: > > This as

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

rjmccall added a comment. In D72231#1878528 , @nathanchance wrote: > There appear to a be semantic difference between GCC and clang with the > current version of this patch which results in a lot of additional warnings > in the Linux kernel: https://god

[PATCH] D74387: [SYCL] Do not diagnose use of __float128

rjmccall added a comment. In D74387#1874612 , @Fznamznon wrote: > > In D74387#1869845 , @rjmccall > > wrote: > > > >> The right approach here is probably what we do in ObjC ARC when we see > >> types that are ill

[PATCH] D74643: [Parse] Consider attributes in `TryParsePtrOperatorSeq`.

rjmccall added inline comments. Comment at: clang/lib/Parse/ParseTentative.cpp:798 +if (Tok.is(tok::l_square)) { + ConsumeBracket(); + if (!SkipUntil(tok::r_square)) Do you want to check for double-brackets here? Repository: rG LLVM

[PATCH] D74361: [Clang] Uninitialize attribute on global variables

rjmccall added a comment. I'm not sure if it's a good idea to be re-using the existing attribute like this. It's not that unreasonable, I guess, but I'd like to hear JF's thoughts. I think you do need to update the documentation to mention the impact on globals. For normal targets (i.e. ones

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

rjmccall added a comment. Thanks, I think this approach is really improving the existing code. Comment at: clang/include/clang/AST/GlobalDecl.h:40 + Stub = 1, +}; + The attribute here is `CUDAGlobalAttr`; should this be named in terms of CUDA, or is the CUDA

[PATCH] D74643: [Parse] Consider attributes in `TryParsePtrOperatorSeq`.

rjmccall accepted this revision. rjmccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Parse/ParseTentative.cpp:780 + if (!SkipUntil(tok::r_square) || Tok.isNot(tok::r_square)) +return false; + ConsumeBracket(); -

[PATCH] D74643: [Parse] Consider attributes in `TryParsePtrOperatorSeq`.

rjmccall accepted this revision. rjmccall added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74643/new/ https://reviews.llvm.org/D74643 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D73186: [AST] Add fixed-point multiplication constant evaluation.

rjmccall added inline comments. Comment at: clang/lib/Basic/FixedPoint.cpp:242 + } else +Overflowed = Result < Min || Result > Max; + ebevhan wrote: > rjmccall wrote: > > leonardchan wrote: > > > ebevhan wrote: > > > > rjmccall wrote: > > > > > ebevhan wrote

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

rjmccall added inline comments. Comment at: clang/include/clang/AST/GlobalDecl.h:40 + Stub = 1, +}; + tra wrote: > rjmccall wrote: > > The attribute here is `CUDAGlobalAttr`; should this be named in terms of > > CUDA, or is the CUDA model sufficiently different

[PATCH] D72231: [Sema] Adds the pointer-to-int-cast diagnostic

rjmccall added a comment. In D72231#1881760 , @nickdesaulniers wrote: > In D72231#1879347 , @rjmccall wrote: > > > In D72231#1878528 , @nathanchance > > wrote: > > > > > Th

[PATCH] D72742: Don't assume promotable integers are zero/sign-extended already in x86-64 ABI.

rjmccall added a comment. > Didn't we work this out already when John added the alignment tracking stuff? > I remember this bug involving libjpegturbo standalone assembly receiving a > 32-bit argument, and then using the full 64-bit RDI register to read it, but > clang stopped zero extending it

[PATCH] D72114: [MS] Overhaul how clang passes overaligned args on x86_32

rjmccall added inline comments. Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:91 bool InAllocaSRet : 1;// isInAlloca() + bool InAllocaIndirect : 1;// isInAlloca() bool IndirectByVal : 1; // isIndirect() rnk wrote: > rjmccall wrote: > > rnk

[PATCH] D72897: List implicit operator== after implicit destructors in a vtable.

rjmccall added a comment. I see two reasonable approaches here: we can rely on the implicit members being added in the right order by Sema, as this patch seems to do, or we can broaden the special treatment of implicit virtual functions in the v-table layout code. The latter is more complex (w

[PATCH] D72897: List implicit operator== after implicit destructors in a vtable.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. It wouldn't be a bad idea to track that correspondence in the AST just on the general principle that it's useful information (for diagnostics, tooling, etc.) that's otherwise hard to recre

[PATCH] D72820: [FPEnv] Add pragma FP_CONTRACT support under strict FP.

rjmccall added a comment. Remember that the design is that constrained intrinsics must be used whenever *any* code in the function is constrained. It is not unreasonable that part of the function might be constrained and the rest subject to fast-math; it'd be a shame if the intrinsics couldn't

[PATCH] D72970: clang: Only define OBJC_NEW_PROPERTIES when -x objective-c

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

[PATCH] D73185: [AST] Add fixed-point subtraction constant evaluation.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM other than the request to split the commit. Comment at: clang/include/clang/Basic/FixedPoint.h:82 + unsigned IsSaturated: 1; + unsigned HasUnsignedPadding : 1;

[PATCH] D73186: [AST] Add fixed-point multiplication constant evaluation.

rjmccall added inline comments. Comment at: clang/lib/Basic/FixedPoint.cpp:242 + } else +Overflowed = Result < Min || Result > Max; + If the maximum expressible value is *k*, and the fully-precise multiplication yields *k+e* for some epsilon *e* that isn't

[PATCH] D72959: Relative VTables ABI on Fuchsia

rjmccall added a comment. There's two independently-useful things here for the relocation, right? First, it's useful to be able to express a relocation to a symbol that has the semantics of a particular function but doesn't necessarily have its address, and that concept could be used across ma

[PATCH] D72959: Relative VTables ABI on Fuchsia

rjmccall added a comment. Okay. In that case, I would ask that if you're considering going through the work of adding relocations, please consider adding both scaled and unscaled relative-offset-to-equivalent-symbol. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[PATCH] D73257: [AST] Compress the FixedPointSemantics type better.

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

[PATCH] D73186: [AST] Add fixed-point multiplication constant evaluation.

rjmccall added inline comments. Comment at: clang/lib/Basic/FixedPoint.cpp:242 + } else +Overflowed = Result < Min || Result > Max; + ebevhan wrote: > ebevhan wrote: > > rjmccall wrote: > > > If the maximum expressible value is *k*, and the fully-precise >

[PATCH] D73185: [AST] Add fixed-point subtraction constant evaluation.

rjmccall accepted this revision. rjmccall added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73185/new/ https://reviews.llvm.org/D73185 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D73186: [AST] Add fixed-point multiplication constant evaluation.

rjmccall added inline comments. Comment at: clang/lib/Basic/FixedPoint.cpp:242 + } else +Overflowed = Result < Min || Result > Max; + leonardchan wrote: > ebevhan wrote: > > rjmccall wrote: > > > ebevhan wrote: > > > > ebevhan wrote: > > > > > rjmccall wrote

[PATCH] D73187: [AST] Add fixed-point division constant evaluation.

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

[PATCH] D73545: Fix a crash when casting _Complex and ignoring the results

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

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

rjmccall requested changes to this revision. rjmccall added inline comments. This revision now requires changes to proceed. Comment at: clang/test/Misc/warning-flags.c:41 CHECK-NEXT: warn_asm_label_on_auto_decl +CHECK-NEXT: warn_bad_cxx_cast_nested_pointer_addr_space CHECK-

[PATCH] D73183: [CodeGen] Emit IR for fixed-point unary operators.

rjmccall added a comment. Please test prefix increment/decrement as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73183/new/ https://reviews.llvm.org/D73183 ___ cfe-commits mailing list cfe-commi

[PATCH] D73184: [CodeGen] Emit IR for compound assignment with fixed-point operands.

rjmccall added a comment. Please test the cases where the result is used; this should be handled for you automatically, but still. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73184/new/ https://reviews.llvm.org/D73184

[PATCH] D73360: [OpenCL] Restrict address space conversions in nested pointers

rjmccall added a comment. Is there no follow-up code when actually emitting the failure diagnostic which tries to figure out a more specific cause of failure? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73360/new/ https://reviews.llvm.org/D73360 ___

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