[PATCH] D130446: [apinotes] Upstream changes to `APINotesYAMLCompiler.cpp`.

zoecarver added a comment. > Can you please add a round trip test as well? There is no testing infrastructure for this upstream. I have tests downstream. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130446/new/ https://reviews.llvm.org/D130446

[PATCH] D130446: [apinotes] Upstream changes to `APINotesYAMLCompiler.cpp`.

zoecarver created this revision. zoecarver added a reviewer: compnerd. Herald added a project: All. zoecarver requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D130446 Files:

[PATCH] D129384: [objcxx] Fix `std::addressof` for `id`.

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG22c7a6ec: [objcxx] Fix `std::addressof` for `id`. (authored by zoecarver). Repository: rG LLVM Github Monorepo

[PATCH] D129384: [objcxx] Fix `std::addressof` for `id`.

zoecarver created this revision. Herald added a project: All. zoecarver requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D129384 Files: clang/lib/Sema/SemaChecking.cpp

[PATCH] D120160: [Clang] Add `-funstable` flag to enable unstable and experimental features

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

[PATCH] D116203: [clang] adds unary type transformations as compiler built-ins

zoecarver added a comment. This patch looks awesome, Chris. Does it make sense to have builtins for `add_const`, etc.? Isn't `T const` already kind of a builtin for this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116203/new/

[PATCH] D113455: [clang][objc][codegen] Skip emitting ObjC category metadata when the category is empty

zoecarver accepted this revision. zoecarver added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/CodeGen/CGObjCMac.cpp:6683 + values.add(classMethodList); + isEmptyCategory &= + instanceMethodList->isNullValue() &&

[PATCH] D106394: [clang][pp] adds '#pragma include_instead'

zoecarver added a comment. Can't wait to start using this! (Note: this is not a full review, just some thoughts.) Do you have a test case where // private: header one *exists* // private: header two #include<__header_one.h> // public: header three #include<__header_two.h> I

[PATCH] D101598: [clang][Sema] adds `[[clang::no_address]]` attribute

zoecarver added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:5972 + Although opt-in, the attribute poisons the entire overload set, meaning that only one overload + needs the attribute per set (although such usage is discouraged). Until there is

[PATCH] D98904: Instantiate static constexpr data members on MS ABI.

zoecarver added a comment. > So. I think the status quo is OK but not great; we accept invalid code, in > the name of MSVC compatibility, that MSVC doesn't accept. I don't think > following MSVC would be a good thing, as that'd lead to our rejecting valid > code (that MSVC also rejects but

[PATCH] D98904: Instantiate static constexpr data members on MS ABI.

zoecarver added a comment. > (I think you're missing something here to trigger the instantiation of the > class, such as Invalid x;.) Yes, you're right. > The above example is valid in C++17 onwards, because in C++17 onwards, static > constexpr data members are implicitly inline, and the

[PATCH] D98904: Instantiate static constexpr data members on MS ABI.

zoecarver added a comment. So, as I mentioned in the description of this patch, the reason that these members are treated as definitions is that Clang implicitly marks them as inline when targeting MS. This is sort of interesting a) because it only applies to constexpr vars, and b) because

[PATCH] D98904: Instantiate static constexpr data members on MS ABI.

zoecarver created this revision. zoecarver added a reviewer: rsmith. zoecarver requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Because MSVC will not treat the definition of a static data member as part of the declaration, it will not get

[PATCH] D98104: Update __is_unsigned builtin to match the Standard.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGa89ac0dd185d: Update __is_unsigned builtin to match the Standard. (authored by zoecarver). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D98104: Update __is_unsigned builtin to match the Standard.

zoecarver updated this revision to Diff 329761. zoecarver added a comment. - Fix review comments: add colon, reflow line, and fix typo. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98104/new/ https://reviews.llvm.org/D98104 Files:

[PATCH] D98104: Update __is_unsigned builtin to match the Standard.

