[PATCH] D77954: [CUDA][HIP] Fix host/device based overload resolution

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:9481 + // emitted, Cand1 is not better than Cand2. This rule should have precedence + // over other rules. + // Please add `[CUDA]` or something similar to the top of this comment so t

[PATCH] D77743: [HIP] Emit symbols with kernel name in host binary

rjmccall added a comment. Ah, too bad. Is there any way to suppress that in debug info? I'm not sure there's any other way to satisfy the competing requirements here, and if it's not going to be consistent, it'd be better to avoid the complexity of mangling the thunk differently. CHANGES SI

[PATCH] D77954: [CUDA][HIP] Fix host/device based overload resolution

rjmccall added a reviewer: echristo. rjmccall added a subscriber: echristo. rjmccall added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:9481 + // emitted, Cand1 is not better than Cand2. This rule should have precedence + // over other rules. + // -

[PATCH] D76182: [AVR] Support aliases in non-zero address space

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

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. Thanks. Just a bunch of fairly minor requests now. Comment at: clang/include/clang/AST/Expr.h:3474 + size_t offsetOfTrailingStorage() const; + size_t offsetOfTrailingStorage(); + You don't need the non-const overload.

[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

rjmccall added a comment. Reading through the rest of the spec. Comment at: clang/docs/LanguageExtensions.rst:500 +Clang provides a matrix extension, which is currently being implemented. See +:ref:`matrixsupport` for more details. + rjmccall wrote: > This shou

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added inline comments. Comment at: clang/lib/Analysis/BodyFarm.cpp:120 +VK_RValue, OK_Ordinary, SourceLocation(), +FPOptions::defaultWithoutTrailingStorage(C)); } mibintc wrote: > rjmccall

[PATCH] D77592: [NFC}[CodeGen] Make VTable initialization a method of CGCXXABI

rjmccall added a comment. This is a global switch, right, not something that's mix-and-match between hierarchies or even between classes? I think I would prefer that the ABI object just tell us the expected layout of a v-table entry (as an enum) rather than forcing a bunch of different callbac

