[PATCH] D85710: Code generation support for lvalue bit-field conditionals.

2020-08-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I always imagined that we'd implement this by just inverting the emission: emit the RHS of the assignment and then pass a callback down to a function that descended into conditional operators and called the callback when it hit a leaf. Having an LValue case that's an

[PATCH] D83325: [Sema] Iteratively strip sugar when removing address spaces.

2020-08-10 Thread John McCall via Phabricator via cfe-commits
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/D83325/new/ https://reviews.llvm.org/D83325

[PATCH] D85312: [ADT] Move FixedPoint.h from Clang to LLVM.

2020-08-10 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85312/new/ https://reviews.llvm.org/D85312 ___ cfe-commits mailing list

[PATCH] D84992: [clang codegen] Use IR "align" attribute for static array arguments.

2020-08-10 Thread John McCall via Phabricator via cfe-commits
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Okay. LGTM, then. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84992/new/ https://reviews.llvm.org/D84992

[PATCH] D84992: [clang codegen] Use IR "align" attribute for static array arguments.

2020-08-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:2503 + llvm::Align Alignment = + CGM.getNaturalTypeAlignment(ETy).getAsAlign(); + AI->addAttrs(llvm::AttrBuilder().addAlignmentAttr(Alignment));

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

2020-08-07 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D75574#2202136 , @theraven wrote: > This feature looks generally useful. A few small suggestions: > > - This is really a way of transforming a formal protocol into an informal > protocol. Objective-C has had a convention of

[PATCH] D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary

2020-08-07 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D82317#2200964 , @guiand wrote: > In D82317#2200789 , @rjmccall wrote: > >> Are you seriously adding an attribute to literally every argument and return >> value? Why is this the

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

2020-08-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D79279#2200916 , @rsmith wrote: > In D79279#2197176 , @rjmccall wrote: > >> I thought part of the point of `__builtin_memcpy` was so that C library >> headers could do `#define

[PATCH] D82317: [Clang/Test]: Update tests where `noundef` attribute is necessary

2020-08-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Are you seriously adding an attribute to literally every argument and return value? Why is this the right representation? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82317/new/ https://reviews.llvm.org/D82317

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

2020-08-06 Thread John McCall via Phabricator via cfe-commits
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/D79744/new/ https://reviews.llvm.org/D79744 ___ cfe-commits mailing list

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

2020-08-06 Thread John McCall via Phabricator via cfe-commits
rjmccall added a reviewer: theraven. rjmccall added a comment. One thing that's come up so far: you generally need to be looking through non-runtime protocols, not ignoring them. This matters when non-runtime protocols inherit from ordinary protocols. It may be useful to provide a generic

[PATCH] D85113: [ABI][NFC] Fix the confusion of ByVal and ByRef argument names

2020-08-05 Thread John McCall via Phabricator via cfe-commits
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/D85113/new/ https://reviews.llvm.org/D85113

[PATCH] D83325: [Sema] Be more thorough when unpacking the AS-qualified pointee for a pointer conversion.

2020-08-05 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. There's no way to do that, no. Stripping sugar down to the point where you don't have that qualifier anymore is the best we can do. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83325/new/

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

2020-08-05 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I thought part of the point of `__builtin_memcpy` was so that C library headers could do `#define memcpy(x, y, z) __builtin_memcpy(x, y, z)`. If so, the conformance issue touches `__builtin_memcpy` as well, not just calls to the library builtin. If that's not true,

[PATCH] D85319: [analyzer] Get info from the LLVM IR for precision

2020-08-05 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Thanks. It'd be a good idea to mention that this is contingent on that discussion in the patch summary. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85319/new/ https://reviews.llvm.org/D85319

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

2020-08-05 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:8816 + // FIXME: Should use byref when promoting pointers in structs, but this + // requires adding implementing the coercion. + if (!getContext().getLangOpts().OpenCL && LTy == OrigLTy &&

[PATCH] D85319: [analyzer] Get info from the LLVM IR for precision

2020-08-05 Thread John McCall via Phabricator via cfe-commits
rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. This seems a huge architectural change that we need to talk about. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85319/new/

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

2020-08-05 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2446 +order in which they occur (and in which they are observable) can only be +guaranteed using appropriate fences around the function call. Element size must +therefore be a lock-free size for the

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

