[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

majnemer added inline comments. Comment at: clang/lib/Headers/intrin.h:345-347 + _BitBase += (_BitPos / 32); + _BitPos %= 32; return (*_BitBase >> _BitPos) & 1; `_bittest` seems to expand to `(((unsigned char const *)_BitBase)[_BitPos >> 3] >> (_BitPos &

[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

majnemer accepted this revision. majnemer added a comment. LGTM https://reviews.llvm.org/D33616 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

majnemer added a comment. In https://reviews.llvm.org/D33616#768287, @hans wrote: > From looking in the Intel manual (Table 3-2, in 3.1.1.9 about > Bit(BitBase,BitOffset)) it does sound like the bit offset can be negative > *shudder*, so I suppose this is necessary and explains why the type is

[PATCH] D33616: [MS] Fix _bittest* intrinsics for values bigger than 31

majnemer added a comment. Do you really want to be doing signed division here? Because it is signed, it won't turn into the simple bitshifts and masks that one might expect. https://reviews.llvm.org/D33616 ___ cfe-commits mailing list

[PATCH] D32988: [libc++] Refactor Windows support headers.

majnemer added inline comments. Comment at: include/support/win32/msvc_builtin_support.h:33 + +_LIBCPP_ALWAYS_INLINE int __builtin_popcount(unsigned int x) +{ compnerd wrote: > I think I prefer the following implementation: > > _LIBCPP_ALWAYS_INLINE int

[PATCH] D32435: clang-cl: Add support for /permissive-

majnemer requested changes to this revision. majnemer added a comment. This revision now requires changes to proceed. I don't think this is correct. GDR (of Microsoft) says the behavior is different:

[PATCH] D30809: [coroutines] Add codegen for await and yield expressions

majnemer added inline comments. Comment at: lib/CodeGen/CGCoroutine.cpp:85 + unsigned No = 0; + StringRef AwaitKindStr = 0; + switch (Kind) { I'd just let the default constructor do its thing. https://reviews.llvm.org/D30809

[PATCH] D30809: [coroutines] Add codegen for await and yield expressions

majnemer added inline comments. Comment at: lib/CodeGen/CGCoroutine.cpp:85 + unsigned No = 0; + const char* AwaitKindStr = 0; + switch (Kind) { I'd use a StringRef here. https://reviews.llvm.org/D30809 ___

[PATCH] D30809: [coroutines] Add codegen for await and yield expressions

majnemer added inline comments. Comment at: lib/CodeGen/CGCoroutine.cpp:26 +enum class AwaitKind { Init, Normal, Yield, Final }; +char const *AwaitKindStr[] = {"init", "await", "yield", "final"}; +} I'd move this into buildSuspendSuffixStr.

[PATCH] D30806: [nonnull] Teach Clang to attach the nonnull LLVM attribute to declarations and calls instead of just definitions, and then teach it to *not* attach such attributes even if the source c

majnemer added inline comments. Comment at: lib/AST/ASTContext.cpp:8786 +if (OverrideNonnull && OverrideNonnullArgs) + *OverrideNonnullArgs |= 1 << ArgTypes.size(); + `1U` to avoid overflow UB? Comment at: lib/CodeGen/CGCall.cpp:2082

[PATCH] D30082: Fix assertion when generating debug information for deduced template specialization types.

majnemer added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:2479 +case Type::DeducedTemplateSpecialization: { + QualType DT = dyn_cast(T)->getDeducedType(); + assert(!DT.isNull() && "Undeduced types shouldn't reach here."); You are

[PATCH] D29930: Add `__is_direct_constructible` trait for checking safe reference initialization.

majnemer added inline comments. Comment at: include/clang/Basic/TokenKinds.def:418 TYPE_TRAIT_N(__is_nothrow_constructible, IsNothrowConstructible, KEYCXX) +TYPE_TRAIT_N(__is_direct_constructible, IsDirectConstructible, KEYCXX) Should this trait be grouped

[PATCH] D29912: [MS ABI] Correctly mangling vbase destructors

This revision was automatically updated to reflect the committed changes. Closed by commit rL295010: [MS ABI] Correctly mangling vbase destructors (authored by majnemer). Changed prior to commit: https://reviews.llvm.org/D29912?vs=88258=88280#toc Repository: rL LLVM

[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword

majnemer added inline comments. Comment at: lib/Parse/ParseDecl.cpp:2989 + + Diag(Loc, diag::err_ms_attributes_not_enabled); + continue; aaron.ballman wrote: > majnemer wrote: > > aaron.ballman wrote: > > > compnerd wrote: > > > > aaron.ballman

[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword

majnemer added inline comments. Comment at: lib/Parse/ParseDecl.cpp:2989 + + Diag(Loc, diag::err_ms_attributes_not_enabled); + continue; aaron.ballman wrote: > compnerd wrote: > > aaron.ballman wrote: > > > compnerd wrote: > > > > I think that

[PATCH] D29912: [MS ABI] Correctly mangling vbase destructors

majnemer created this revision. They are a little bit of a special case in the mangling. They are always mangled without taking into account their virtual-ness of the destructor. They are also mangled to return void, unlike the actual destructor. This fixes PR31931.

[PATCH] D28788: [AST] AttributedType should derive type properties from the EquivalentType

This revision was automatically updated to reflect the committed changes. Closed by commit rL292194: [AST] AttributedType should derive type properties from the EquivalentType (authored by majnemer). Changed prior to commit: https://reviews.llvm.org/D28788?vs=84609=84625#toc Repository: rL

[PATCH] D28788: [AST] AttributedType should derive type properties from the EquivalentType

majnemer created this revision. Using the canonical type instead of the equivalent type can result in insufficient template instantiations. This fixes PR31656. https://reviews.llvm.org/D28788 Files: include/clang/AST/Type.h test/CodeGenCXX/microsoft-abi-default-cc.cpp Index:

[PATCH] D28220: provide Win32 native threading

majnemer added inline comments. Comment at: include/__threading_support:29-30 +#include +#define WIN32_LEAN_AND_MEAN +#include +#include EricWF wrote: > I think we agreed that we cannot use this macro. Can we do as Reid suggests and not expose users to

[PATCH] D28383: build: add a heuristic to determine the C++ ABI

majnemer added a comment. Why isn't this equivalent to `_MSC_VER` ? Repository: rL LLVM https://reviews.llvm.org/D28383 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28226: threading_support: introduce __libcpp_recursive_mutex_t

majnemer added a comment. In https://reviews.llvm.org/D28226#634282, @compnerd wrote: > The dynamic behavior only is used for Windows, not pthreads. So, we dont see > it here, but it becomes apparent in the windows support. I was trying to > minimize the changes to libc++ itself to avoid

[PATCH] D28226: threading_support: introduce __libcpp_recursive_mutex_t

majnemer added a comment. Is `__libcpp_mutex_reference` all that useful? There is no real dynamism for these mutexes. I'd just add functions for the recursive locks just like we have a separate function for `__libcpp_recursive_mutex_init` Repository: rL LLVM https://reviews.llvm.org/D28226

[PATCH] D28220: provide Win32 native threading

majnemer added inline comments. Comment at: include/__threading_support:300-305 +int __libcpp_recursive_mutex_init(__libcpp_mutex_t *__m) +{ + InitializeSRWLock(__m); + return 0; +} + compnerd wrote: > majnemer wrote: > > I don't think you can use slim rw

[PATCH] D28220: provide Win32 native threading

majnemer added a comment. In https://reviews.llvm.org/D28220#633622, @compnerd wrote: > @majnemer Im using the Fls* APIs since they provide the thread termination > callback, which Tls* doesn't. Good point about the SRW. Those are newer, > but, we can always provide a fallback later. I

[PATCH] D28220: provide Win32 native threading

majnemer added a comment. slim reader-writer locks are faster than critical sections, I'd recommend your implementation switch to those. Also, why do you use Fls instead of Tls? Repository: rL LLVM https://reviews.llvm.org/D28220 ___

[PATCH] D28212: typeinfo: provide a partial implementation for Win32

majnemer added inline comments. Comment at: src/typeinfo.cpp:28-29 +#if defined(_WIN64) + static constexpr const size_t fnv_offset_basis = 14695981039346656037; + static constexpr const size_t fnv_prime = 10995116282110; +#else majnemer wrote: > Why make these

[PATCH] D27641: DebugInfo: Added support for Checksum debug info feature (Clang part)

majnemer added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:433-434 void CGDebugInfo::CreateCompileUnit() { + SmallString<32> Checksum; + llvm::DIFile::ChecksumKind CSKind = llvm::DIFile::CSK_None; Formatting looks wrong

[PATCH] D27163: Introduce -f[no-]strict-return flag that controls code generation for C++'s undefined behaviour return optimisation

majnemer added a comment. My 2 cents: If making this a target-specific default is not OK, why not turn on -fno-strict-return if -fapple-kext is specified? I believe that would cover the critical use case in question. Repository: rL LLVM https://reviews.llvm.org/D27163

[PATCH] D27680: [CodeGen] Move lifetime.start of a variable when goto jumps back past its declaration

majnemer added inline comments. Comment at: lib/CodeGen/CodeGenFunction.h:619 +bool hasLabel(const LabelDecl *LD) const { + return std::find(Labels.begin(), Labels.end(), LD) != Labels.end(); +} This can be written as `llvm::is_contained(Labels,

[PATCH] D27486: Correct class-template deprecation behavior

majnemer added inline comments. Comment at: lib/Sema/SemaTemplate.cpp:2355 Converted, nullptr); + if (auto *attr = ClassTemplate->getTemplatedDecl() + ->getAttr()) Please

[PATCH] D27529: Correct Vectorcall Register passing and HVA Behavior

majnemer added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:3871 +for (auto : FI.arguments()) { + if (Count < 6) +I.info = classify(I.type, FreeSSERegs, false, IsVectorCall, IsRegCall); erichkeane wrote: > majnemer wrote: > >

[PATCH] D27529: Correct Vectorcall Register passing and HVA Behavior

majnemer added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:1688 +for (auto : FI.arguments()) { + if(Count < 6) +I.info = reclassifyHvaArgType(I.type, State, I.info); erichkeane wrote: > majnemer wrote: > > Formatting. > I don't see

[PATCH] D27529: Correct Vectorcall Register passing and HVA Behavior

majnemer added inline comments. Comment at: lib/CodeGen/TargetInfo.cpp:1688 +for (auto : FI.arguments()) { + if(Count < 6) +I.info = reclassifyHvaArgType(I.type, State, I.info); Formatting. Comment at:

[PATCH] D26846: __uuidof() and declspec(uuid("...")) should be allowed on enumeration types

majnemer added a comment. This LGTM but Aaron should give the go ahead. Repository: rL LLVM https://reviews.llvm.org/D26846 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27226: [MS ABI] Implement more of the Itanium mangling rules

This revision was automatically updated to reflect the committed changes. Closed by commit rL288826: [MS ABI] Implement more of the Itanium mangling rules (authored by majnemer). Changed prior to commit: https://reviews.llvm.org/D27226?vs=80096=80437#toc Repository: rL LLVM

[PATCH] D27226: [MS ABI] Implement more of the Itanium mangling rules

majnemer updated this revision to Diff 80096. majnemer added a comment. - Simplify the mangling a little bit https://reviews.llvm.org/D27226 Files: lib/AST/MicrosoftMangle.cpp test/CodeGenCXX/mangle-ms-cxx11.cpp Index: test/CodeGenCXX/mangle-ms-cxx11.cpp

[PATCH] D27226: [MS ABI] Implement more of the Itanium mangling rules

majnemer updated this revision to Diff 79848. majnemer marked an inline comment as done. majnemer added a comment. - Address review comments https://reviews.llvm.org/D27226 Files: lib/AST/MicrosoftMangle.cpp test/CodeGenCXX/mangle-ms-cxx11.cpp Index: test/CodeGenCXX/mangle-ms-cxx11.cpp

[PATCH] D27226: [MS ABI] Implement more of the Itanium mangling rules

majnemer updated this revision to Diff 79837. majnemer added a comment. - Address review comments https://reviews.llvm.org/D27226 Files: lib/AST/MicrosoftMangle.cpp test/CodeGenCXX/mangle-ms-cxx11.cpp Index: test/CodeGenCXX/mangle-ms-cxx11.cpp

[PATCH] D27226: [MS ABI] Implement more of the Itanium mangling rules

majnemer created this revision. majnemer added a reviewer: rnk. majnemer added a subscriber: cfe-commits. We didn't implement one of the corner cases: a lambda which belongs to an initializer for a field. In this case, we need to mangle the field name into the lambda. This fixes PR31197.

<    1   2