[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

rjmccall added inline comments. Comment at: clang/docs/MatrixSupport.rst:211 + +``M __builtin_matrix_column_load(T *ptr, int row, int col, int stride)`` + fhahn wrote: > rjmccall wrote: > > This name sounds like it's loading a column, when I think you're saying

[PATCH] D77592: [NFC}[CodeGen] Make VTable initialization a method of CGCXXABI

rjmccall added a comment. Thanks. Having an enum like this would also be important if we wanted to implement the Itanium rule (as in, the actual Itanium architecture, not the generic C++ ABI) that v-tables contain "inline" function descriptors rather than pointers to them. (On Itanium, a C fu

[PATCH] D77545: Represent FP options in AST by special Expression node

rjmccall added a comment. In D77545#1977688 , @sepavloff wrote: > In D77545#1974509 , @rjmccall wrote: > > > For example, in this code: > > > > const float global = 2.22 + 3.33; > > > > #pragma STDV FENV_ROUND

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

rjmccall added inline comments. Comment at: clang/include/clang/Serialization/ASTBitCodes.h:1892 + /// A FixedPointLiteral record + EXPR_FIXEDPOINT_LITERAL, }; Thanks! I'm sorry to be extremely picky about this, but: - Please put the comment on th

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

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

[PATCH] D78134: [Sema] Don't apply an lvalue-to-rvalue conversion to a discarded-value expression if it has an array type

rjmccall added a comment. C++ says: [expr.context]p2: > In some contexts, an expression only appears for its side effects. Such an > expression is called a discarded-value expression. The array-to-pointer > (7.3.2) and function-to-pointer (7.3.3) standard conversions are not applied. > The lv

[PATCH] D78098: [CGExprAgg] Fix infinite loop in `findPeephole`

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprAgg.cpp:688 + +op = castE; } I liked the structure of the old code better, in case we want to look through other kinds of expressions. Please just add `op = castE->getSubExpr()` before

[PATCH] D78125: [AVR] Use the correct address space for non-prototyped function calls

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:5051 llvm::Type *CalleeTy = getTypes().GetFunctionType(FnInfo); -CalleeTy = CalleeTy->getPointerTo(); +int AS = getTypes().getDataLayout().getProgramAddressSpace(); +CalleeTy = CalleeTy->get

[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

rjmccall added inline comments. Comment at: clang/docs/MatrixTypes.rst:79 + floating point type, convert the integer or floating point operand to the + underlying element type of the operand of matrix type. + fhahn wrote: > rjmccall wrote: > > You should standa

[PATCH] D78117: [AVR] Define __ELF__

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. I agree that these tests are in the wrong directory; please prepare a patch that moves them. This patch is okay to go, though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

[PATCH] D78134: [Sema] Don't apply an lvalue-to-rvalue conversion to a discarded-value expression if it has an array type

rjmccall added a comment. That doesn't seem to be how we use it in practice; I'd just change the comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78134/new/ https://reviews.llvm.org/D78134 ___ cf

[PATCH] D78098: [CGExprAgg] Fix infinite loop in `findPeephole`

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprAgg.cpp:688 + +op = castE; } ekatz wrote: > rjmccall wrote: > > I liked the structure of the old code better, in case we want to look > > through other kinds of expressions. Please just

[PATCH] D78125: [AVR] Use the correct address space for non-prototyped function calls

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

[PATCH] D78134: [Sema] Don't apply an lvalue-to-rvalue conversion to a discarded-value expression if it has an array type

rjmccall added a comment. In D78134#1982291 , @rsmith wrote: > In D78134#1981866 , @rsmith wrote: > > > I'm going to take this to CWG. > > > So far, the direction the wind is blowing is that attempting to perform an

[PATCH] D78125: [AVR] Use the correct address space for non-prototyped function calls

rjmccall added inline comments. Comment at: clang/test/CodeGen/address-space-avr.c:3 + +// CHECK: define void @bar() addrspace(1) +// CHECK: call addrspace(1) void bitcast (void (...) addrspace(1)* @foo to void (i16) addrspace(1)*)(i16 3) MaskRay wrote: > Add a

[PATCH] D78098: [CGExprAgg] Fix infinite loop in `findPeephole`

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGExprAgg.cpp:688 + +op = castE; } ekatz wrote: > rjmccall wrote: > > ekatz wrote: > > > rjmccall wrote: > > > > I liked the structure of the old code better, in case we want to look > > > > th

[PATCH] D78163: [AVR][NFC] Move preprocessor tests to Preprocessor directory

rjmccall added a comment. Looks like the established convention would be to just put these tests in `test/Preprocessor` with an `avr-` prefix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78163/new/ https://reviews.llvm.org/D78163

[PATCH] D78163: [AVR][NFC] Move preprocessor tests to Preprocessor directory

rjmccall added a comment. I don't think the REQUIRES is needed unless there's something special about AVR as a target. Clang doesn't conditionally compile out frontend target support. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78163/new/ https

[PATCH] D78098: [CGExprAgg] Fix infinite loop in `findPeephole`

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

[PATCH] D78098: [CGExprAgg] Fix infinite loop in `findPeephole`

