[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] 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 ___ cfe-com

[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 with

[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 https://lists.llvm.org/cgi-bin/mailman/list

[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 it

[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 SINC

[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 == TSK_Implici

[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 L

[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 https:/

[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 t

[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 cc

[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 unspecifie

[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&ss=chromium Regarding -Oz / minsize / SizeLevel

[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 languag

[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: cfe-commits.

[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 wr

[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 and

[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] 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 via

[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 isAA

[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] 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 `-fc++-ab

[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 in

[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 clang/test/CodeGenCXX/mic

[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 L

[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 when

[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 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 cfe-commits@li

[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 cfe-comm

[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] 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 speci

[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 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 no

[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&id=307674#toc Repository: rG LLVM Github Monor

[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: clang/include/clang/Basic/BuiltinsAA

[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 wr

[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 ACTIO

[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 ___ cfe-co

[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 thinks

[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 like

[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 com

[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 rewrot

[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 c

[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 be

[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 http

[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 https://rev

[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] 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] 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] 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] 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 unin

[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 op

[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] 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 auto-detecting

[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] 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] 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/ https:

[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 G

[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 pointer

[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 &Other) const { +return std::tie(Device, File) < std::tie(Othe

[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 a

[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 ___ cfe-com

[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] 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 cfe-commits@lists.llv

[PATCH] D102339: [clang] On Windows, ignore case and separators when discarding duplicate dependency file paths.

rnk added inline comments. Comment at: clang/lib/Frontend/DependencyFile.cpp:145 + StringRef SearchPath; +#ifdef _WIN32 + // Make the search insensitive to case and separators. I feel like this should somehow be a property of the input virtual filesystem: we s

[PATCH] D100611: [Clang] Add clang attribute `clang_builtin_alias`.

rnk added subscribers: vsk, rnk. rnk added inline comments. Herald added a subscriber: jeroen.dobbelaere. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5155 +#define BUILTIN(ID, TYPE, ATTRS) case RISCV::BI##ID: +#include "clang/Basic/BuiltinsRISCV.def" +#undef BUILTIN -

[PATCH] D102339: [clang] On Windows, ignore case and separators when discarding duplicate dependency file paths.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Ideally we should avoid the ifdef, but we don't have a good proxy for case sensitivity of the filesystem here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102339/new/ https://revie

[PATCH] D102736: Fix tmp files being left on Windows builds.

rnk added a subscriber: jansvoboda11. rnk added a comment. For other reviewers, I know Duncan, @dexonsmith, has also spent some time working on the clang output file management APIs. And @jansvoboda11 as well, I think. Comment at: clang/lib/Frontend/CompilerInstance.cpp:720 +

[PATCH] D102583: -fno-semantic-interposition: Don't set dso_local on GlobalVariable

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

[PATCH] D102818: [PGO] Don't reference functions unless value profiling is enabled

rnk created this revision. rnk added reviewers: vsk, davidxl, Bigcheese. Herald added subscribers: wenlei, hiraditya. rnk requested review of this revision. Herald added projects: clang, Sanitizers, LLVM. Herald added a subscriber: Sanitizers. This reduces the size of chrome.dll.pdb built with opt

[PATCH] D102818: [PGO] Don't reference functions unless value profiling is enabled

rnk added a comment. Thanks! I'll wait to see if I can get an ack from the Apple folks who indicated that they are using frontend PGO. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102818/new/ https://reviews.llvm.org/D102818

[PATCH] D101479: [Driver] Support libc++ in MSVC

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, sorry for the delay, this fell off the end. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101479/new/ https://reviews.llvm.org/D101479

[PATCH] D102517: [clang] Add support for the "abstract" contextual keyword of MSVC

rnk added a comment. @hans, can you review this? I am trying to offload clang reviews. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102517/new/ https://reviews.llvm.org/D102517 ___ cfe-commits mailing l

[PATCH] D102818: [PGO] Don't reference functions unless value profiling is enabled

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8f20ac9595c8: [PGO] Don't reference functions unless value profiling is enabled (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D102736: Fix tmp files being left on Windows builds.

rnk added a comment. Nice, this seems to be working out. Comment at: clang/include/clang/Frontend/CompilerInstance.h:170 std::string TempFilename; +Optional TempFile; aganea wrote: > Can we always use `TempFile`? And remove `TempFilename` in that cas

[PATCH] D102736: Fix tmp files being left on Windows builds.

rnk accepted this revision. rnk added a comment. I think this look good. Adrian, are your concerns addressed? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102736/new/ https://reviews.llvm.org/D102736 __

[PATCH] D102970: [clang] [MinGW] Don't mark emutls variables as DSO local

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

[PATCH] D103293: [clang-cl] Bump default -fms-compatibility-version to 19.14

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm with the comment update I'm surprised there wasn't more fallout. Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:1340 IsWindowsMSVC)) { -// -fms-comp

[PATCH] D103495: [static initializers] Don't put ordered dynamic initializers of static variables into global_ctors

rnk added subscribers: aeubanks, hans, rsmith. rnk added a comment. +@rsmith @hans @aeubanks I think this would be an unfortunate code size and startup time regression for Itanium C++ inline variables. The existing code was written under the assumption that vague linkage (GVA_DiscardableODR) im

[PATCH] D103452: [clang] Fix reading long doubles with va_arg on x86_64 mingw

rnk added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:4364-4365 // MS x64 ABI requirement: "Any argument that doesn't fit in 8 bytes, or is // not 1, 2, 4, or 8 bytes, must be passed by reference." if (isAggregateTypeForABI(Ty) || Ty->isMemberPointerT

[PATCH] D103495: [static initializers] Don't put ordered dynamic initializers of static variables into global_ctors

rnk added a comment. In D103495#2794667 , @ychen wrote: > Is there anything preventing us from using the existing priority field to > define the order instead of introducing the order among initializers with the > same priority? If we go with 1., there

[PATCH] D103495: [static initializers] Don't put ordered dynamic initializers of static variables into global_ctors

rnk added a comment. In D103495#2794870 , @wolfgangp wrote: > For PS4, we use the .ctors scheme, and so the initialization order was > suddenly reversed, which was not noticed for a while until the user had a > dependency on the previous initialization

[PATCH] D92136: [clang] Enhanced test for --relocatable-pch, and corresponding fixes for Windows

rnk added a comment. Seems reasonable Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:534 +#ifdef _WIN32 +// Support special case of Windows absolute file paths rnk wrote: > I think looking for a slash after the colon is a bit more robust,

[PATCH] D93668: [clang] Override the Fuchsia platform ABI using the triple environment

rnk added a comment. I guess the spelling -fc++-abi= is intuitive, discoverable, and easy to use. I'd be OK going back to that, as long as the compiler knows which ABIs work on which targets and rejects the unsupported configurations. Regarding the idea of separating the platform ABI from the C

[PATCH] D95187: [DebugInfo][CodeView] Use as the display name for lambdas.

rnk added a comment. I see a variety of presubmit failures in Harbormaster that seem related, so make sure to check those. Comment at: clang/include/clang/AST/Mangle.h:92 + virtual StringRef getLambdaString(const CXXRecordDecl *Lambda) = 0; + I think I woul

[PATCH] D95187: [DebugInfo][CodeView] Use as the display name for lambdas.

rnk added a comment. I see a potential issue, but I think this looks good otherwise. Comment at: clang/lib/AST/ItaniumMangle.cpp:210 +if (Number == 0) + return getAnonymousStructId(Lambda); +return Number; This has the potential to create a situati

[PATCH] D95187: [DebugInfo][CodeView] Use as the display name for lambdas.

rnk added inline comments. Comment at: clang/include/clang/AST/Mangle.h:92 + virtual StringRef getLambdaString(const CXXRecordDecl *Lambda) = 0; + akhuang wrote: > rnk wrote: > > I think I would prefer to have this return a number. I think CGDebugInfo > > sho

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

rnk added a comment. We need a solution to our problem in the short term, though: there must be a code pattern that can be used in -std=c++14 modes to accomplish what the `, ## __VA_ARGS__` code pattern accomplishes with GCC extensions. As of right now, my understanding is that `__VA_OPT__` is

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

rnk added a comment. In D91913#2525993 , @hvdijk wrote: > @rnk Taking it upon yourself to revert this without approval and without > communication on all branches, especially given the earlier suggestion by > @rsmith to only revert this on the LLVM 12 br

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

rnk added a comment. OK, thanks for the direction. We'll try to port code to use the device you listed. I see you think we need a new warning for this, a clang 12 release note, and then this can reland. @hvdijk , that plan looks good to me, and I'll let you and Richard sort it out. I don't pla

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

rnk added a comment. In D91913#2526317 , @hvdijk wrote: > My concern isn't with the revert itself, it's without waiting for approval. > That's a crystal clear LLVM developer policy violation: changes need to be > approved, or obvious, or by developers re

[PATCH] D95187: [DebugInfo][CodeView] Use as the display name for lambdas.

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

[PATCH] D95499: [NFC] Disallow unused prefixes under clang/test/CodeGenCXX

rnk accepted this revision. rnk added a subscriber: akhuang. rnk added a comment. This revision is now accepted and ready to land. lgtm I believe @akhuang's recent change to this file made it so debug info names are unqualified in all modes. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D95482: [PATCH] [clang] Fix bug 48848 by removing assertion related to recoverFromMSUnqualifiedLookup

rnk added a subscriber: erik.pilkington. rnk added a comment. @erik.pilkington , you added this assert in http://github.com/llvm/llvm-project/commit/3cdc317342d8c2b36de2839ea6ebefec17cb271e, are you OK with this? I'm OK with the change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D69322: [hip][cuda] Enable extended lambda support on Windows.

rnk added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:395-400 /// The number used to indicate this lambda expression for name /// mangling in the Itanium C++ ABI. unsigned ManglingNumber : 31; +/// The device side mangling number. +unsi

[PATCH] D95673: [dllimport] Honor always_inline when deciding whether a dllimport function should be available for inlining (PR48925)

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

[PATCH] D95482: [PATCH] [clang] Fix bug 48848 by removing assertion related to recoverFromMSUnqualifiedLookup

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. In the absence of a response, let's land this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95482/new/ https://reviews.llvm.org/D95482 _

[PATCH] D95534: clang-cl: Invent a /winsysroot concept

rnk added subscribers: smeenai, compnerd. rnk added a comment. +People who have spent time on cross-compilation Windows sysroots in the past: @smeenai @compnerd Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95534/new/ https://reviews.llvm.org/D955

[PATCH] D95918: [Coverage] Propogate counter to condition of conditional operator

rnk added a comment. Nice. I believe there are some integration tests in compiler-rt/test/profile. First, please run them and make sure they pass. Second, @vsk, do you think it's worth adding an integration test for this, seeing as the bug is really only observable when you put all the coverage

[PATCH] D69322: [hip][cuda] Enable extended lambda support on Windows.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Looks good with one suggestion. Comment at: clang/include/clang/AST/ASTContext.h:540 llvm::MapVector MangleNumbers; llvm::MapVector StaticLocalNumbers; Thi

<    6   7   8   9   10   11   12   13   14   15   >