[PATCH] D122087: Add HLSL Language Option and Preprocessor

rnk accepted this revision. rnk added a comment. Herald added a subscriber: StephenFan. lgtm Comment at: clang/lib/Frontend/InitPreprocessor.cpp:401-403 +uint32_t StageInteger = StageInteger = +(uint32_t)TI.getTriple().getEnvironment() - +(uint32_t)llvm::Tri

[PATCH] D121687: [clang-tidy] Don't try to build CTTestTidyModule for Windows with dylibs

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

[PATCH] D121733: Clean pathnames in FileManager.

rnk added a comment. I've been somewhat afraid to touch this code because of symlinks. Is that something we need to worry about? Consider this path: root/symlink/../foo.h. remove_dots will turn this into root/foo.h, but if symlink points to some unrelated directory, the .. directory entry point

[PATCH] D121865: [CodeGen] Inline _byteswap_* builtins.

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

[PATCH] D121497: Lex: add support for `{,u}i128` Microsoft extension

rnk added a comment. Seems reasonable, this could use far more testing. Comment at: clang/test/Lexer/ms-extensions.c:27 +#define INT128_MAX 170141183460469231731687303715884105727i128 +#define UINT128_MAX 0xui128 + This seems li

[PATCH] D121412: Complete the list of single-underscore keywords for MSVC compat.

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

[PATCH] D121412: Complete the list of single-underscore keywords for MSVC compat.

rnk added a subscriber: rsmith. rnk added a comment. @rsmith has previously advocated for a policy of only permitting conforming language extensions under fms-extensions. These single underscore names are not in the implementors namespace, so he has argued they should be under fms-compatibility

[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

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

[PATCH] D120936: [Sema][Windows] Don't special-case void* in __unaligned conversions.

rnk added a comment. I remember writing up feedback on this patch, but it's gone now, oh well. I wasn't able to find any history for why `void*` was special here. I see that MSVC doesn't warn when converting from `__unaligned int*` to `int*`, but that seems dangerous. Our team has an active fea

[PATCH] D119910: [clang][debug] port clang-cl /JMC flag to ELF

rnk added inline comments. Comment at: llvm/test/Instrumentation/JustMyCode/jmc-instrument-elf.ll:1 -; REQUIRES: system-windows -; RUN: opt -jmc-instrument -mtriple=x86_64-pc-windows-msvc -S < %s | FileCheck %s -; RUN: opt -jmc-instrument -mtriple=aarch64-pc-windows-msvc -S < %

[PATCH] D119910: [clang][debug] port clang-cl /JMC flag to ELF

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Looks like I hit submit too early... I wanted to say the path thing is a pre-existing issue, and it seems reasonable to address that as a separate patch. If you don't have plans to do a follow up p

[PATCH] D119910: [clang][debug] port clang-cl /JMC flag to ELF

rnk added inline comments. Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:72 SmallString<256> FilePath(SP.getDirectory()); sys::path::append(FilePath, SP.getFilename()); sys::path::native(FilePath); These sys::path calls introduce host-dependence, and

[PATCH] D119910: [clang][debug] port clang-cl /JMC flag to ELF

rnk added inline comments. Herald added a project: All. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5372 + // This controls whether or not we perform JustMyCode instrumentation. + if (TC.getTriple().isOSBinFormatELF() && Args.hasArg(options::OPT_fjmc)) { +if (DebugInf

[PATCH] D111457: [test] Add lit helper for windows paths

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

[PATCH] D119051: Extend the C++03 definition of POD to include defaulted functions

rnk added a comment. I would structure this as a LangOpt, and feed the target checks and ABI compat checks into the default setting for it. It could be something like `DefaultedSMFArePOD` / `-f[no-]defaulted-smf-are-pod` (smf being special member functions). The LangOpt default is true, and the

[PATCH] D111457: [test] Add lit helper for windows paths

rnk added inline comments. Comment at: llvm/docs/TestingGuide.rst:577 +``${fssep}`` + Exp``ands to the file system separator, i.e. ``/`` or ``\`` on Windows. + Stray backticks in "Expands"? Comment at: llvm/utils/lit/lit/TestRunner.py:1124 +

[PATCH] D120666: [docs] Add note about interaction between clang plugins and -clear-ast-before-backend

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

[PATCH] D120666: [docs] Add note about interaction between clang plugins and -clear-ast-before-backend

rnk added a comment. Thanks for writing docs, apologies for bikeshedding. Comment at: clang/docs/ClangPlugins.rst:207 + +If the main AST action is codegen, having any plugins that run after the +codegen action automatically turns off ``-clear-ast-before-backend``. -

[PATCH] D120629: [clang] Remove unused variable AllElementsInt. NFC.

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

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

rnk added a comment. This broke our libc++ -Werror build: llvm-project/libcxx/src/filesystem/directory_iterator.cpp:107:57: error: unqualified call to std::move [-Werror,-Wunqualified-std-cast-call] __root_(move(other.__root_)),

[PATCH] D112081: Define __STDC_NO_THREADS__ when targeting windows-msvc (PR48704)

rnk added a comment. In D112081#3330585 , @emmenlau wrote: > Current OpenSSL 1.1.1m requires `__STDC_NO_ATOMICS__` to build with clang-cl > against MSVC. Is this on-topic here? I think stdatomic.h is supposed to work with clang-cl, see this comment here

[PATCH] D119711: Add asan support for MSVC debug runtimes

rnk added subscribers: cbezault, vitalybuka. rnk added a comment. I'm hesitant to do this. The main reason we disabled the use of the debug runtimes was that the debug runtimes interfered with malloc interception, which leads to crashes and a generally poor user experience. I'd like some confir

[PATCH] D116861: [UBSan] Fix incorrect alignment reported when global new returns an offset pointer

rnk added inline comments. Comment at: clang/lib/CodeGen/CGExprCXX.cpp:1658-1659 Target.getNewAlign(), getContext().getTypeSize(allocType))); allocationAlign = std::max( allocationAlign, getContext().toCharUnitsFromBits(AllocatorAlign)); } --