zoecarver added a comment. Sorry for the delay in updating. All review comments have been addressed. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98104/new/ https://reviews.llvm.org/D98104 ___

[PATCH] D98104: Update __is_unsigned builtin to match the Standard.

zoecarver updated this revision to Diff 328905. zoecarver added a comment. - Address Arthur's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98104/new/ https://reviews.llvm.org/D98104 Files: clang/docs/LanguageExtensions.rst

[PATCH] D98104: Update __is_unsigned builtin to match the Standard.

zoecarver added inline comments. Comment at: clang/lib/Sema/SemaExprCXX.cpp:4837 +// Enum types should always return false (same as UTT_IsSigned). +return !T->isEnumeralType() && T->isUnsignedIntegerType(); Quuxplusone wrote: > FWIW, I'd lose the

[PATCH] D98104: Update __is_unsigned builtin to match the Standard.

zoecarver added inline comments. Comment at: clang/docs/LanguageExtensions.rst:1199 + types. Note, before Clang 10, returned true for enumeration types if the + underlying type was signed, and returned false for floating-point types. * ``__is_standard_layout`` (C++, GNU,

[PATCH] D98104: Update __is_unsigned builtin to match the Standard.

zoecarver created this revision. zoecarver added reviewers: ldionne, rsmith, tmatheson. zoecarver requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Updates __is_unsigned to have the same behavior as the standard specifies. This is in line

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver updated this revision to Diff 324875. zoecarver added a comment. - * Format with clang-format. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92361/new/ https://reviews.llvm.org/D92361 Files: clang/include/clang/Basic/AttrDocs.td

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added a comment. Sorry for the delay in updating. @ahatanak let me know if you've found another way to break this patch :P Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92361/new/ https://reviews.llvm.org/D92361

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver updated this revision to Diff 324874. zoecarver added a comment. - Fix the case where an object inherits from two objects, one with a deleted copy constructor and the other with a deleted move constructor. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added a comment. > We could probably do something like what this patch is doing and determine > whether a class can be passed in registers based on whether its subobjects > can be passed in registers. If all of the subobjects can be passed in > registers, the current class can be

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added a comment. @ahatanak this patch should now be a nfc for types that don't have anything to do with the trivial-abi attribute. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92361/new/ https://reviews.llvm.org/D92361

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver updated this revision to Diff 312629. zoecarver added a comment. - Fix base-derived case. - Rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92361/new/ https://reviews.llvm.org/D92361 Files: clang/include/clang/Basic/AttrDocs.td

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added a comment. In D92361#2459655 , @rjmccall wrote: > In D92361#2459513 , @zoecarver wrote: > >>> I think that as long as the class leaves a copy/move constructor defaulted, >>> there's no need for a

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6502 + // except that it has a non-trivial member *with* the trivial_abi attribute. + for (auto Base : D->bases()) { +if (auto CxxRecord = Base.getType()->getAsCXXRecordDecl())

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6502 + // except that it has a non-trivial member *with* the trivial_abi attribute. + for (auto Base : D->bases()) { +if (auto CxxRecord = Base.getType()->getAsCXXRecordDecl())

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added a comment. > I think that as long as the class leaves a copy/move constructor defaulted, > there's no need for a new trivial_abi attribute. Sorry, I'm not sure I follow. Could you elaborate a bit or provide an example? What do you mean by "new" trivial_abi attribute?

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added a comment. Sorry for the slow update. This patch now implements the suggested change (to "inherit" the trivial-abi attribute if there are no user-defined special members and its copy/move constructor is deleted solely because of a subobject with the attribute). Let me know what

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver updated this revision to Diff 38. zoecarver added a comment. - Add lots of tests for S0. - Implement discussed change for handling S0. - Update docs to reflect change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92361/new/

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver added a comment. > If the implementation-specific builtins don't match on these, then maybe they > should have different names, is his argument I think. That's a fair point. And I agree, if they don't match, maybe it would be best to have different names. I'm hoping that we can all

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver added a comment. In D87974#2438682 , @BillyONeal wrote: >> Are they actually the same, with the same handling of corner cases like >> unions and tail padding? >> There's more to this than just the name, and if they aren't the same, it >> seems

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added a comment. > I think perhaps we are talking past each other and have reached the limits of > what this sub-thread can hope to achieve. I agree. I am sure that we (or at least you two) could talk about the semantics of "teleportation" and "destructively moving" objects for many

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver added a subscriber: BillyONeal. zoecarver added a comment. So, it looks like GCC already uses `__builtin_clear_padding` and MSVC already uses `__builtin_zero_non_value_bits`. This patch (obviously) is currently implementing `__builtin_zero_non_value_bits ` but, I had planned to update

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added a comment. @rjmccall Thanks for all that information. You're right; thinking about it in the context of four value operations is helpful. > Hmm. My first instinct is to say that a type that "adds new information" > about its special members — i.e. that explicitly declares its

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added a comment. > If it is not possible to copy or move the type at all, it is not possible to > copy or move it trivially. This was a bit confusing to me because I think this changed between C++14 and 17. In C++14, a class was trivially copyable if it had no non-trivial copy/move

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver updated this revision to Diff 308699. zoecarver added a comment. - Add comment in SemaDeclCXX. - Add tests for non-trivial bases/members. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92361/new/ https://reviews.llvm.org/D92361 Files:

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6416 + if (D->hasAttr()) +return true; + rjmccall wrote: > zoecarver wrote: > > rjmccall wrote: > > > I don't think we can just unconditionally check for the attribute. The > > >

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6416 + if (D->hasAttr()) +return true; + rjmccall wrote: > I don't think we can just unconditionally check for the attribute. The rule > for `trivial_abi` is that it overrides

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver updated this revision to Diff 308526. zoecarver added a comment. - Update wording in AttrDocs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92361/new/ https://reviews.llvm.org/D92361 Files: clang/include/clang/Basic/AttrDocs.td

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:2986 purpose of calls. -A class annotated with ``trivial_abi`` can have non-trivial destructors or -copy/move constructors without automatically becoming non-trivial for the -purposes of calls.

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver updated this revision to Diff 308523. zoecarver edited the summary of this revision. zoecarver added a comment. - Fix wording in AttrDocs - Add more tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92361/new/

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:2986 purpose of calls. -A class annotated with ``trivial_abi`` can have non-trivial destructors or -copy/move constructors without automatically becoming non-trivial for the -purposes of calls.

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:2986 purpose of calls. -A class annotated with ``trivial_abi`` can have non-trivial destructors or -copy/move constructors without automatically becoming non-trivial for the -purposes of calls.

[PATCH] D92361: [trivial-abi] Support types without a copy or move constructor.

zoecarver created this revision. Herald added a reviewer: aaron.ballman. Herald added a project: clang. Herald added a subscriber: cfe-commits. zoecarver requested review of this revision. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D92361 Files:

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver added a comment. > Filed https://gcc.gnu.org/PR97943 for that. Avoiding the crash is trivial, > deciding what we want to do exactly for flexible array members (which are > beyond the C++ standard) is harder. Yes, and we should try to do the same thing in this case. Currently, this

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver added a comment. @jwakely It looks like `UnsizedTail` causes a crash. Other than that all the tests in this PR pass. It also looks like there's (at least) some support for unions and bitfields. This patch doesn't support those but I'm planning to add support as a follow-up.

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver added a comment. > How should I check for support? Is it going to be e.g. > __has_feature(__builtin_clear_padding)? I'm not sure `__has_feature` will work but `__has_builtin` should work on both Clang and GCC. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver added a comment. > Let's make sure that we follow the same semantics that GCC does, particularly > w.r.t. union, bitfields, and padding at the end of an object (whether it's in > an array or not). Agreed. I'm planning to run some tests tomorrow once the nightly build has updated.

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver added a comment. In D87974#2408119 , @jwakely wrote: > As of a few hours ago, GCC has `__builtin_clear_padding`, see > https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fclear_005fpadding > for the docs. Great.

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver added a comment. @jfb (and others), friendly ping. Are there any other changes (especially to the logic) that you'd like me to make? Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1647 +QualType Ty) { + auto *I8Ptr =

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1704 +->getArrayElementTypeNoTypeQual() +->isRecordType()) { + auto FieldElement = CGF.Builder.CreateStructGEP(Ptr, Index); Is it OK to possibly create

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1652 +auto Element = CGF.Builder.CreateGEP(I8Ptr, Index); +CGF.Builder.CreateAlignedStore(Zero, Element, MaybeAlign()); + }; jfb wrote: > You should use `alignmentAtOffset`

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver updated this revision to Diff 295445. zoecarver added a comment. - Support constant arrays - Format changes with clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87974/new/ https://reviews.llvm.org/D87974 Files:

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver marked an inline comment as done. zoecarver added inline comments. Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp:46 +void test(Baz *baz) { + __builtin_zero_non_value_bits(baz); +} zoecarver wrote: > jfb wrote: > > It would

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver marked 3 inline comments as done. zoecarver added inline comments. Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits-codegen.cpp:16 + Bar f; +}; + jfb wrote: > It would be helpful to have a comment with the final layout of the struct, >

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver updated this revision to Diff 294603. zoecarver marked 3 inline comments as done. zoecarver added a comment. - Add UnsizedTail codegen test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87974/new/ https://reviews.llvm.org/D87974 Files:

[PATCH] D87974: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver updated this revision to Diff 293606. zoecarver edited the summary of this revision. zoecarver added a comment. - Add more test cases. - Fix typo. - Add codegen tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87974/new/

[PATCH] D87974: Summary: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver added inline comments. Comment at: clang/test/CodeGenCXX/builtin-zero-non-value-bits.cpp:160 + +int main() { + testAllForType<32, 16, char>(11, 22, 33, 44); jfb wrote: > Usually CodeGen tests will use lit to check the emitted IR matches >

[PATCH] D87974: Summary: [Builtin] Add __builtin_zero_non_value_bits.

zoecarver created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. zoecarver requested review of this revision. Adds `__builtin_zero_non_value_bits` to zero all padding bits of a struct. Currently does not support unions or bitfields. Repository: rG LLVM

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGe7c5da57a5f3: [CodeGen] Add public function to emit C++ destructor call. (authored by zoecarver). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82392/new/

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

zoecarver added a comment. @rjmccall I updated this patch to introduce the function `getCXXDestructorImplicitParam` instead. This should be closer to D79942 . It seems like there is never more than one implicit parameter so, I just have it return a single

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

zoecarver updated this revision to Diff 274616. zoecarver added a comment. - Remove `emitCXXDestructorCall` and add `getCXXDestructorImplicitParam`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82392/new/ https://reviews.llvm.org/D82392 Files:

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

zoecarver created this revision. zoecarver added reviewers: rjmccall, mboehme. Herald added a project: clang. Herald added a subscriber: cfe-commits. Adds `CodeGen::emitCXXDestructorCall`, a function that creates a CodeGenFunction using the arguments provided, then invokes

[PATCH] D67052: Add reference type transformation builtins

zoecarver updated this revision to Diff 258834. zoecarver marked 2 inline comments as done. zoecarver added a comment. - Rebase - Fix based on review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67052/new/ https://reviews.llvm.org/D67052

[PATCH] D67052: Add reference type transformation builtins

zoecarver updated this revision to Diff 258836. zoecarver added a comment. - Format using clang-format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67052/new/ https://reviews.llvm.org/D67052 Files: clang/include/clang/AST/Type.h

[PATCH] D67052: Add reference type transformation builtins

zoecarver marked 6 inline comments as done. zoecarver added a comment. @EricWF and @miscco thanks for the review comments. Sorry for the delay, I forgot about this patch. All your comments have been addressed/fixed. Comment at: clang/lib/AST/TypePrinter.cpp:1020 switch

[PATCH] D77519: Fix __is_pointer builtin type trait to work with Objective-C pointer types.

This revision was automatically updated to reflect the committed changes. Closed by commit rGb25ec45809fb: Fix __is_pointer builtin type trait to work with Objective-C pointer types. (authored by zoecarver). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D77519: Fix __is_pointer builtin type trait to work with Objective-C pointer types.

zoecarver marked 2 inline comments as done. zoecarver added inline comments. Comment at: clang/test/SemaObjCXX/type-traits-is-pointer.mm:1 +// RUN: %clang_cc1 -fsyntax-only -fobjc-arc -fobjc-runtime-has-weak -verify %s +// expected-no-diagnostics ldionne wrote:

[PATCH] D77519: Fix __is_pointer builtin type trait to work with Objective-C pointer types.

zoecarver updated this revision to Diff 255362. zoecarver added a comment. Diff from master not last commit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77519/new/ https://reviews.llvm.org/D77519 Files: clang/lib/Sema/SemaExprCXX.cpp

[PATCH] D77519: Fix __is_pointer builtin type trait to work with Objective-C pointer types.

zoecarver updated this revision to Diff 255361. zoecarver added a comment. Fix based on review: - Use static_assert directly in the test - Update run command to work on non-objc platforms (e.g. linux) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D77519: Fix __is_pointer builtin type trait to work with Objective-C pointer types.

zoecarver marked an inline comment as done. zoecarver added a comment. @ldionne of course! Comment at: libcxx/include/type_traits:901 +// In clang 10.0.0 and earlier __is_pointer didn't work with Objective-C types. +#if __has_keyword(__is_pointer) && _LIBCPP_CLANG_VER > 1000 +

[PATCH] D77519: Fix __is_pointer builtin type trait to work with Objective-C pointer types.

zoecarver created this revision. Herald added projects: clang, libc++. Herald added subscribers: libcxx-commits, cfe-commits. Herald added a reviewer: libc++. zoecarver added reviewers: ldionne, rsmith, EricWF. Herald added a subscriber: dexonsmith. 5ade17e

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

zoecarver updated this revision to Diff 252742. zoecarver added a comment. - Diff from master Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76525/new/ https://reviews.llvm.org/D76525 Files: clang/include/clang/Basic/Builtins.def

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

zoecarver updated this revision to Diff 252741. zoecarver added a comment. - Fix based on review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76525/new/ https://reviews.llvm.org/D76525 Files: clang/include/clang/Basic/Builtins.def

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

zoecarver added a comment. @craig.topper I'm not sure what should happen. It should probably just use that CPU (although that's not a great solution). Is there a way to detect if an attribute was used to change the target in which case we could error? What do you think should happen?

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