rjmccall added a comment. Oh, no, not quite. Please name the test case to something more specific; `pr45476.cpp` would be fine. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78098/new/ https://reviews.llvm.org/D78098 ___ cfe-commits maili

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. Minor fixes, then LGTM. Comment at: clang/lib/AST/ASTImporter.cpp:6807 + importChecked(Err, ToComputationLHSType), + importChecked(Err, ToComputationResultType)); } Oh, these two calls need to use `E->getFPFeatures(Importer.

[PATCH] D78125: [AVR] Use the correct address space for non-prototyped function calls

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

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. In D76384#1984498 , @mibintc wrote: > Responding to @rjmccall 's review. John, after this is approved I want to > proceed with pragma float_control as proposed in D72841 > . Can you recommend an

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

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

[PATCH] D76384: Move FPFeatures from BinaryOperator bitfields to Trailing storage

rjmccall added a comment. In D76384#1984683 , @mibintc wrote: > In D76384#1984584 , @rjmccall wrote: > > > I *would* like an NFC patch first that renames `FPFeatures` to something > > like `CurFPFeatures` in order

[PATCH] D77592: [NFC][CodeGen] Add enum for selecting the layout of components in the vtable

rjmccall added a comment. I'm not sure if the AST-level v-table layout abstraction really cares about these differences. I don't think it vends byte offsets into the v-table, just slot indices (i.e. word offsets). Comment at: clang/include/clang/AST/VTableBuilder.h:380 +

[PATCH] D76612: [Matrix] Add draft specification for matrix support in Clang.

rjmccall added inline comments. Comment at: clang/docs/MatrixTypes.rst:29 +A *matrix element type* must be a real type (as in C99 6.2.5p17) excluding +enumeration types or an implementation-defined half-precision floating point +type, otherwise the program is ill-formed.

[PATCH] D79744: clang: Add address space to indirect abi info and use it for kernels

rjmccall added a comment. In D79744#2069962 , @jdoerfert wrote: > In D79744#2069786 , @arsenm wrote: > > > In D79744#2069774 , @arsenm wrote: > > > > > I think this is conver

[PATCH] D79052: [clang codegen] Fix alignment of "Address" for incomplete array pointer.

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

[PATCH] D81138: [SemaOverload] Use iterator_range to iterate over VectorTypes (NFC).

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. (But you can use QualType by value.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81138/new/ https://reviews.llvm.org/D81138

[PATCH] D80462: Fix floating point math function attributes definition.

rjmccall added a comment. Can we do this conservatively only if something is changed within the function definition, and otherwise respect the global settings? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80462/new/ https://reviews.llvm.org/D8046

[PATCH] D81178: [FPEnv] Initialization of C++ globals not strictfp aware

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:4348 + /// Query for the use of constrained floating point math + bool isStrictFP() { return Builder.getIsFPConstrained(); } + How is this set that we don't end up also setting it a

[PATCH] D80462: Fix floating point math function attributes definition.

rjmccall added a comment. We have a very similar problem with the constrained FP intrinsics where the use of the intrinsics anywhere in a function requires (1) an attribute to be set on the function and (2) all the other code in the function to be emitted with intrinsics. What I've been treati

[PATCH] D81311: [RFC] LangRef: Define inmem parameter attribute

rjmccall added a comment. Most of the generalized optimization properties of this attribute seem to be redundant with existing attributes. What are the properties you're trying to convey exactly? The argument is a pointer to memory of a certain size and alignment, and that memory is known to

[PATCH] D81311: [RFC] LangRef: Define inmem parameter attribute

rjmccall added a comment. In D81311#2077944 , @arsenm wrote: > In D81311#2077868 , @rjmccall wrote: > > > Most of the generalized optimization properties of this attribute seem to > > be redundant with existing att

[PATCH] D81311: [RFC] LangRef: Define inmem parameter attribute

rjmccall added a comment. I wonder if `byref` would be a better name for this, as a way to say that the object is semantically a direct argument that's being passed by implicit reference. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81311/new/ https://reviews.llvm.org/D81311 _

[PATCH] D76794: [Matrix] Implement * binary operator for MatrixType.

rjmccall added a comment. Your IRGen test cases cover a lot of ground, but please add more Sema test cases that go over the basics: element types matching, column/row counts matching, multiplication by inappropriate scalars, etc. Otherwise LGTM! Comment at: clang/lib/Sema/Se

[PATCH] D72778: [Matrix] Add __builtin_matrix_transpose to Clang.

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1642 + return cast(Ty.getCanonicalType()); +}; + Unnecessary semicolon. I think it's probably clearer just to `castAs()` inline in the code rather than introducing this trivial wrappe

[PATCH] D76794: [Matrix] Implement * binary operator for MatrixType.

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

[PATCH] D72778: [Matrix] Add __builtin_matrix_transpose to Clang.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:15051 + + Expr *Arg = TheCall->getArg(0); + if (!Arg->getType()->isConstantMatrixType()) { fhahn wrote: > rjmccall wrote: > > When a builtin has custom type-checking (`t`), you need to

[PATCH] D72778: [Matrix] Add __builtin_matrix_transpose to Clang.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM with the given testing suggestion. Comment at: clang/lib/Sema/SemaChecking.cpp:15051 + + Expr *Arg = TheCall->getArg(0); + if (!Arg->getType()->isConstantM

[PATCH] D81178: [FPEnv] Initialization of C++ globals not strictfp aware

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:4348 + /// Query for the use of constrained floating point math + bool isStrictFP() { return Builder.getIsFPConstrained(); } + kpn wrote: > rjmccall wrote: > > How is this set that

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

rjmccall added a comment. `SubstTemplateTypeParmType` is a "sugar" type node, like a `typedef`, and code should generally be looking through it automatically by using `getAs` rather than `isa` / `dyn_cast`. Comment at: clang/lib/Sema/SemaChecking.cpp:15107 + { +ExprResul

[PATCH] D72778: [Matrix] Add __builtin_matrix_transpose to Clang.

rjmccall added a comment. Yeah, that's fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72778/new/ https://reviews.llvm.org/D72778 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lis

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10789 +def err_builtin_matrix_scalar_constant_unsigned_expr_arg: Error< + "%0 argument must be a constant unsigned integer expression">; + These are the same now.

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:15130 + +ElementTy.removeLocalConst(); +if (!ConstantMatrixType::isValidElementType(ElementTy)) { fhahn wrote: > rjmccall wrote: > > It's almost never correct to do "local" qualif

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

rjmccall added inline comments. Comment at: clang/include/clang/Sema/Sema.h:4708 + /// conversion. + ExprResult tryConvertExprToTy(Expr *E, QualType Ty); + Please spell out "type" in the method name. Comment at: clang/include/clang/Sema/Sema.

[PATCH] D80462: Fix floating point math function attributes definition.

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

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added a comment. Linking compiler-rt is something that should be automatically happening. It's possible that compiler-rt will have different maximum widths on different targets, though. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7939 "to a non-c

[PATCH] D81311: [RFC] LangRef: Define inmem parameter attribute

rjmccall added a comment. In D81311#2083295 , @arsenm wrote: > In D81311#2078235 , @rjmccall wrote: > > > I wonder if `byref` would be a better name for this, as a way to say that > > the object is semantically a d

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:7939 "to a non-const integer (%0 invalid)">; +def err_overflow_builtin_extint_size : Error< + "_ExtInt argument larger than 64-bits to overflow builtin requires runtime "

[PATCH] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/include/clang/Sema/Sema.h:12126 + ExprResult SemaBuiltinMatrixColumnMajorLoadOverload(CallExpr *TheCall, +

[PATCH] D77592: [clang] Frontend components for the relative vtables ABI

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

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:327 return true; TheCall->setArg(2, Arg.get()); } I know this is existing code, but this is a broken mess. Please change this to: ``` ExprResult Arg = DefaultFunctionAr