2020-08-05 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1997 case ABIArgInfo::Ignore: + case ABIArgInfo::IndirectAliased: return false; arsenm wrote: > arsenm wrote: > > rjmccall wrote: > > > arsenm wrote: > > > > rjmccall wrote: >

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

2020-08-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Patch looks basically okay to me, although I'll second Richard's concern that we shouldn't absent-mindedly start producing overloaded `memcpy`s for ordinary `__builtin_memcpy`. Comment at: clang/docs/LanguageExtensions.rst:2446 +order in which they

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

2020-08-04 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Sorry, this slipped out of my mind. I've started the process internally. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75574/new/ https://reviews.llvm.org/D75574 ___

[PATCH] D44536: Avoid segfault when destructor is not yet known

2020-08-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Would you like to pick it back up? We laid out an implementation path: we need to track the fact that a delete was of an incomplete class type in the AST and then unconditionally treat such operations as trivial to destroy in IRGen. Repository: rC Clang CHANGES

[PATCH] D84540: [CodeGen][ObjC] Mark calls to objc_unsafeClaimAutoreleasedReturnValue as notail on x86-64

2020-08-03 Thread John McCall via Phabricator via cfe-commits
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/D84540/new/ https://reviews.llvm.org/D84540

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

2020-08-03 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1997 case ABIArgInfo::Ignore: + case ABIArgInfo::IndirectAliased: return false; arsenm wrote: > rjmccall wrote: > > In principle, this can be `inreg` just as much as Indirect

[PATCH] D83997: [os_log] Improve the way we extend the lifetime of objects passed to __builtin_os_log_format

2020-07-28 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D83997#2169745 , @ahatanak wrote: > The use case for this is a macro in which the call to > `__builtin_os_log_format` that writes to the buffer and the call that uses > the buffer appear in two different statements. For

[PATCH] D84602: [MSP430] Expose msp430_builtin calling convention to C code

2020-07-27 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Is there only one special calling convention, or is there any chance that different builtin functions would use different conventions? I don't have a problem with introducing a new convention even if it's only used for "builtin" functions. Repository: rG LLVM

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

2020-07-24 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:52 +/// IndirectAliased - Similar to Indirect, but the pointer may not be +/// writable. +IndirectAliased, Hmm. I guess there's actually two different potential

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

2020-07-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I agree, that can be done separately. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82999/new/ https://reviews.llvm.org/D82999 ___ cfe-commits mailing list

[PATCH] D83325: [Sema] Be more thorough when unpacking the AS-qualified pointee for a pointer conversion.

2020-07-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. `removeAddrSpaceQualType` should guarantee that it removes the address space qualifier; you shouldn't need to do something special here. That means it needs to iteratively desugar and collect qualifiers as long as the type is still address-space-qualified.

[PATCH] D84343: [AST] Keep FP options in trailing storage of CallExpr

2020-07-23 Thread John McCall via Phabricator via cfe-commits
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/D84343/new/ https://reviews.llvm.org/D84343

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

2020-07-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D79279#2170187 , @jfb wrote: > In D79279#2170157 , @rjmccall wrote: > > > I think the argument is treated as if it were 1 if not given. That's all > > that ordinary memcpy formally

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

2020-07-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I think the argument is treated as if it were 1 if not given. That's all that ordinary memcpy formally guarantees, which seems to work fine (semantically, if not performance-wise) for pretty much everything today. I don't think you need any restrictions on element

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

2020-07-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. I don't think any of these should allow _Atomic unless we're going to give it some sort of consistent atomic semantics (which is hard to imagine being useful), and I think you should just take an extra argument of the minimum access width on all of them uniformly if

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

2020-07-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D79279#2169522 , @jfb wrote: > In D79279#2168649 , @rjmccall wrote: > > > In D79279#2168533 , @jfb wrote: > > > > > In D79279#2168479

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

2020-07-23 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D79279#2168533 , @jfb wrote: > In D79279#2168479 , @rjmccall wrote: > > > Is there a need for an atomic memcpy at all? Why is it useful to allow > > this operation to take on "atomic"

[PATCH] D83997: [os_log] Improve the way we extend the lifetime of objects passed to __builtin_os_log_format

