[PATCH] D93668: [clang] Add -ffuchsia-c++-abi flag to explicitly use the Fuchsia C++ ABI

rnk added a comment. I guess a triple of -fuchsia-itanium would be a reasonable way of expressing this. Why would we want a feature flag for the wasm C++ ABI? Is there a use case for using the webassembly C++ ABI on non-wasm ISAs? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D93668: [clang] Add -ffuchsia-c++-abi flag to explicitly use the Fuchsia C++ ABI

rnk added a comment. Generally makes sense, but I had a concern. Comment at: clang/include/clang/Basic/LangOptions.h:333 + /// the -ffuchsia-c++-abi flag. + bool UseFuchsiaCXXABI; + Why isn't this part of the LangOptions.def xmacro list? Repository: rG

[PATCH] D83892: [clang][cli] Port CodeGen option flags to new option parsing system

rnk added a comment. Reposting my comment here, since this is where the discussion was: I think this change broke -gline-tables-only functionality. I compared object files before and after this change. The new object file had a large .debug_loc section, which is only present when full debug

[PATCH] D93772: [Clang][Driver] Fix read-after-free when using /clang:

rnk accepted this revision. rnk 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/D93772/new/ https://reviews.llvm.org/D93772 ___

[PATCH] D92751: [clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate

rnk added a comment. Thank for the update, apologies for not providing these suggestions the first time. Comment at: clang/lib/CodeGen/CGCXXABI.cpp:320-321 + // No virtual functions + // Additionally, we need to ensure that there is a trivial copy assignment + //

[PATCH] D93458: clang-cl: Remove /Zd flag

rnk accepted this revision. rnk added a comment. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93458/new/ https://reviews.llvm.org/D93458 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D91659: Allow enum typedefs to be referenced with the 'enum' specifier under MSVC compat mode

rnk added a comment. In D91659#2458639 , @rsmith wrote: > @rnk Your thoughts on this would be appreciated. I think it's important to make MIDL happy if we can. As to how to restructure the code to get there, I took a look, but wasn't able to come up

[PATCH] D93350: [Test] Fix undef var in catch-undef-behavior.c

rnk accepted this revision. rnk 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/D93350/new/ https://reviews.llvm.org/D93350 ___

[PATCH] D92751: [clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate

rnk added a comment. Thanks for the fix. At first, misunderstood, I expected that an aggregate containing non-aggregates should be returned indirectly, and that the fix would be in the C++ ABI codepath. However, I see that is not the case. An aggregate may contain non-aggregates, and MSVC will

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

rnk added a comment. I think the biggest concern here is what to do about `/Fa` / `-save-temps`. If we do nothing, the S_OBJNAME record will change if you run the same compilation twice with and without those flags. Generally, we strive to ensure that the directly emitted object file is

[PATCH] D92800: [Clang] Make nomerge attribute a function attribute as well as a statement attribute.

rnk added inline comments. Comment at: clang/test/CodeGen/attr-nomerge.cpp:8 + [[clang::nomerge]] void f(); + [[clang::nomerge]] virtual void g(); + [[clang::nomerge]] static void f1(); zequanwu wrote: > rnk wrote: > > Hm, virtual functions, there's something

[PATCH] D92800: [Clang] Make nomerge attribute a function attribute as well as a statement attribute.

rnk added a comment. Nice, that wasn't too difficult. I had some suggestions for improving the test case, and I'd like to hear from Aaron. Comment at: clang/test/CodeGen/attr-nomerge.cpp:8 + [[clang::nomerge]] void f(); + [[clang::nomerge]] virtual void g(); +

[PATCH] D92944: Don't setup inalloca for swiftcc on i686-windows-msvc

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGdf282215d497: Dont setup inalloca for swiftcc on i686-windows-msvc (authored by rnk). Changed prior to commit:

[PATCH] D92883: De-templatify EmitCallArgs argument type checking, NFCI

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd7098ff29c58: De-templatify EmitCallArgs argument type checking, NFCI (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D91673: [PGO] Enable preinline and cleanup when optimize for size

rnk added a comment. I see two pre-commit test failures on the patch. Can you take a look at those as well? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91673/new/ https://reviews.llvm.org/D91673 ___

[PATCH] D92883: De-templatify EmitCallArgs argument type checking, NFCI

rnk updated this revision to Diff 310569. rnk added a comment. - use assign instead of push_back Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92883/new/ https://reviews.llvm.org/D92883 Files: clang/lib/CodeGen/CGCall.cpp

[PATCH] D92944: Don't setup inalloca for swiftcc on i686-windows-msvc

rnk created this revision. rnk added reviewers: compnerd, rjmccall. rnk requested review of this revision. Herald added a project: clang. Swiftcall does it's own target-independent argument type classification, since it is not designed to be ABI compatible with anything local on the target that

[PATCH] D92883: De-templatify EmitCallArgs argument type checking, NFCI

rnk created this revision. rnk added reviewers: compnerd, aeubanks, rsmith. rnk requested review of this revision. Herald added a project: clang. This template exists to abstract over FunctionPrototype and ObjCMethodDecl, which have similar APIs for storing parameter types. In place of a

[PATCH] D92606: Clean up debug locations for logical-and/or expressions

rnk added inline comments. Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4293 + { +// There is no need to emit line number for unconditional branch. +auto NL = ApplyDebugLocation::CreateEmpty(CGF); I see this is consistent with LAnd handling above, but

[PATCH] D92570: [clang] [Headers] Use the corresponding _aligned_free or __mingw_aligned_free in _mm_free

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/lib/Headers/mm_malloc.h:42 void *__mallocedMemory; #if defined(__MINGW32__) __mallocedMemory = __mingw_aligned_malloc(__size, __align);

[PATCH] D92512: ADT: Change AlignedCharArrayUnion to an alias of std::aligned_union_t, NFC

rnk added a comment. Sorry, racing updates. I agree, landing them together is better, since there is risk that some corner case platform won't have this support. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92512/new/ https://reviews.llvm.org/D92512

[PATCH] D92512: ADT: Change AlignedCharArrayUnion to an alias of std::aligned_union_t, NFC

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Looks good to me. Comment at: llvm/include/llvm/Support/ErrorOr.h:256-257 union { AlignedCharArrayUnion TStorage; AlignedCharArrayUnion ErrorStorage; };

[PATCH] D91673: [PGO] Enable preinline and cleanup when optimize for size

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Thanks, my concerns are addressed, and I believe @xur's are as well. Please wait for his input before landing, though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D92072: [CMake][NewPM] Move ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER into llvm/

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

[PATCH] D92072: [CMake][NewPM] Move ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER into llvm/

rnk added inline comments. Comment at: llvm/include/llvm/Config/llvm-config.h.cmake:95 +/* Define to 1 to enable the experimental new pass manager by default */ +#cmakedefine01 ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER + aeubanks wrote: > rnk wrote: > > This should

[PATCH] D92072: [CMake][NewPM] Move ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER into llvm/

rnk added inline comments. Comment at: llvm/include/llvm/Config/llvm-config.h.cmake:92 /* Define to 1 if you have the header file. */ #cmakedefine HAVE_SYSEXITS_H ${HAVE_SYSEXITS_H} This is unrelated, but appears to be in the wrong header, it should be in

[PATCH] D92109: [ASTImporter] Support import of CXXDeductionGuideDecl

rnk added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:3336 +if (Fun->getExplicitSpecifier().getExpr()) { + ExplicitExpr = importChecked(Err, Fun->getExplicitSpecifier().getExpr()); + if (Err) GCC 5.3 complained about this, so I

[PATCH] D91747: [Clang] Add __STDCPP_THREADS__ to standard predefine macros

rnk added a comment. In D91747#2423987 , @zequanwu wrote: > So, we could remove the checking for if `__STDCPP_THREADS__` and > `_LIBCPP_HAS_NO_THREADS` are both set. And let libcxx adds flag > `-mthread-model single` to use single thread (but this is

[PATCH] D92245: -fstack-clash-protection: Return an actual error when used on unsupported OS

rnk added a comment. Windows has effectively always had stack clash protection: we've always emitted those little chkstk probe calls for stack frames larger than a page. Would it make more sense to ignore this flag on Windows, since it opts into always-on behavior? If so, this doesn't seem

[PATCH] D91747: [Clang] Add __STDCPP_THREADS__ to standard predefine macros

rnk added a comment. If we believe the standard says that the compiler is supposed to set `__STDCPP_THREADS__`, then I think the libc++ #error needs to be adjusted. libcxxabi, or any other client, should be able to define `_LIBCPP_HAS_NO_THREADS`, and it should work, even if the compiler

[PATCH] D91840: OpaquePtr: Require byval on x86_intrcc parameter 0

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Thanks for removing that special case. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91840/new/ https://reviews.llvm.org/D91840 ___

[PATCH] D92062: [MS] Add more 128bit cmpxchg intrinsics for AArch64

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1e843a987d84: [MS] Add more 128bit cmpxchg intrinsics for AArch64 (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D92062: [MS] Add more 128bit cmpxchg intrinsics for AArch64

rnk marked an inline comment as done. rnk added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:336 + + // For Release ordering, the failure ordering should be Monotonic. + auto FailureOrdering = SuccessOrdering == AtomicOrdering::Release thakis

[PATCH] D92062: [MS] Add more 128bit cmpxchg intrinsics for AArch64

rnk updated this revision to Diff 307678. rnk marked an inline comment as done. rnk added a comment. - add tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92062/new/ https://reviews.llvm.org/D92062 Files:

[PATCH] D92061: [MS] Fix double evaluation of MSVC builtin arguments

This revision was automatically updated to reflect the committed changes. Closed by commit rG3bd067272671: [MS] Fix double evaluation of MSVC builtin arguments (authored by rnk). Changed prior to commit: https://reviews.llvm.org/D92061?vs=307468=307674#toc Repository: rG LLVM Github

[PATCH] D92061: [MS] Fix double evaluation of MSVC builtin arguments

rnk marked an inline comment as done. rnk added a comment. Thanks! Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1019 + default: +break; + case ARM::BI_BitScanForward: thakis wrote: > Maybe `return None` here and LLVM_UNREACHABLE at the bottom? Sure, why

[PATCH] D92062: [MS] Add more 128bit cmpxchg intrinsics for AArch64

rnk created this revision. rnk added reviewers: hans, thakis, rsmith, akhuang. Herald added subscribers: danielkiss, jfb, kristof.beyls. Herald added a project: clang. rnk requested review of this revision. The MSVC STL for requires this on ARM64. Requested in https://llvm.org/pr47099 Depends on

[PATCH] D92061: [MS] Fix double evaluation of MSVC builtin arguments

rnk created this revision. rnk added reviewers: hans, thakis, rsmith, akhuang. Herald added a subscriber: jfb. Herald added a project: clang. rnk requested review of this revision. This code got quite twisted because we consider some MSVC builtins to be target agnostic, and some to be target

[PATCH] D50979: Eliminate instances of `EmitScalarExpr(E->getArg(n))` in EmitX86BuiltinExpr().

rnk added inline comments. Herald added a subscriber: pengfei. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:10471 case X86::BI_InterlockedIncrement64: return EmitMSVCBuiltinExpr(MSVCIntrin::_InterlockedIncrement, E); case X86::BI_InterlockedCompareExchange128: {

[PATCH] D91747: [Clang] Add __STDCPP_THREADS__ to standard predefine macros

rnk added a comment. Does rG64802d48d51d651bd2e4567b2f228f8795569542 fix it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91747/new/ https://reviews.llvm.org/D91747 ___ cfe-commits mailing list

[PATCH] D91747: [Clang] Add __STDCPP_THREADS__ to standard predefine macros

rnk added a comment. I'll push a fix to add it to the cmake deps list. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91747/new/ https://reviews.llvm.org/D91747 ___ cfe-commits mailing list

[PATCH] D91747: [Clang] Add __STDCPP_THREADS__ to standard predefine macros

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

[PATCH] D91747: [Clang] Add __STDCPP_THREADS__ to standard predefine macros

rnk added inline comments. Comment at: clang/test/CXX/cpp/cpp.predefined/p2.cpp:1 +// RUN: %clang_cc1 %s -verify +// expected-no-diagnostics Let's expand on this: - test that we don't set the macro when compiling C (`-x c`) - test that we don't set the macro

[PATCH] D91747: [Clang] Add __STDCPP_THREADS__ to standard predefine macros

rnk added inline comments. Comment at: clang/include/clang/Frontend/FrontendOptions.h:307 + /// Wheather using -mthread-model single. + unsigned IsSingleThreadModel : 1; + This doesn't seem to fit with the other frontend options. This seems more like a

[PATCH] D91673: [PGO] Enable preinline and cleanup when optimize for size

rnk added a comment. Zequan, to build clang with PGO, you can follow the steps in Chrome's script to build clang with PGO: https://source.chromium.org/chromium/chromium/src/+/master:tools/clang/scripts/build.py;l=703?q=clang%2Fscripts%2F%20build.py=chromium Regarding -Oz / minsize / SizeLevel

[PATCH] D90194: [Driver] split LangOptions::SSPOff into SSPOFF and SSPUnspecified

rnk added a comment. Given that only three tests fail when the `nossp` attribute gets added from `-cc1` with no stack protector, I think it's reasonable to add it and skip adding the extra enum. I think it would be weird if we LTO'd together three kinds of objects (sp on, sp off, sp

[PATCH] D90194: [Driver] split LangOptions::SSPOff into SSPOFF and SSPUnspecified

rnk added a comment. Why does `-cc1` need to distinguish between enabled, disabled, unset? The design philosophy is that the driver figures out all the target-specific configuration stuff, and then tells cc1 which features to enable. See, for example, -fexceptions, which is off by default in

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

rnk added a comment. I had another thought, which is that even if it is UB, perhaps we really shouldn't be using UB as the basis for debug info emission. All programs have bugs, and most bugs invoke some form of UB. If we don't provide sufficient info when UB is involved, it can become harder

[PATCH] D90630: [CodeGen] Fix Bug 47499: __unaligned extension inconsistent behaviour with C and C++

This revision was automatically updated to reflect the committed changes. Closed by commit rGd2e7dca5ca92: [CodeGen] Fix Bug 47499: __unaligned extension inconsistent behaviour with C… (authored by j0le, committed by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D90714: [clang]Fix length threshold for MicrosoftMangle md5 hash

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm I think the test case looks good as is, FWIW. Token pasting might make it more readable, but it's improving the existing test, and not necessarily in scope for this patch. Repository: rG

[PATCH] D90849: [dllexport] Avoid multiple codegen assert for explicitly defaulted methods in explicit instantiation definitions (PR47683)

rnk added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:5887-5915 if (MD->isUserProvided()) { // Instantiate non-default class member functions ... // .. except for certain kinds of template specializations. if (TSK ==

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

rnk added a comment. In D90719#2372656 , @dblaikie wrote: > My understanding is that such code is UB, is that right? I guess I'm not convinced it's UB, and need some language lawyering help to decide. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D90719: [DebugInfo] Modify ctor homing as workaround for unconstructed libcxx types

rnk added a subscriber: EricWF. rnk added a comment. In D90719#2372463 , @dblaikie wrote: > Does Chromium need this fixed in clang? Or if it were fixed in libc++ would > that be adequate? (does Chromium's build need to work with old libc++s, or > does

[PATCH] D90571: [compiler-rt] [ubsan] Use the itanium type info lookup for mingw targets

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

[PATCH] D90571: [compiler-rt] [ubsan] Use the itanium type info lookup for mingw targets

rnk added inline comments. Comment at: compiler-rt/lib/ubsan/ubsan_type_hash_itanium.cpp:15 #include "ubsan_platform.h" -#if CAN_SANITIZE_UB && !SANITIZER_WINDOWS +#if CAN_SANITIZE_UB && (!SANITIZER_WINDOWS || defined(__MINGW32__)) #include "ubsan_type_hash.h"

[PATCH] D90572: [clang] [MinGW] Allow using the vptr sanitizer

rnk accepted this revision. rnk 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/D90572/new/ https://reviews.llvm.org/D90572 ___

[PATCH] D90630: [CodeGen] Fix Bug 47499: __unaligned extension inconsistent behaviour with C and C++

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Thanks, this basically looks correct to me, aside from some formatting details. Do you want me to apply those fixes and land this for you? I see that was done for your last patch, but it's best to

[PATCH] D89761: Split out llvm/Support/FileSystem/UniqueID.h and clang/Basic/FileEntry.h, NFC

rnk added a comment. I just saw this in passing. Thanks a lot, I had noticed that Filesystem.h is super expensive. Comment at: llvm/include/llvm/Support/FileSystem/UniqueID.h:36 + bool operator<(const UniqueID ) const { +return std::tie(Device, File) <

[PATCH] D89998: [c++20] For P0732R2 / P1907R1: Basic code generation and name mangling supportfor non-type template parameters of class type and template parameter objects.

rnk added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:2490 +ThisAdjustment += getASTRecordLayout(Derived).getBaseClassOffset(Base); +RD = Path[I]; + } rjmccall wrote: > What should we do on targets that allow virtual bases in member

[PATCH] D89072: [CodeView] Emit static data members as S_CONSTANTs.

rnk accepted this revision. rnk added a comment. lgtm Please fix this case in a follow-up, though: static const signed int gv_signed = 0x; const signed int useit2() { return gv_signed; } I think your `APSInt` code only works on static data member constants. Repository: rG LLVM

[PATCH] D89072: [CodeView] Emit static data members as S_CONSTANTs.

rnk added a comment. It seems like there are related test failures according to the Phabricator/Jenkins automation. Is that a true positive? Please confirm either way before landing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89072/new/

[PATCH] D89072: [CodeView] Emit static data members as S_CONSTANTs.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Looks good with a test for explicit zero initialization. Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:3157 + +if (Value.isNullValue()) + continue;

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

rnk added a comment. Herald added a subscriber: dexonsmith. Thanks, I like the new direction, hopefully others agree. Sorry for providing slow feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85802/new/ https://reviews.llvm.org/D85802

[PATCH] D89974: [driver][CUDA] Use CMake's FindCUDA as default --cuda-path.

rnk added a comment. My opinion carries less weight since I don't use CUDA, but I agree with everything Art said. Here's some input, if it helps. I like the `PATH` search for `ptxas` as a way to make things work out of the box as often as possible. I don't like the idea of CMake

[PATCH] D89799: [clang][driver] Rename DriverOption as NoXarchOption (NFC)

rnk added a comment. This seems like pretty corner case functionality. Do we really need this diagnostic? @tra @yaxun Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89799/new/ https://reviews.llvm.org/D89799

[PATCH] D89702: [clang] [msvc] Automatically link against oldnames just as linking against libcmt

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm We need to invent spellings for /MD /MT for the GCC-style driver if we want it to be usable in the MSVC environment. I don't think there's a precise translation to the `-static-*` family of

[PATCH] D70378: [LLD][COFF] Cover usage of LLD as a library

rnk added inline comments. Comment at: lld/Common/ErrorHandler.cpp:78 } - _exit(val); + llvm::sys::Process::Exit(val); } This appears to have regressed shutdown. `sys::Process::Exit` calls `exit`, not `_exit`, so now atexit destructors are run. That's

[PATCH] D89362: [MS] Apply `inreg` to AArch64 sret parms on instance methods

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG5fbab4025eb5: [MS] Apply `inreg` to AArch64 sret parms on instance methods (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D89362: [MS] Apply `inreg` to AArch64 sret parms on instance methods

rnk updated this revision to Diff 298473. rnk added a comment. - simplify some more Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89362/new/ https://reviews.llvm.org/D89362 Files: clang/lib/CodeGen/MicrosoftCXXABI.cpp

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

rnk added a comment. Would "f[no-]fuchsia-c++-abi-extensions" (or shorter, -ffuchsia-c++-abi) do the trick? I know it doesn't map well onto our current internal option representation, but I don't think the internal representation is particularly good. I'd rather limit the user-visible driver

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

rnk added a comment. In D85802#2331403 , @leonardchan wrote: > Perhaps we can add some mechanism in Clang that determines which ABIs are > supported for specific platforms? That is, when targetting Linux, Clang will > diagnose an error if using

[PATCH] D85802: [clang] Add -fc++-abi= flag for specifying which C++ ABI to use

rnk added a comment. I'd like to point out that we used to have a very similar flag, but we removed it back in 2014: http://reviews.llvm.org/D2545 Are you sure you need all the flexibility that this flag allows? For example, this will let users ask for the MSVC C++ ABI on Linux. I really don't

[PATCH] D89362: [MS] Apply `inreg` to AArch64 sret parms on instance methods

rnk added inline comments. Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:1108 + bool isTrivialForABI = + RD->canPassInRegisters() && !(isAArch64 && !isCXX14Aggregate(RD)); bool isIndirectReturn = efriedma wrote: > isTrivialForABI is only used if

[PATCH] D89362: [MS] Apply `inreg` to AArch64 sret parms on instance methods

rnk created this revision. rnk added reviewers: mgrang, efriedma. Herald added subscribers: danielkiss, kristof.beyls. Herald added a project: clang. rnk requested review of this revision. The documentation rules indicate that instance methods should return large, trivially copyable aggregates

[PATCH] D89072: [CodeView] Emit static data members as S_CONSTANTs.

rnk added inline comments. Comment at: clang/test/CodeGenCXX/debug-info-static-member.cpp:144-147 +// For some reason, const_va is not emitted when the target is MS. +// NOT-MS: !DIDerivedType(tag: DW_TAG_member, name: "const_va", +// NOT-MS-SAME: line: [[@LINE-3]] +//

[PATCH] D88680: Add ability to turn off -fpch-instantiate-templates in clang-cl

rnk added a subscriber: zequanwu. rnk added a comment. I think the flag was originally intended to be an internal -cc1 flag not exposed to users. You should be able to work around your problem with `-Xclang -fno-pch-instantiate-templates`, btw. @zequanwu, can you patch this in locally

[PATCH] D77598: Integral template argument suffix and cast printing

rnk added inline comments. Comment at: clang/lib/AST/TemplateBase.cpp:71-72 if (T->isBooleanType() && !Policy.MSVCFormatting) { Out << (Val.getBoolValue() ? "true" : "false"); } else if (T->isCharType()) { rsmith wrote: > reikdas wrote: > > rsmith

[PATCH] D88150: BuildVectorType with a dependent (array) type is crashing the compiler - Fix for PR-47542

This revision was automatically updated to reflect the committed changes. Closed by commit rGefd04721c9a2: BuildVectorType with a dependent (array) type is crashing the compiler - Fix… (authored by zahiraam, committed by rnk). Herald added a project: clang. Herald added a subscriber:

[PATCH] D87888: [X86] Use inlineasm flag output for the _bittest* intrinsics.

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

[PATCH] D74455: [MS] Pass aligned, non-trivially copyable things indirectly on x86

rnk abandoned this revision. rnk added a comment. I actually forgot about this change entirely and uploaded and landed a new one: https://reviews.llvm.org/D87923 I suppose they are functionally different: if we have an overaligned type that can be passed in registers due to [[trivial_abi]], we

[PATCH] D84362: [NFC] Refactor DiagnosticBuilder and PartialDiagnostic

rnk added subscribers: rjmccall, rnk. rnk added inline comments. Comment at: clang/include/clang/Basic/Diagnostic.h:1086-1090 /// Note that many of these will be created as temporary objects (many call /// sites), so we want them to be small and we never want their address

[PATCH] D87187: [Driver] Perform Linux distribution detection just once

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. `llvm::call_once` is fine, but IMO the static local version is preferable: it's built in to the language, no headers to include. Comment at: clang/lib/Driver/Distro.cpp:224 + if

[PATCH] D70378: [LLD][COFF] Cover usage of LLD as a library

rnk accepted this revision. rnk added a comment. Looks good to me, I didn't review very in depth, but I see the test case that we need. :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70378/new/ https://reviews.llvm.org/D70378

[PATCH] D88121: [X86] Add a memory clobber to the bittest intrinsic inline asm. Get default clobbers from the target

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

[PATCH] D87187: [Driver] Perform Linux distribution detection just once

rnk added a comment. I think we can go forward with the reviewers we have. I have one more concern. Are the other reviewers happy? Comment at: clang/lib/Driver/Distro.cpp:206 +const llvm::Triple ) { + static Distro::DistroType Type =

[PATCH] D88005: [clang] [MinGW] Add an implicit .exe suffix even when crosscompiling

rnk accepted this revision. rnk 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/D88005/new/ https://reviews.llvm.org/D88005 ___

[PATCH] D87888: [X86] Use inlineasm flag output for the _bittest* intrinsics.

rnk added a comment. Honestly, I forget exactly what the memory clobber does beyond the "sideeffect" marker. I would expect LLVM to model these just as external function calls that could read or write memory that is passed to them. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D87923: [MS] On x86_32, pass overaligned, non-copyable arguments indirectly

This revision was automatically updated to reflect the committed changes. Closed by commit rG3b3a16548568: [MS] On x86_32, pass overaligned, non-copyable arguments indirectly (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D88002: [clang-cl] Always interpret the LIB env var as separated with semicolons

rnk accepted this revision. rnk 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/D88002/new/ https://reviews.llvm.org/D88002 ___

[PATCH] D87923: [MS] On x86_32, pass overaligned, non-copyable arguments indirectly

rnk created this revision. rnk added reviewers: aeubanks, craig.topper. Herald added a project: clang. rnk requested review of this revision. This updates the C++ ABI argument classification code to use the logic from D72114 , fixing an ABI incompatibility with

[PATCH] D87808: [DebugInfo] Fix bug in constructor homing where it would use ctor homing when a class only has copy/move constructors

rnk added a comment. In D87808#2280197 , @dblaikie wrote: > @rsmith What's the deal with these anonymous structs/unions? Why do they have > copy/move constructors (are those technically called from the enclosing > class's copy/move constructors?) but no

[PATCH] D80172: Revert "Re-fix _lrotl/_lrotr to always take Long, no matter the platform."

rnk added a comment. I'd be fine going back to the behavior from before. > Without this behavior there is no intrinsic for 32 bit rotates on these > platforms. Strictly speaking, there seems to be `__builtin_rotate(left|right)32`, which is portable to all clang platforms. Repository: rG

[PATCH] D79895: Add a new warning to warn when passing uninitialized variables as const reference parameters to a function

rnk added inline comments. Comment at: clang/lib/Sema/AnalysisBasedWarnings.cpp:1518 UsesMap uses; + UsesMap constRefUses; If possible, it would be nice to avoid having a second map. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79895/new/

[PATCH] D79121: Add nomerge function attribute to clang

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I think this looks good and @zequanwu has addressed the concerns of other reviewers. Please wait until Wednesday before pushing in case they raise other concerns. Comment at:

[PATCH] D78659: Add nomerge function attribute to supress tail merge optimization in simplifyCFG

rnk added inline comments. Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:1305 +if (const auto *CB1 = dyn_cast(I1)) + if (CB1->cannotMerge()) +return Changed; zequanwu wrote: > rnk wrote: > > It seems inconsistent that we don't apply the

[PATCH] D79895: Fix warning about using uninitialized variable as function const reference parameter

rnk added a comment. Arthur recently went through the process of refining a diagnostic, so he should be able to help guide you in this if you have more questions. Comment at: clang/lib/Analysis/UninitializedValues.cpp:425 if ((*I)->getType().isConstQualified()) -

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79274/new/ https://reviews.llvm.org/D79274 ___ cfe-commits mailing list

[PATCH] D78659: Add nomerge function attribute to supress tail merge optimization in simplifyCFG

This revision was automatically updated to reflect the committed changes. Closed by commit rGcb22ab740355: Add nomerge function attribute to supress tail merge optimization in simplifyCFG (authored by zequanwu, committed by rnk). Changed prior to commit:

[PATCH] D79274: Fix template class debug info for Visual Studio visualizers

rnk added inline comments. Comment at: clang/test/CodeGenCXX/debug-info-codeview-display-name.cpp:10 +// RUN: %clang_cc1 -fblocks -debug-info-kind=limited -gcodeview -emit-llvm %s \ +// RUN: -o - -triple=x86_64-pc-win32 -Wno-new-returns-null -fms-compatibility | \ +//

[PATCH] D79636: [LangRef] Clarify the semantics of the `byval` attribute

rnk added a comment. Some copy editing comments, but I agree with the semantics: From the IR perspective, it is better to think of argument stack memory as belonging to the callee. A byval argument has more in common with a local static alloca than a passed in pointer.

<    1   2   3   4   5   6   7   8   9   10   >