[PATCH] D81311: [RFC] LangRef: Define inmem parameter attribute

rjmccall added a comment. In D81311#2086227 , @jdoerfert wrote: > Do we allow `inmem` to be used for other purposes? I would assume the answer > is yes, as we do not forbid it. I don't know what else we might use it for off-hand, but yes, I think the f

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added inline comments. Comment at: clang/test/Sema/builtins-overflow.c:39 +_ExtInt(129) result; +_Bool status = __builtin_mul_overflow(x, y, &result); // expected-error {{signed _ExtInt of bit sizes greater than 128 not supported}} + } jtmott-i

[PATCH] D81624: [CodeGen] Simplify the way lifetime of block captures is extended

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

[PATCH] D81311: [RFC] LangRef: Define inmem parameter attribute

rjmccall added a comment. In D81311#2087592 , @jdoerfert wrote: > In D81311#2086326 , @rjmccall wrote: > > > In D81311#2086227 , @jdoerfert > > wrote: > > > > > Do we allow

[PATCH] D81311: [RFC] LangRef: Define inmem parameter attribute

rjmccall added a comment. In D81311#2088568 , @jdoerfert wrote: > In D81311#2088075 , @rjmccall wrote: > > > In D81311#2087592 , @jdoerfert > > wrote: > > > > > In D81311#20