2020-07-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Why is the lifetime extended to the enclosing block scope anyway? I understand why we need a clang.arc.use — the optimizer can't reasonably understand that the object has to live within the buffer — but isn't the buffer only used for the duration of the call? Why is

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

2020-07-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Is there a need for an atomic memcpy at all? Why is it useful to allow this operation to take on "atomic" semantics — which aren't actually atomic because the loads and stores to elements are torn — with hardcoded memory ordering and somewhat arbitrary rules about

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

2020-07-22 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. You need to add user docs for these builtins. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8917 + +def err_atomic_type_must_be_lock_free : Error<"_Atomic type must always be lock-free, %0 isn't">; + I don't know why

[PATCH] D79744: clang: Use byref for aggregate kernel arguments

2020-07-21 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Arguably we should add this attribute to all indirect arguments. I can understand not wanting to update all the test cases, but you could probably avoid adding a new IndirectByRef kind of ABIArgInfo by treating kernels specially in ConstructAttributeList. Or, sorry,

[PATCH] D83812: [clang][RelativeVTablesABI] Do not emit stubs for architectures that support a PLT relocation

2020-07-21 Thread John McCall via Phabricator via cfe-commits
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/D83812/new/ https://reviews.llvm.org/D83812

[PATCH] D84147: Use typedef to represent storage type in FPOption and FPOptionsOverride

2020-07-20 Thread John McCall via Phabricator via cfe-commits
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/D84147/new/ https://reviews.llvm.org/D84147

[PATCH] D83812: [clang][RelativeVTablesABI] Do not emit stubs for architectures that support a PLT relocation

2020-07-17 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:633 + auto Arch = targetTriple.getArch(); + if (Arch == llvm::Triple::aarch64 || Arch == llvm::Triple::x86_64) +return false; Could you add a method to TargetCodeGenInfo for this

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

2020-07-15 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D82663#2153176 , @ebevhan wrote: > In D82663#2144551 , @rjmccall wrote: > > > In D82663#2144219 , @ebevhan wrote: > > > > > In D82663#2142426

[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

2020-07-10 Thread John McCall via Phabricator via cfe-commits
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] D83502: Change behavior with zero-sized static array extents

2020-07-10 Thread John McCall via Phabricator via cfe-commits
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] D83502: Change behavior with zero-sized static array extents

2020-07-10 Thread John McCall via Phabricator via cfe-commits
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] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

2020-07-10 Thread John McCall via Phabricator via cfe-commits
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

[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

2020-07-09 Thread John McCall via Phabricator via cfe-commits
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

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

2020-07-09 Thread John McCall via Phabricator via cfe-commits
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

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

2020-07-09 Thread John McCall via Phabricator via cfe-commits
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: > >

[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

2020-07-09 Thread John McCall via Phabricator via cfe-commits
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

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

2020-07-09 Thread John McCall via Phabricator via cfe-commits
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

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

2020-07-09 Thread John McCall via Phabricator via cfe-commits
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] D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute

2020-07-08 Thread John McCall via Phabricator via cfe-commits
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.

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

2020-07-07 Thread John McCall via Phabricator via cfe-commits
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] D79279: Add overloaded versions of builtin mem* functions

2020-07-03 Thread John McCall via Phabricator via cfe-commits
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] D82999: [CodeGen] Check the cleanup flag before destructing lifetime-extended temporaries created in conditional expressions

2020-07-03 Thread John McCall via Phabricator via cfe-commits
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

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

2020-07-02 Thread John McCall via Phabricator via cfe-commits
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

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

2020-07-02 Thread John McCall via Phabricator via cfe-commits
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] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

2020-07-01 Thread John McCall via Phabricator via cfe-commits
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

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

2020-07-01 Thread John McCall via Phabricator via cfe-commits
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

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

2020-07-01 Thread John McCall via Phabricator via cfe-commits
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] D82392: [CodeGen] Add public function to emit C++ destructor call.

2020-06-29 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Can we do a design more like what we did with constructors? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82392/new/ https://reviews.llvm.org/D82392 ___ cfe-commits mailing

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

2020-06-26 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. Why not legalize to the signed operation? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82663/new/ https://reviews.llvm.org/D82663 ___ cfe-commits mailing list

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

2020-06-24 Thread John McCall via Phabricator via cfe-commits
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

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

2020-06-24 Thread John McCall via Phabricator via cfe-commits
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