[PATCH] D119215: [clang] Properly cache member pointer LLVM types

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

[PATCH] D119215: [clang] Properly cache member pointer LLVM types

rnk added a comment. Seems reasonable Comment at: clang/lib/CodeGen/CodeGenTypes.cpp:776 + auto *C = MPTy->getClass(); + auto I = RecordsWithOpaqueMemberPointers.find(C); + if (I == RecordsWithOpaqueMemberPointers.end()) { Can this use something

[PATCH] D84225: [CFE] Add nomerge function attribute to inline assembly.

rnk added a comment. In D84225#3304189 , @pengfei wrote: > It's not a workaround. We do need to avoid the merging sometime. For example, > given we have 2 branches begin with inline asm of `endbr`. We have to use > `nomerge` to stop them been merged out

[PATCH] D119234: [clang] [MinGW] Recognize -lcrtdll as a library replacing -lmsvcrt

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

[PATCH] D118744: [clang] Fix some clang->llvm type cache invalidation issues

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

[PATCH] D84225: [CFE] Add nomerge function attribute to inline assembly.

rnk added a comment. I think LLVM already doesn't do some tail merging optimizations on inline asm, but allowing the use of the attribute is more principled, and will block more optimizations (CSE). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84

[PATCH] D119051: Fix pod-packed functionality to use the C++11 definition of pod-ness

rnk added a comment. Looks like the test fails on the Windows pre-merge bot: https://buildkite.com/llvm-project/premerge-checks/builds/77696#1836f181-a998-4695-b587-a83239ea The debian bot seems to be failing for unrelated (clang tooling) reasons. Comment at: clang/lib/AS

[PATCH] D118744: [clang] Fix some clang->llvm type cache invalidation issues

rnk added a comment. This seems unfortunately complex, but I think we can live with it for a year or two. Is it possible to use the compile time tracker to benchmark if this clang->LLVM type cache actually saves compile time? This change disables the cache pretty often, and I'm wondering if "p

[PATCH] D118428: [clang-cl] Support the /JMC flag

rnk added a comment. In D118428#3294935 , @ychen wrote: > The instrumentation is per-function, ideally for each function that has > debuginfo and ends up in the executable. So I want it to happen the last in > the IR codegen pipeline (target could arran

[PATCH] D118428: [clang-cl] Support the /JMC flag

rnk added a comment. In D118428#3294411 , @ychen wrote: > The passes in `lib/Transforms/Instrumentation` runs with > `EmitAssemblyHelper::RunOptimizationPipeline`. JMC pass runs with > `EmitAssemblyHelper::RunCodegenPipeline`. Sure, but that's a choice

[PATCH] D118428: [clang-cl] Support the /JMC flag

rnk added inline comments. Comment at: llvm/lib/CodeGen/JMCInstrumenter.cpp:145 + LLVMContext &Ctx = M.getContext(); + bool UseX86FastCall = Triple(M.getTargetTriple()).getArch() == Triple::x86; + ychen wrote: > ychen wrote: > > hans wrote: > > > I still worry

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

rnk added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1890 - bool FieldPacked = Packed || D->hasAttr(); + llvm::Triple Target = Context.getTargetInfo().getTriple(); + bool FieldPacked = (Packed && (!FieldClass || FieldClass->isPOD() ||

[PATCH] D117262: [NFC] Store Address's alignment into PointerIntPairs

rnk accepted this revision. rnk added a comment. lgtm BTW, as a follow-on cleanup, which I may attempt, I think we could convert this alignment from CharUnits to llvm::Align. The vast majority of the `getAlignment` callers feed it to CGBuilder::CreateAlignedLoad/Store: http://llvm-cs.pcc.me.uk/

[PATCH] D117616: GCC ABI Compatibility: Preserve alignment of non-pod members in packed structs

rnk added a comment. In addition to the ABI compatibility concern, IMO, GCC's behavior here is better and more user friendly: - C++ classes have methods - calling a method on a field takes the address of a field - if the field is in a packed struct, it may be misaligned - misaligned loads and st

[PATCH] D45438: [CodeView] Enable debugging of captured variables within C++ lambdas

rnk added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:812-814 + // CodeView types with C++ mangling need a type identifier. + if (CGM.getCodeGenOpts().EmitCodeView) +return true; dblaikie wrote: > rnk wrote: > > dblaikie wrote: > > > bwyma wrot

[PATCH] D45438: [CodeView] Enable debugging of captured variables within C++ lambdas

rnk added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:812-814 + // CodeView types with C++ mangling need a type identifier. + if (CGM.getCodeGenOpts().EmitCodeView) +return true; dblaikie wrote: > bwyma wrote: > > dblaikie wrote: > > > rnk wrot

[PATCH] D117262: [NFC] Store Address's alignment into PointerIntPairs

rnk added inline comments. Comment at: clang/lib/CodeGen/Address.h:30 + // Int portion stores lower 3 bits of the log of the alignment. + llvm::PointerIntPair ElementType; dblaikie wrote: > nikic wrote: > > dblaikie wrote: > > > aeubanks wrote: > > > > nikic

[PATCH] D117262: [NFC] Store Address's alignment into PointerIntPairs

rnk added a comment. Thanks! Does this have any impact on binary size, as a proxy for more thorough performance testing? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117262/new/ https://reviews.llvm.org/D117262 __

[PATCH] D116861: [UBSan] Fix incorrect alignment reported when global new returns an offset pointer

rnk added a subscriber: rjmccall. rnk added a comment. I think the main thing here is that the diagnostic is incorrect: It claims that the type requires `__STDC_DEFAULT_NEW_ALIGNMENT__` (16) byte alignment, but it doesn't, it only requires 8. That's confusing. The next question is, does LLVM ex

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

rnk added a comment. In D97446#3240309 , @JonChesterfield wrote: > I don't know the context of this patch but changing attribute((used)) to put > things under compiler.used is definitely a breaking change. Please introduce > a new attribute if necessary

[PATCH] D115942: [X86][MS] Change the alignment of f80 to 16 bytes on Windows 32bits to match with ICC

rnk added a comment. Right, the auto-upgrade should upgrade and prevent the need to change the bitcode data layout. LTO is the main use case for auto-upgrading. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/ https://reviews.llvm.org/D11

[PATCH] D45438: [CodeView] Enable debugging of captured variables within C++ lambdas

rnk added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:812-814 + // CodeView types with C++ mangling need a type identifier. + if (CGM.getCodeGenOpts().EmitCodeView) +return true; rnk wrote: > dblaikie wrote: > > Just came across this code today

[PATCH] D115942: [X86][MS] Change the alignment of f80 to 16 bytes on Windows 32bits to match with ICC

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I'm happy with this, but I'd like to get an ack from @craig.topper or @efriedma about the backwards compatibility concern. I think it's addresed. Comment at: llvm/lib/IR/AutoUpgra

[PATCH] D45438: [CodeView] Enable debugging of captured variables within C++ lambdas

rnk added inline comments. Comment at: lib/CodeGen/CGDebugInfo.cpp:812-814 + // CodeView types with C++ mangling need a type identifier. + if (CGM.getCodeGenOpts().EmitCodeView) +return true; dblaikie wrote: > Just came across this code today - it's /probab

[PATCH] D114639: Raise the minimum Visual Studio version to VS2019

rnk added a comment. In D114639#3199458 , @RKSimon wrote: > @rnk I'm not certain but I think the only buildbot still using VS2017 is > windows-gcebot2 ? Yes, it probably uses VS2017, and probably also the sanitizer-windows bot as well: https://lab.llvm

[PATCH] D115942: [X86][MS] Change the alignment of f80 to 16 bytes on Windows 32bits to match with ICC

rnk added a comment. However, do we need to make changes to `llvm::UpgradeDataLayoutString`? Otherwise, I believe old bitcode for MSVC targets will no longer be able to be loaded for LTO. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115942/new/

[PATCH] D115942: [X86][MS] Change the alignment of f80 to 16 bytes on Windows 32bits to match with ICC

rnk added a comment. In D115942#3227047 , @pengfei wrote: > In D115942#3226146 , @rnk wrote: > >> Yeah, let's try to reach some resolution on that. > > The things are different. We don't support f80 type on Window

[PATCH] D116835: Use sha256 for source file debug info checksums with MSVC compat >= 1930

rnk added a comment. I think it's worth taking the time to hook up /ZH. Sorry we missed the patch last March. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116835/new/ https://reviews.llvm.org/D116835 _

[PATCH] D115103: Leak Sanitizer port to Windows

rnk added a subscriber: aganea. rnk added a comment. See a recent change rG7cd109b92c72855937273a6c8ab19016fbe27d33 by @aganea, who presumably was able to run the ASan tests for it. CHANGES SINCE LAST ACTION https://review

[PATCH] D115103: Leak Sanitizer port to Windows

rnk added a comment. In D115103#3209468 , @vitalybuka wrote: > In D115103#3209326 , @clemenswasser > wrote: > >> @vitalybuka >> No `check-asan` **doesn't work** for me. It just hangs forever and does >> absolut

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

rnk accepted this revision. rnk added a comment. lgtm I believe you can go ahead and land this, it doesn't depend on the data layout changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115441/new/ https://reviews.llvm.org/D115441

[PATCH] D115942: [X86][MS] Change the alignment of f80 to 16 bytes on Windows 32bits to match with ICC

rnk requested changes to this revision. rnk added a comment. This revision now requires changes to proceed. Yeah, let's try to reach some resolution on that. In the mean time, I discovered the `alignstack` parameter attribute: https://llvm.org/docs/LangRef.html#parameter-attributes Could that be

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86-64 datalayout

rnk added a comment. Herald added a subscriber: ormris. In D86310#2231378 , @efriedma wrote: > As far as I know, there are basically three categories of things that depend > on the alignment of a type. > > 1. The default alignment of load/store/alloca. O

[PATCH] D115942: [X86][MS] Change the alignment of f80 to 16 bytes on Windows 32bits to match with ICC

rnk accepted this revision. rnk added subscribers: jyknight, thakis. rnk added a comment. This revision is now accepted and ready to land. Thanks, looks good. I have a minor code refactoring suggestion, but feel free to resolve it as you see fit and land it. Comment at: clang/

[PATCH] D114908: [clang] Don't call inheritDefaultTemplateArguments() on CXXDeductionGuideDecl's template parameters

rnk added a comment. Friendly ping for a modules CTAD bugfix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114908/new/ https://reviews.llvm.org/D114908 ___ cfe-commits mailing list cfe-commits@lists.llv

[PATCH] D116599: Simplify AttrBuilder storage for target dependent attributes

rnk added a comment. In D116599#3221724 , @nikic wrote: > LG from my side, but I'd like a second opinion for the "require LLVMContext > in AttrBuilder" part of the change, as that's the main API impact. I think every non-toy frontend for LLVM probably u

[PATCH] D116020: [clang][#52782] Bail on incomplete parameter type in stdcall name mangling

rnk accepted this revision. rnk added a comment. lgtm Comment at: clang/test/CodeGen/pr52782-stdcall-func-decl.cpp:10 +class nsICanvasRenderingContextInternal { + NS_IMETHOD_(nsresult) InitializeWithDrawTarget(NotNull); +} nsTBaseHashSet; rnk wrote: > Please c

[PATCH] D116020: [clang][#52782] Bail on incomplete parameter type in stdcall name mangling

rnk added a comment. > clang actually refuses taking the address of a stdcall function if it has an > incomplete type parameter: I was going to say, I thought I remembered fixing this problem, and I guess that's how I fixed it: with errors. MSVC doesn't add stdcall mangling suffixes to non-ext

[PATCH] D116599: Simplify AttrBuilder storage for target dependent attributes

rnk added a comment. Nice! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116599/new/ https://reviews.llvm.org/D116599 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Sorry for the delay, I got sick last week and read through email newest to oldest. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D43002/new/ htt

[PATCH] D115456: Implement on-demand TLS initialization for Microsoft CXX ABI

rnk added a comment. I probably don't have time to review this, so let me redirect to @hans. Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:2400 +static llvm::GlobalValue *getTlsGuardVar(CodeGenModule &CGM) { + llvm::Type *VTy = llvm::Type::getInt8Ty(CGM.getLLVMContext())

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

rnk added a comment. I see, thanks for the info. Can you please add a targeted LLVM test for long double arguments? From what I can tell, the auto-generated update_llc_test_checks.py style tests are not a good fit for testing parameter passing because they pattern-match away the stack offsets

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

rnk added a comment. Let me know if it would be more helpful to set up a call, that might help us reach agreement sooner. I've used discord for this previously if that works for you, my username there is the same (`@rnk#8591`). In D115441#3194066 , @pen

[PATCH] D112081: Define __STDC_NO_THREADS__ when targeting windows-msvc (PR48704)

rnk added a comment. @hans, I don't think you landed this. Should we do it? I came here after reviewing my issues on github: https://github.com/llvm/llvm-project/issues/48048 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112081/new/ https://review

[PATCH] D115798: Provide SmallAttrBuilder as a lightweight alternative to AttrBuilder

rnk added a comment. I would really prefer to avoid adding a new variant of AttrBuilder. What is the main blocker to making AttrBuilder more efficient? It just needs an `LLVMContext`, right? Would that be feasible instead? Most AttrBuilders are constructed from existing AttributeLists, which ha

[PATCH] D114394: Compile-time computation of string attribute hashes

rnk added a comment. That's interesting, somehow clang spends 1% of its time for sqlite3 in attribute lookup. I wonder how we ended up doing so much string-based attribute lookup. These were not really ever intended to be used for anything semantically interesting for the middle end, they were

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

rnk added a comment. In D115441#3191482 , @pengfei wrote: > We have to change LLVM data layout because it's required by the calling > conversion. Is that necessary? It would be simpler to leave the fp80 value 4 byte aligned, which I believe is consiste

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

rnk added a comment. In D115441#3188526 , @mstorsjo wrote: > In D115441#3188172 , @pengfei wrote: > >>> In GCC on Windows (and clang in mingw mode), long double is always 80 bit >>> on x86. (On i386, sizeof(long

[PATCH] D115441: [X86][MS] Add 80bit long double support for Windows

rnk added a subscriber: mstorsjo. rnk added a comment. I seem to recall assuming that Windows `long double` was 64-bits in many, many places. Unfortunately, I have no idea where that could've happened. Something that comes to mind immediately is the MSVC name mangler. I don't think that's a blo

[PATCH] D102090: [CMake][ELF] Link libLLVM.so and libclang-cpp.so with -Bsymbolic-functions

rnk added a comment. In D102090#3172693 , @ebrevnov wrote: > I don't see what's wrong here (maybe you have specific example). IMHO, quite > opposite, LLVM will consistently use it's own instance while user's binary > it's own as if there were two globa

[PATCH] D106585: Fix clang debug info irgen of i128 enums

rnk added a comment. This usage of isSameValue seems suspicious: https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/LLVMContextImpl.h#L389 It seems to allow the possibility that APInts of differing bitwidth compare equal, but the hash value incorporates the bitwidth, so they may be inse

[PATCH] D106585: Fix clang debug info irgen of i128 enums

rnk added a subscriber: aeubanks. rnk added a comment. Thanks for the reduction, it sounds like there is something wrong with the way DIEnumerator is uniqued in the LLVMContext. I probably don't have time to follow up on this, but maybe @dblaikie and @aeubanks can help out. Repository: rG LL

[PATCH] D102090: [CMake][ELF] Link libLLVM.so and libclang-cpp.so with -Bsymbolic-functions

rnk added a comment. In D102090#3169439 , @ebrevnov wrote: > While -Bsymbolic-funtions brings nice performance improvements it also > changes symbol resolution order. That means we effectively disabled > preemption for functions and all references from

[PATCH] D114902: [Attrs] Elaborate on the trivial_abi documentation

rnk created this revision. rnk added reviewers: aaron.ballman, rjmccall, dblaikie, ahatanak. rnk requested review of this revision. Herald added a project: clang. My team recently answered some questions about the trivial_abi attribute. This change attempts to distill those answers into user-focus

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

rnk added a comment. Sounds good. This patch is still pending, right? I don't see it in the logs. Do you need someone to push it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111639/new/ https://reviews.llvm.org/D111639 _

[PATCH] D114576: [PR52549][clang-cl] Predefine _MSVC_EXECUTION_CHARACTER_SET

rnk added a comment. In D114576#3153189 , @mstorsjo wrote: > Would it be possible to move setting of this flag to somewhere else (e.g. > somewhere in Driver/ToolChains/MSVC.cpp), so that it would be picked up also > when building with the gcc-style driv

[PATCH] D114651: [clang-cl] Expose -Wall to clang-cl by unaliasing -Wall, keeping /Wall as alias to -Weverything

rnk added a comment. It looks like this Wall -> Weverything alias is from my 2017 change: https://reviews.llvm.org/rGf9b08a382cc1e0669805991849ad69efbd4703e8 The commit message doesn't link to any bugs or any other motivating material. All I said is that this is being done for MSVC compatibility

[PATCH] D114651: [clang-cl] Expose -Wall to clang-cl by unaliasing -Wall, keeping /Wall as alias to -Weverything

rnk added a comment. I will add that multiple users have run into this problem, and I think it might be more practical to consider unaliasing Wall altogether. Clang doesn't emit the same set of warnings as MSVC. Anyone seriously using `clang-cl /Wall` is going to receive a pile of `-WcxxNN-comp

[PATCH] D114082: [WIP] Normalize String Attributes

rnk added subscribers: void, rnk. rnk added a comment. > it is possible to list all internal keys in one place I think one of the original goals of adding support for string attributes (maybe @void will give some input) was specifically to avoid having one giant list with all the attributes sup

[PATCH] D114394: Compile-time computation of string attribute hashes

rnk added subscribers: aeubanks, rnk. rnk added a comment. I think @aeubanks might be a good reviewer for this. I like the performance wins here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114394/new/ https://reviews.llvm.org/D114394 ___

[PATCH] D111457: [clang][test] Add lit helper for windows paths

rnk added a comment. These seem like generic substitutions that would be useful to anyone writing lit tests. We already have `%{pathsep}`: https://llvm.org/docs/CommandGuide/lit.html#substitutions These should live there as well, and be documented similarly, I think. Repository: rG LLVM Gith

[PATCH] D113490: [NFC] Let Microsoft mangler accept GlobalDecl

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/lib/AST/MicrosoftMangle.cpp:47 + if (auto *CD = dyn_cast(DC)) +GD = GlobalDecl(CD, Ctor_Complete); + else if (auto *DD = dyn_cast(DC))

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

rnk added a comment. In D110257#3134001 , @JonChesterfield wrote: > So you won't articulate or document the new invariant and you think there's a > llvm-dev discussion that says we can't verify the invariant which you won't > reference, but means you w

[PATCH] D113707: [clang] Make -masm=intel affect inline asm style

rnk added a comment. In D113707#3132812 , @thakis wrote: > Fairly big update. I'd like to punt on the pragma for now since this became a > lot more involved than expected already (see dependent patches; several of > them have already landed). No proble

[PATCH] D113707: [clang] Make -masm=intel affect inline asm style

rnk added a comment. One alternative would be to control this setting with a pragma stack, like we do for warnings, struct packing, etc. Something like: // foo.h #pragma clang asm_dialect push "att" static inline ... foo { asm("..."); } #pragma clang asm_dialect pop The pragma really wo

[PATCH] D112921: [clang] Enable sized deallocation by default in C++14 onwards

rnk added a comment. Let's not bring back the weak function thunk approach. That approach caused problems described in my commit, D8467 , which is why the code was removed. The next steps are to sort out right defaults for Apple and understand the libc++ test fa

[PATCH] D113707: [clang] Make -masm=intel affect inline asm style

rnk added a comment. This seems reasonable, but I worry about headers that contain AT&T style inline asm blobs, such as clang's own intrinsic headers. As you say, the directive is local. I would hate to end up in a place where we have to prefix every asm blob in clang/lib/Headers/*mmintrin.h wi

[PATCH] D8467: C++14: Disable sized deallocation by default due to ABI breakage

rnk added a comment. In D8467#3125386 , @rjmccall wrote: > Conceptually, this is (and will always be) a platform decision. On Apple > platforms, we have a formalized concept of deployment target, where specific > minimum OS versions support sized dealloc

[PATCH] D106585: Fix clang debug info irgen of i128 enums

rnk added a subscriber: hans. rnk added a comment. In D106585#3122726 , @glandium wrote: > It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8 > . Are you sure yo

[PATCH] D112492: [CUDA][HIP] Allow comdat for kernels

rnk added a comment. I think the key is the self-reference in the LEA instruction: > ; foo > .seh_proc "??$foo@H@@YAXH@Z" > ... > leaq"??$foo@H@@YAXH@Z"(%rip), %rcx > ... > ; foo > .seh_proc "??$foo@M@@YAXM@Z" > ... > leaq"??$foo@M@@YAXM@Z"(%rip), %rcx >

[PATCH] D112492: [CUDA][HIP] Allow comdat for kernels

rnk added a comment. In D112492#3119892 , @tra wrote: > Yes, we do need to merge identical functions with **identical names** for > templates. > > The comdat-folding issue is different. IIUIC, it allows merging two functions > with identical code and **

[PATCH] D113490: [NFC] Let Microsoft mangler accept GlobalDecl

rnk added inline comments. Comment at: clang/lib/AST/MicrosoftMangle.cpp:47 + if (auto *CD = dyn_cast(DC)) +GD = GlobalDecl(CD, Ctor_Complete); + else if (auto *DD = dyn_cast(DC)) I would prefer if you passed Ctor_Base and Dtor_Base here. I believe MSVC mo

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm I believe you have addressed John's comments, and I think the IR changes mainly affect AMDGPU users, so I don't think this will be too disruptive. Sorry about the delay, there's a bit of a bys

[PATCH] D112492: [CUDA][HIP] Allow comdat for kernels

rnk added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4290-4293 - // Do not set COMDAT attribute for CUDA/HIP stub functions to prevent - // them being "merged" by the COMDAT Folding linker optimization. - if (D.hasAttr()) -return false; --

[PATCH] D112492: [CUDA][HIP] Allow comdat for kernels

rnk added a subscriber: zequanwu. rnk added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4290-4293 - // Do not set COMDAT attribute for CUDA/HIP stub functions to prevent - // them being "merged" by the COMDAT Folding linker optimization. - if (D.hasAttr())

[PATCH] D106585: Fix clang debug info irgen of i128 enums

rnk added a comment. Reviewing the code, I don't see any obvious sources of non-determinism (hash iteration). The only source I can imagine is uninitialized memory usage in the APInt. Let me know if this can be repro'd somehow. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Sorry for the delay Comment at: clang/lib/Sema/TreeTransform.h:14576 !Second->getType()->isOverloadableType()) return getSema().CreateBuiltinArraySubscriptExp

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