[PATCH] D80462: Fix floating point math function attributes definition.

rjmccall closed this revision. rjmccall added a comment. To ssh://github.com/llvm/llvm-project a98d618f6e5f..7fac1acc6171 master -> master Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80462/new/ https://reviews.llvm.org/D80462 _

[PATCH] D81420: Fix size for _ExtInt types with builtins

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:327 return true; TheCall->setArg(2, Arg.get()); } jtmott-intel wrote: > rjmccall wrote: > > I know this is existing code, but this is a broken mess. Please change > > thi

[PATCH] D81420: Fix size for _ExtInt types with builtins

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

[PATCH] D81795: [clang] Enable -mms-bitfields by default for mingw targets

rjmccall added a comment. Seems reasonable; GCC is the "system compiler" for this platform. Does `isWindowsGNUEnvironment` exactly track the condition that GCC uses? It's just MinGW, not Cygwin? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D817

[PATCH] D81857: Fix ConstantAggregateBuilderBase::getRelativeOffset

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

[PATCH] D81795: [clang] Enable -mms-bitfields by default for mingw targets

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

[PATCH] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:2280 } + FPOptionsOverride getFPFeatures(const LangOptions &LO) const { +if (UnaryOperatorBits.HasFPFeatures) I would call this one getFPOptionOverrides or getOverriddenFPOptions.

[PATCH] D72782: [Matrix] Add __builtin_matrix_column_store to Clang.

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10792 def err_builtin_matrix_pointer_arg: Error< - "%select{first|second}0 argument must be a pointer

[PATCH] D82392: [CodeGen] Add public function to emit C++ destructor call.

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

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

rjmccall added a comment. Can the missing bit just be added? It seems to me that frontends ought to be able to emit the obvious intrinsic for the semantic operation here rather than having to second-guess the backend. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D82781: [OpenCL] Fix missing address space deduction in template variables

rjmccall added a comment. Seems like you shouldn't do it earlier if the type is dependent. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82781/new/ https://reviews.llvm.org/D82781 ___ cfe-commits maili

[PATCH] D82999: [CodeGen] Check the cleanup flag before destructing temporaries created in conditional expressions

rjmccall added a comment. Please adjust the commit message to be clear that this is about lifetime-extended temporaries; it's not like we got this wrong for all temporaries. Do we need to do anything to ensure that the after-full-expression cleanup is configured conditionally? Repository:

[PATCH] D82999: [CodeGen] Check the cleanup flag before destructing lifetime-extended temporaries created in conditional expressions

rjmccall added a comment. In D82999#2129031 , @ahatanak wrote: > After-full-expression cleanup looks fine to me. `pushCleanupAfterFullExpr` > sets the flags and saves the values when it's in a conditional branch. > > I think ideally we shouldn't have to c

[PATCH] D82999: [CodeGen] Check the cleanup flag before destructing lifetime-extended temporaries created in conditional expressions

rjmccall added a comment. In D82999#2129417 , @ahatanak wrote: > In test case `test13` in clang/test/CodeGenCXX/exceptions.cpp, I think you > can turn `invoke void @_ZN6test131AC1Ev` into `call void @_ZN6test131AC1Ev`, > no? If the false expression throw

[PATCH] D79279: Add overloaded versions of builtin mem* functions

rjmccall added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1605 + })); + }; + I am not a fan of this lambda style, not because I dislike lambdas, but because you've pulled a ton of code that's supporting one or two cases (that could

[PATCH] D83317: [Sema] Teach -Wcast-align to compute alignment of CXXThisExpr

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

[PATCH] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

rjmccall added a comment. I agree with Eli that this should be considered a bugfix in the implementation of a recent language change and should just be rolled out consistently for all targets. If this were a five-year-old feature, the ABI considerations would be different, but it's not. CHAN

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

rjmccall added a comment. Would it be sensible to use a technical design more like what the matrix folks are doing, where LLVM provides a small interface for emitting operations with various semantics? FixedPointSemantics would move to that header, and Clang would just call into it. That way

[PATCH] D79730: [NFCi] Switch ordering of ParseLangArgs and ParseCodeGenArgs.

rjmccall added a comment. Either way, I think. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79730/new/ https://reviews.llvm.org/D79730 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://l

[PATCH] D82513: [CodeGen] Store the return value of the target function call to the thunk's return value slot directly when the return type is an aggregate instead of doing so via a temporary

rjmccall added a comment. This seems fine. I do wonder if the "real" bug is that this ought to be handled properly in EmitReturnFromThunk, but regardless, the fix seems acceptable. Is there a similar bug with trivial_abi? Should we have a test case for that? Repository: rG LLVM Github Mon

[PATCH] D80858: [CUDA][HIP] Support accessing static device variable in host code

rjmccall added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:10068 +isa(D) && cast(D)->isFileVarDecl() && +cast(D)->getStorageClass() == SC_Static) { + return GVA_StrongExternal; JonChesterfield wrote: > yaxunl wrote: > > rjmccall

[PATCH] D83502: Change behavior with zero-sized static array extents

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2515 +} else { + AI->addAttr(llvm::Attribute::NonNull); +} Isn't the old logic still correct? If the element size is static and the element