zoecarver added a comment. @jyknight yes, the current plan is to use it in libc++'s implementation. I can put that implementation (which uses this builtin) up for review before this lands. That way we can discuss the implementation before adding a (possibly unneeded) builtin. Sound good?

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rGb915aec6b591: Add method to TargetInfo to get CPU cache line size (authored by zoecarver). Repository: rG LLVM Github

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver added a comment. @lebedev.ri my misunderstanding. I'll commit :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74918/new/ https://reviews.llvm.org/D74918 ___ cfe-commits mailing list

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver added a comment. I don't know enough about clang and llvm to know what is both possible and preferable. @craig.topper @lebedev.ri what is the best course of action here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74918/new/

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

zoecarver added a comment. @craig.topper thanks for the comments! I'm going to hold off on updating this until we figure out what's happening with D74918 , though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver added a comment. @craig.topper that would mean that it couldn't be constexpr though, right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74918/new/ https://reviews.llvm.org/D74918 ___

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver updated this revision to Diff 252079. zoecarver added a comment. - Rebase off master - For i386, return 16 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74918/new/ https://reviews.llvm.org/D74918 Files:

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver added a comment. @lebedev.ri Where should I put this? Could you maybe point me to another LLVM target API that clang uses that I could base this off? Maybe it should go inside the triple or just be a static function? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver marked an inline comment as done. zoecarver added inline comments. Comment at: clang/lib/Basic/Targets/X86.cpp:1786 +// i386 +case CK_i386: +// Netburst craig.topper wrote: > zoecarver wrote: > > craig.topper wrote: > > > I found the

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver updated this revision to Diff 251758. zoecarver added a comment. Fix based on review: - update comments - return 64 for Core2 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74918/new/ https://reviews.llvm.org/D74918 Files:

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver marked 2 inline comments as done. zoecarver added a comment. I've made the suggested changes. Is there a consensus that this should be moved to LLVM? I think it's fairly clang-specific (given the use case). We can also always move it to LLVM if the need arises in the future.