2020-06-24 Thread John McCall via Phabricator via cfe-commits
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 ) const { +

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

2020-06-22 Thread John McCall via Phabricator via cfe-commits
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] D81311: [RFC] LangRef: Define byref parameter attribute

2020-06-18 Thread John McCall via Phabricator via cfe-commits
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

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

2020-06-17 Thread John McCall via Phabricator via cfe-commits
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] D81869: Modify FPFeatures to use delta not absolute settings to solve PCH compatibility problems

2020-06-16 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/include/clang/AST/Expr.h:2280 } + FPOptionsOverride getFPFeatures(const LangOptions ) const { +if (UnaryOperatorBits.HasFPFeatures) I would call this one getFPOptionOverrides or getOverriddenFPOptions.

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

2020-06-16 Thread John McCall via Phabricator via cfe-commits
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] D81857: Fix ConstantAggregateBuilderBase::getRelativeOffset

2020-06-15 Thread John McCall via Phabricator via cfe-commits
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

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

2020-06-15 Thread John McCall via Phabricator via cfe-commits
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

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

2020-06-12 Thread John McCall via Phabricator via cfe-commits
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

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

2020-06-12 Thread John McCall via Phabricator via cfe-commits
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 > >

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

2020-06-11 Thread John McCall via Phabricator via cfe-commits
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] D81311: [RFC] LangRef: Define inmem parameter attribute

2020-06-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D81311#2088568 , @jdoerfert wrote: > In D81311#2088075 , @rjmccall wrote: > > > In D81311#2087592 , @jdoerfert > > wrote: > > > > > In

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

2020-06-11 Thread John McCall via Phabricator via cfe-commits
rjmccall added a comment. In D81311#2087592 , @jdoerfert wrote: > In D81311#2086326 , @rjmccall wrote: > > > In D81311#2086227 , @jdoerfert > > wrote: > > > > > Do we allow

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

2020-06-10 Thread John McCall via Phabricator via cfe-commits
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] D81420: Fix size for _ExtInt types with builtins

2020-06-10 Thread John McCall via Phabricator via cfe-commits
rjmccall added inline comments. Comment at: clang/test/Sema/builtins-overflow.c:39 +_ExtInt(129) result; +_Bool status = __builtin_mul_overflow(x, y, ); // expected-error {{signed _ExtInt of bit sizes greater than 128 not supported}} + } jtmott-intel

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

2020-06-10 Thread John McCall via Phabricator via cfe-commits
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

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

2020-06-10 Thread John McCall via Phabricator via cfe-commits
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 =

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

2020-06-09 Thread John McCall via Phabricator via cfe-commits
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

2020-06-09 Thread John McCall via Phabricator via cfe-commits
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.

2020-06-09 Thread John McCall via Phabricator via cfe-commits
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] D81311: [RFC] LangRef: Define inmem parameter attribute

2020-06-09 Thread John McCall via Phabricator via cfe-commits
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

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

2020-06-08 Thread John McCall via Phabricator via cfe-commits
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

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

2020-06-08 Thread John McCall via Phabricator via cfe-commits
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] D72781: [Matrix] Add __builtin_matrix_column_load to Clang.

2020-06-08 Thread John McCall via Phabricator via cfe-commits
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:

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

2020-06-08 Thread John McCall via Phabricator via cfe-commits
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"

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

2020-06-08 Thread John McCall via Phabricator via cfe-commits
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] D72778: [Matrix] Add __builtin_matrix_transpose to Clang.

2020-06-08 Thread John McCall via Phabricator via cfe-commits
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

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

2020-06-08 Thread John McCall via Phabricator via cfe-commits
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 + { +

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

2020-06-08 Thread John McCall via Phabricator via cfe-commits
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] D72778: [Matrix] Add __builtin_matrix_transpose to Clang.

2020-06-08 Thread John McCall via Phabricator via cfe-commits
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

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

2020-06-06 Thread John McCall via Phabricator via cfe-commits
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] D76794: [Matrix] Implement * binary operator for MatrixType.

2020-06-06 Thread John McCall via Phabricator via cfe-commits
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.

2020-06-06 Thread John McCall via Phabricator via cfe-commits
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

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

2020-06-06 Thread John McCall via Phabricator via cfe-commits
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:

  1   2   3   4   5   6   7   8   9   10   >