[PATCH] D82513: [CodeGen] Store the return value of the target function call to the thunk's return value slot directly when the return type is an aggregate instead of doing so via a temporary

rjmccall added a comment. I agree that avoiding the copy is best. However, at the very least, if that function isn't going to handle the aggregate case correctly, it should assert that it isn't in it. Otherwise this LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

rjmccall added a comment. In D82663#2144219 , @ebevhan wrote: > In D82663#2142426 , @rjmccall wrote: > > > Would it be sensible to use a technical design more like what the matrix > > folks are doing, where LLVM pr

[PATCH] D83502: Change behavior with zero-sized static array extents

rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2515 +} else { + AI->addAttr(llvm::Attribute::NonNull); +} aaron.ballman wrote: > rjmccall wrote: > > Isn't the old logic still correct? If

[PATCH] D83502: Change behavior with zero-sized static array extents

rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Comment at: clang/lib/CodeGen/CGCall.cpp:2515 +} else { + AI->addAttr(llvm::Attribute::NonNull); +} ---

[PATCH] D82513: [CodeGen] Store the return value of the target function call to the thunk's return value slot directly when the return type is an aggregate instead of doing so via a temporary

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

[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

rjmccall added a comment. In D82663#2153176 , @ebevhan wrote: > In D82663#2144551 , @rjmccall wrote: > > > In D82663#2144219 , @ebevhan wrote: > > > > > In D82663#2142426

[PATCH] D81311: [RFC] LangRef: Define byref parameter attribute

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

[PATCH] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

rjmccall added inline comments. Comment at: clang/include/clang/AST/Stmt.h:620 +//unsigned FPFeatures : 21; +unsigned FPFeatures : 32; }; riccibruno wrote: > mibintc wrote: > > This is a temporary change, I know you don't want me to change the assert

[PATCH] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

rjmccall added a comment. Just a bunch of minor suggestions. LGTM if you get all the tests worked out and it actually works the way you want on the tests. Comment at: clang/include/clang/AST/Expr.h:2280 } + FPOptionsOverride getFPFeatures(const LangOptions &LO) const { +

[PATCH] D82473: [Matrix] Use 1st/2nd instead of first/second in matrix diags.

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

[PATCH] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

rjmccall added a comment. In D81869#2112440 , @mibintc wrote: > I decided that I shouldn't make float options that define a macro, like > -ffast-math, as BENIGN_LANGOPT, I made ffp-contract= , fp-exception-behavior > and rounding-mode BENIGN That seems

<    10   11   12   13   14   15   16   17   18   19   >