[PATCH] D76525: Expose cache line size in __builtin_get_cpu_cache_line_size

zoecarver created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. zoecarver added reviewers: jfb, EricWF, efriedma, jyknight, echristo, __simt__. Herald added a subscriber: dexonsmith. This patch creates the __builtin_get_cpu_cache_line_size to wrap the cpu

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver added a comment. In D76525 I've added a builtin that uses this API. We can use that builtin in libc++. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74918/new/ https://reviews.llvm.org/D74918

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver marked 3 inline comments as done. zoecarver added a comment. @lebedev.ri LLVM may be better, I'm not sure. If you feel strongly I can move it. @jyknight I'm planning on adding a builtin that uses this method. I can put the patch up for that first if you would like.

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver added a comment. @kristof.beyls I'll try to bring that up in my libc++ patch, thanks. I'm planning on committing this today unless anyone still has concerns. @jyknight is this path OK with you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver added a comment. Herald added a subscriber: danielkiss. There are a lot of different ways we could implement the feature. We may want to only enable it when `-march=native`, or maybe only in the unstable ABI, and maybe we want to support aligned pairs on some architectures. I think

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver added a comment. Here's what I think we should do: continue to have this method just return the cache line size. Then have another method that returns `true` or `false` for whether the given architecture supports aligned pairs of cache lines then, users of this (either in clang or

[PATCH] D74918: Add method to TargetInfo to get CPU cache line size

zoecarver added a comment. Friendly ping. Any other comments on this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74918/new/ https://reviews.llvm.org/D74918 ___ cfe-commits mailing list

[PATCH] D66178: Remove std::shared_ptr::make_shared and std::shared_ptr::allocate_shared

This revision was automatically updated to reflect the committed changes. Closed by commit rG28d38a25e963: Remove std::shared_ptr::allocate_shared (authored by zoecarver). Changed prior to commit: https://reviews.llvm.org/D66178?vs=245918=246599#toc Repository: rG LLVM Github Monorepo

[PATCH] D66839: Fix stack address builtin for negative numbers

This revision was automatically updated to reflect the committed changes. Closed by commit rG6201f6601dec: Check args passed to __builtin_frame_address and __builtin_return_address. (authored by zoecarver). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D66839: Fix stack address builtin for negative numbers

zoecarver added a comment. I was using `SemaBuiltinConstantArgRange` incorrectly. I was returning an error for `!SemaBuiltinConstantArgRange` instead of `SemaBuiltinConstantArgRange`. Also, I only modified the fail tests so I wasn't able to catch the error. Updated and am running both the Sema

[PATCH] D66839: Fix stack address builtin for negative numbers

zoecarver updated this revision to Diff 246488. zoecarver added a comment. - diff from master (arg phab...) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66839/new/ https://reviews.llvm.org/D66839 Files: clang/lib/Sema/SemaChecking.cpp

  1   2   >