[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

rnk added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:14254 int SectionFlags = ASTContext::PSF_Read; -if (var->getType().isConstQualified()) { - if (HasConstInit) rsmith wrote: > efriedma wrote: > > rnk wrote: > > > I think this is not

[PATCH] D152752: [MS] Fix passing aligned records by value in some cases

rnk added a comment. Following up, the vararg fix is here: https://github.com/llvm/llvm-project/pull/65692 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152752/new/ https://reviews.llvm.org/D152752 ___

[PATCH] D152604: [Driver] Default -fsanitize-address-globals-dead-stripping to true for ELF

rnk added a comment. It sounds like two users have hit this now: Chromium and Rust folks. This is a flag flip, so it's pretty small and safe to rollback, and IMO we should consider that while we debug the underlying issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION http

[PATCH] D154007: Reland "Try to implement lambdas with inalloca parameters by forwarding without use of inallocas."

rnk added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:5104 +if (CallInfo.isDelegateCall()) { + NeedCopy = false; +} else if (Addr.getAlignment() < Align && Please add a comment about this. We need to avoid copying uncopyable

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

rnk added a comment. I see two ways forward here: 1. Autoupgrade modules with old datalayout strings by increasing the alignment of i128 & co. This will change LLVM IR struct layouts, argument alignments, etc. As far as native ABI boundaries are concerned, this should be "more correct": Clang

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

rnk added a comment. In D86310#4498575 , @efriedma wrote: > https://reviews.llvm.org/D86310#2231136 has an example where IR generated by > clang breaks. Right, so we'd break LTO of packed structs with i128 members. I still think we're overthinking this,

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

rnk added a comment. In D86310#4501170 , @hvdijk wrote: > For example, it would generally be better if long double were 8-byte-aligned, > but the x86 32-bit ABI specifies that it is 4-byte-aligned, and that is set > in stone. I would be against any chang

[PATCH] D152752: [MS] Fix passing aligned records by value in some cases

rnk added a comment. Thanks, I think this is a clang bug. As I understand MSVC's behavior, we should not pass highly aligned variadic arguments indirectly: https://gcc.godbolt.org/z/Kr67xWTeE I'll follow up on that. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

rnk added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417 +not significant. This allows global constants with the same contents to be +merged. This can break global pointer identity, i.e. two different globals have +the same address. + ---

[PATCH] D158538: [MS-ABI] Remove comdat attribute for inheriting ctor.

rnk added a comment. It took me a while to find this code to understand the issue. The problem is that there is an inconsistency between the GVA linkage and the LLVM IR linkage for these constructors. In this godbolt example , `??0?$TInputDataVertexModel..`

[PATCH] D158538: [MS-ABI] Remove comdat attribute for inheriting ctor.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm with some tweaks to the test. Comment at: clang/test/CodeGenCXX/ms-inheriting-ctor.cpp:41 + +// CHECK-LABEL: define internal noundef ptr @"??0?$B@_N@@QEAA@AEBVF@@AEBUA@@@Z"(p

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

rnk added a subscriber: dblaikie. rnk added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417 +not significant. This allows global constants with the same contents to be +merged. This can break global pointer identity, i.e. two different globals have +t

[PATCH] D158857: [clang][aarch64] Add support for the MS qualifiers __ptr32, __ptr64, __sptr, __uptr for aarch64

rnk added a subscriber: efriedma. rnk added a comment. +@efriedma, since I think we had that ongoing thread about what to do with i128 alignment in data layout. Is this a reasonable way to handle the datalayout migration? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158857/new/ https

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

rnk added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:10110-10111 +// propagated to the new node prior to instantiation. +if (PrevDecl && PrevDecl->hasAttr()) { + Specialization->addAttr(PrevDecl->getAttr()); + Consumer.AssignInheritanceModel(Sp

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/test/CodeGenCXX/microsoft-abi-member-pointers.cpp:958 +extern template class a; +template class a; +} tahonermann wrote: > rnk wrote: > > My

[PATCH] D152752: [MS] Fix passing aligned records by value in some cases

rnk added a subscriber: mstorsjo. rnk added a comment. I need to get to it, my recollection is that @mstorsjo ran into the same issue here for mingw and made some changes, I wanted to go dig those up as a starting point. I may have completely forgotten things though. Repository: rG LLVM Gith

[PATCH] D152752: [MS] Fix passing aligned records by value in some cases

rnk added a comment. I put together a fix here: https://github.com/llvm/llvm-project/compare/main...rnk:llvm-project:fix-vararg-align I don't have arcanist set up like I used to, and I don't write as much code as I used to, so I kind of want to submit this as a pull request as soon as they are

[PATCH] D159133: [Sema] Make C++ functional-style cast warn about dropped qualifiers (-Wcast-qual)

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. This seems like it will generate warning cleanup work for vendors, so I'll mention #clang-vendors , but I think the code looks good. I don't think it ma

[PATCH] D153690: [clang][Sema] Remove dead diagnostic for loss of __unaligned qualifier

rnk added a comment. In D153690#4627938 , @hazohelet wrote: > @rnk This reproducer code should be accepted for clang to be aligned with > msvc's behavior, correct? Yes, I think so. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION htt

[PATCH] D137944: [ObjC][ARC] Teach the OptimizeSequences step of ObjCARCOpts about WinEH funclet tokens

rnk added a comment. I can't say I'm familiar with ARC, but this seems right to me. Comment at: llvm/lib/Transforms/ObjCARC/ObjCARCOpts.cpp:767 +const ColorVector &CV = BlockColors.find(BB)->second; +assert(CV.size() == 1 && "non-unique color for block!"); +BasicBlo

[PATCH] D137473: [vfs] Allow root paths relative to the directory of the vfsoverlay YAML file

rnk added subscribers: hans, compnerd, Bigcheese. rnk added inline comments. Comment at: llvm/unittests/Support/VirtualFileSystemTest.cpp:1878 + // with native path seperator, regardless of the actual path seperator + // used in YAMLFilePath field. +#ifndef _WIN32 -

[PATCH] D137872: Try to implement lambdas with inalloca parameters by forwarding without use of inallocas.

rnk added inline comments. Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:571 + /// Whether this function should avoid inalloca arguments. + unsigned DontInAlloca: 1; + This is an amusing name, but we should try to find something better. :) Can we bu

[PATCH] D138326: [CodeView] Don't generate dummy unnamed strcut/class/union type.

rnk resigned from this revision. rnk added a comment. Seems right to me, but I would prefer if someone else can review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138326/new/ https://reviews.llvm.org/D138326

[PATCH] D137872: Try to implement lambdas with inalloca parameters by forwarding without use of inallocas.

rnk added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:727 + if (signature.isInstanceMethod()) +opts |= static_cast(FnInfoOpts::IsInstanceMethod); + if (signature.isChainCall()) We could avoid static_cast by defining the standard flag operators

[PATCH] D138514: Sema: diagnose PMFs passed through registers to inline assembly

rnk added inline comments. Comment at: clang/lib/Sema/SemaStmtAsm.cpp:381 +if (!Context.getTargetInfo().getCXXABI().isMicrosoft()) + if (const auto *UO = dyn_cast(InputExpr)) +if (UO->getOpcode() == UO_AddrOf) This is too narrow, there are lots o

[PATCH] D137642: [X86][CodeGen] Fix crash in hotpatch

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

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

rnk added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:13896 + Diag(var->getLocation(), diag::err_constexpr_var_requires_const_init) + << var << Init->getSourceRange(); + } zahiraam wrote: > efriedma wrote: > > zahiraam wrot

[PATCH] D137107: Allow MS extension: support of constexpr with __declspec(dllimport).

rnk added a comment. To recap, I'm in favor of implementing dllimport constexpr global initializers with a high priority dynamic initializer. I would suggest using init_priority of 201 to run right after the "compiler" initializers. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137107/

[PATCH] D138514: Sema: diagnose PMFs passed through registers to inline assembly

rnk added inline comments. Comment at: clang/lib/Sema/SemaStmtAsm.cpp:381 +if (!Context.getTargetInfo().getCXXABI().isMicrosoft()) + if (const auto *UO = dyn_cast(InputExpr)) +if (UO->getOpcode() == UO_AddrOf) compnerd wrote: > aaron.ballman wrot

[PATCH] D76444: Use FinishThunk to finish musttail thunks

This revision was automatically updated to reflect the committed changes. Closed by commit rGce5173c0e174: Use FinishThunk to finish musttail thunks (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76444/new/ https://reviews.llvm.org

[PATCH] D77010: [OpenMP] set_bits iterator yields unsigned elements, no reference (NFC).

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Lgtm, thanks, I was seeing this locally Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77010/new/ https://reviews.llvm.org/D77010 __

[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor

rnk created this revision. rnk added a reviewer: rsmith. Herald added a project: clang. DONOTSUBMIT: Uploading for feedback on approach, still have test failures: Failing Tests (1): Clang :: CXX/class.access/p4.cpp In the MS C++ ABI, complete destructors for classes with

[PATCH] D75903: [AArch64][CodeGen] Fixing stack alignment of HFA arguments on AArch64 PCS

rnk added a comment. In D75903#1914715 , @ostannard wrote: > This means that `stk1` should be passed as a copy with alignment 16 bytes, > putting it at `sp+16`. GCC does this, clang without this patch passes it at > `sp+8`, and clang with this patch pass

[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor

rnk updated this revision to Diff 253715. rnk added a comment. - add def data bit - add tests - fix test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77081/new/ https://reviews.llvm.org/D77081 Files: clang/include/clang/AST/CXXRecordDeclDefinit

[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor

rnk added a comment. PTAL, here's how I imagine this is supposed to look, but the definition data bit name could probably be improved. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77081/new/ https://reviews.llvm.org/D77081

[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor

rnk marked 2 inline comments as done. rnk added a comment. Thanks for the feedback, I'm going to investigate if we can use the `used` destructor bit to do this. Comment at: clang/include/clang/AST/DeclCXX.h:959-963 + /// Indicates if the complete destructor has been implicitl

[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor

rnk updated this revision to Diff 253933. rnk added a comment. - Remove definition data bit tracking, use destructor isUsed bit Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77081/new/ https://reviews.llvm.org/D77081 Files: clang/include/clang/S

[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor

rnk added a comment. I'm glad to report that your suggestion worked out well! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77081/new/ https://reviews.llvm.org/D77081 ___ cfe-commits mailing list cfe-c

[PATCH] D77081: [MS] Mark vbase dtors ref'd when ref'ing dtor

rnk updated this revision to Diff 253972. rnk added a comment. - finish refactoring, build & test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77081/new/ https://reviews.llvm.org/D77081 Files: clang/include/clang/Sema/Sema.h clang/lib/Sema/Se

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

rnk added a comment. It's unsurprising that the Linux kernel's own Fortify implementation is incompatible with Clang. The whole feature should never have been implemented in the library to begin with, but here we are. I think the Linux kernel folks would prefer it if we can fix forward and find

[PATCH] D76184: [OpenMP][NFC] Remove the need to include `OpenMPClause.h`

rnk added a comment. In D76184#1956352 , @jdoerfert wrote: > @rnk What is the plan for this one now? Should I abandon it? Sorry, I got distracted. I tried reapplying it but it didn't apply cleanly. It's possible I hadn't locally applied your patch serie

[PATCH] D74452: [MS] Mark vectorcall FP and vector args inreg

rnk added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74452/new/ https://reviews.llvm.org/D74452 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin

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

rnk updated this revision to Diff 245491. rnk added a comment. - rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74455/new/ https://reviews.llvm.org/D74455 Files: clang/lib/CodeGen/MicrosoftCXXABI.cpp clang/test/CodeGenCXX/inalloca-overal

[PATCH] D74452: [MS] Mark vectorcall FP and vector args inreg

rnk updated this revision to Diff 245490. rnk added a comment. - rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74452/new/ https://reviews.llvm.org/D74452 Files: clang/lib/CodeGen/TargetInfo.cpp clang/test/CodeGen/vectorcall.c clang/te

[PATCH] D74452: [MS] Mark vectorcall FP and vector args inreg

This revision was automatically updated to reflect the committed changes. Closed by commit rG0edb2129258c: [MS] Mark vectorcall FP and vector args inreg (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74452/new/ https://reviews.llvm

[PATCH] D69471: [Coverage] Revise format to reduce binary size

rnk added a comment. Compiling with -fdump-record-layouts revealed the problem: *** Dumping AST Record Layout 0 | struct llvm::coverage::CovMapFunctionRecordV3 0 | struct llvm::coverage::accessors::FuncHashAndDataSize (base) (empty) 1 | struct llvm::coverage

[PATCH] D69471: [Coverage] Revise format to reduce binary size

rnk added a comment. In D69471#1884043 , @dexonsmith wrote: > In D69471#1883912 , @rnk wrote: > > > Everything is off-by-one because the empty bases are not zero sized. The > > MSVC record layout algorithm is just

[PATCH] D69471: [Coverage] Revise format to reduce binary size

rnk accepted this revision. rnk added a comment. Sounds like a plan, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69471/new/ https://reviews.llvm.org/D69471 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm

[PATCH] D75121: Put microsoft template parameter shadow warning behind separate flag (PR44794)

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. code lgtm Should we have a standalone test for this? Something that compiles with `clang -cc1 -Wno-microsoft -Wmicrosoft-template-shadow` and looks for the shadowing warnings? CHANGES SINCE LAST

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

rnk added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74455/new/ https://reviews.llvm.org/D74455 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin

[PATCH] D75198: [ms] Rename ParsingInlineAsm functions/variables to reflect MS-specificity.

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

[PATCH] D75215: [DebugInfo] Fix for adding "returns cxx udt" option to functions in CodeView.

rnk added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:992 getTagForRecord(RD), RDName, Ctx, DefUnit, Line, 0, Size, Align, llvm::DINode::FlagFwdDecl, Identifier); if (CGM.getCodeGenOpts().DebugFwdTemplateParams) One alternative

[PATCH] D65543: [Windows] Autolink with basenames and add libdir to libpath

rnk added a comment. I ran into this issue again, and would like to move forward with this. I'll rebase it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65543/new/ https://reviews.llvm.org/D65543 ___

[PATCH] D75279: Avoid SourceManager.h include in RawCommentList.h, add missing incs

rnk created this revision. rnk added reviewers: aaron.ballman, hans. Herald added a reviewer: teemperor. Herald added subscribers: lldb-commits, arphaman. Herald added projects: clang, LLDB. SourceManager.h includes FileManager.h, which is expensive due to dependencies on LLVM FS headers. Remove

[PATCH] D75279: Avoid SourceManager.h include in RawCommentList.h, add missing incs

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG86565c130942: Avoid SourceManager.h include in RawCommentList.h, add missing incs (authored by rnk). Repository: rG LLV

[PATCH] D75310: [CUDA, clang-cl] Filter out unsupported arguments for device-side compilation.

rnk accepted this revision. rnk added a subscriber: hans. rnk added a comment. This revision is now accepted and ready to land. lgtm, but let us ask @hans for a second opinion. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75310/new/ https://review

[PATCH] D75215: [DebugInfo] Fix for adding "returns cxx udt" option to functions in CodeView.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm! So, presumably we still do the wrong thing in the non-method type cases. I came up with this, where we differ with MSVC: struct NonTrivial { NonTrivial(); NonTrivial(const NonTrivia

[PATCH] D75406: Avoid including FileManager.h from SourceManager.h

rnk created this revision. rnk added reviewers: arphaman, hans. Herald added subscribers: martong, usaxena95, jsji, kadircet, dexonsmith, jkorous, kbarton, nemanjai. Herald added a project: clang. Most clients of SourceManager.h need to do things like turning source locations into file & line num

[PATCH] D75406: Avoid including FileManager.h from SourceManager.h

rnk updated this revision to Diff 247479. rnk added a comment. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. - fixes on Linux Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75406/new/ https://reviews.llvm.org/D75406 Files:

[PATCH] D75215: [DebugInfo] Fix for adding "returns cxx udt" option to functions in CodeView.

rnk added a comment. BTW, I am curious to know if this helps V8 PDB size. IIRC you said there were some PDBs in Chrome that have this problem a lot. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75215/new/ https://reviews.llvm.org/D75215 __

[PATCH] D75215: [DebugInfo] Fix for adding "returns cxx udt" option to functions in CodeView.

rnk added a comment. I think we should commit the change to clang separately from the first one you made to LLVM to handle method types. Setting flags on forward declarations is something I'd like to get additional input on before landing. Comment at: clang/lib/CodeGen/CGDebu

[PATCH] D75215: [DebugInfo] Fix for adding "returns cxx udt" option to functions in CodeView.

rnk accepted this revision. rnk added a comment. lgtm, but please remove the clang test update from this patch, since it shouldn't pass yet. Comment at: clang/test/CodeGenCXX/debug-info-flag-nontrivial.cpp:1 +// RUN: %clang_cc1 -emit-llvm -gcodeview -debug-info-kind=limited %s

[PATCH] D75215: [DebugInfo] Fix for adding "returns cxx udt" option to functions in CodeView.

rnk added inline comments. Comment at: llvm/include/llvm/IR/DebugInfoFlags.def:61 HANDLE_DI_FLAG((1 << 29), AllCallsDescribed) +HANDLE_DI_FLAG((1 << 30), CxxReturnUdt) aprantl wrote: > dblaikie wrote: > > rnk wrote: > > > @dblaikie @aprantl, does this seem lik

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I'm busy and I haven't looked at the code in detail, but I'm OK with going back to the old way of doing things. I think honoring user requests to use more threads than cores is an important use case

[PATCH] D75643: [Sema] Don't emit pointer to int cast warnings under -Wmicrosoft-cast

rnk added a subscriber: rsmith. rnk added a comment. Looking at the review, I think @rsmith requested this -Wmicrosoft-cast behavior here: https://reviews.llvm.org/D72231#1857660 This change looks good to me, but I wanted to get his input before we undo his request. Repository: rG LLVM Gith

[PATCH] D75153: [ThinLTO] Allow usage of all SMT threads in the system

rnk added a comment. I think making the build system responsible for running the backend actions is the ideal solution, actually. The main reason we have all this threading logic in the linker is to make it easy for users of traditional build systems to use ThinLTO with a few flag flips. We hav

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

rnk added inline comments. Comment at: clang/include/clang/AST/MangleNumberingContext.h:57-58 + + /// Has device mangle numbering context. + virtual bool hasDeviceMangleNumberingContext() const { return false; } + It would be nicer if there were a single entry

[PATCH] D69582: Let clang driver support parallel jobs

rnk added subscribers: aganea, amccarth, rnk. rnk added a comment. +@aganea @amccarth Users have been asking for /MP support in clang-cl for a while, which is basically this. Is there anything in JobScheduler that could reasonably be moved down to llvm/lib/Support? I would also like to be able

[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] D69626: Fix Microsoft compatibility handling of commas in nested macro expansions.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Based on the comments it looks like you confirmed this matches MSVC's behavior, at least in this regard. I haven't stared at this deeply to think of all the corner cases, though. To Nico's po

[PATCH] D69760: [Clang][Driver] Don't pun -fuse-ld=lld as -fuse-ld=lld-link with msvc

rnk added a comment. > I think a better distinguisher would be whether clang was invoked as > clang[++] or clang-cl (i.e. driver mode). I think Martin said most of what I wanted to say. Making this dependent on the driver mode seems reasonable and would be the easier way forward. If you still

[PATCH] D69763: [Clang][Test]: Remaining "lld-link2" -> "lld-link"

rnk accepted this revision. rnk added a comment. In D69763#1733382 , @Ericson2314 wrote: > I am curious, how did this work since there is no longer an `lld-link2`? Were > these tests failing or not being run? These tests don't actually need to run the l

[PATCH] D69766: [Clang][MSVC] Use GetLinkerPath like the other toolchains for consistency

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

[PATCH] D69958: Improve VFS compatibility on Windows

rnk added a comment. This looks good to me. I had one suggestion, and I would also like to hear from another reviewer who has more ownership over VFS. Comment at: llvm/include/llvm/Support/VirtualFileSystem.h:657 + // slashes to match backslashes (and vice versa). + bool pat

[PATCH] D69950: Reapply "Fix crash on switch conditions of non-integer types in templates"

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I don't have experience with clang-tidy tests, but I think these two small, test-only changes are small enough that you can go ahead and commit them without review from a clang-tidy owner. lgtm CH

[PATCH] D69969: [SEH] Defer checking filter expression types until instantiaton

rnk created this revision. rnk added a reviewer: amccarth. Herald added a project: clang. While here, wordsmith the error a bit. Now clang says: error: filter expression has non-integral type 'Foo' Fixes PR43779 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D69969 Files:

[PATCH] D69969: [SEH] Defer checking filter expression types until instantiaton

This revision was automatically updated to reflect the committed changes. Closed by commit rG7177ce978e8f: [SEH] Defer checking filter expression types until instantiaton (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69969/new/ ht

[PATCH] D70101: [X86] Fix the implementation of __readcr3/__writecr3 to work in 64-bit mode

rnk added a comment. What do you think of using `__INTPTR_TYPE__` for this instead? If that doesn't work, we could define and undefine our own macro to use as a typedef. The resulting code should be clean enough that we can generalize it to `__readcr0` etc. It looks like `long` is the correct

[PATCH] D69959: [C-index] Fix test when using Debug target & MSVC STL

rnk added a comment. I see. You know, these things would be nicely handled if we just had a reliable asynch SEH implementation. >_> What do you think of making `FrontendOptions::Inputs` be a `SmallVector<*, 0>`? It's janky and inconsistent with the rest of that struct, but it seems less invasi

[PATCH] D69766: [Clang][MSVC] Use GetLinkerPath like the other toolchains for consistency

rnk added a comment. In D69766#1739060 , @Ericson2314 wrote: > Reading https://llvm.org/docs/DeveloperPolicy.html it looks like I need to > submit an patch email? Is that true, or can/will someone with perms see the > approval and eventually merge it fro

[PATCH] D69959: [C-index] Fix test when using Debug target & MSVC STL

rnk added a subscriber: dblaikie. rnk added a comment. For reference, this is the code sequence that needs the SmallVector change: https://github.com/llvm/llvm-project/blob/99e2cba219aea80b3f11de2aa4e0192b28852de4/clang/lib/Frontend/CompilerInvocation.cpp#L1872 Comment at: clan

[PATCH] D70196: [clang-include-fixer] Skip .rc files when finding symbols

rnk created this revision. rnk added reviewers: sammccall, bkramer. Herald added a project: clang. For some reason CMake includes entries for .rc files, but find-all-symbols handles them improperly. See PR43993 Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D70196 Files: cl

[PATCH] D70172: [CUDA][HIP] Fix assertion due to dtor check on windows

rnk added a comment. Are we sure using both Itanium and MS C++ ABIs at the same time is really the best way forward here? What are the constraints on CUDA that require the Itanium ABI? I'm sure there are real reasons you can't just use the MS ABI as is, but I'm curious what they are. Was there

[PATCH] D70196: [clang-include-fixer] Skip .rc files when finding symbols

rnk added a comment. This is a single .rc entry: { "directory": "C:/src/llvm-project/build", "command": "C:\\PROGRA~2\\WI3CF2~1\\10\\bin\\100183~1.0\\x64\\rc.exe -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_ DEPRECATE -D_CRT_SE

[PATCH] D70196: [clang-include-fixer] Skip .rc files when finding symbols

This revision was automatically updated to reflect the committed changes. Closed by commit rGe2369fd197d9: [clang-include-fixer] Skip .rc files when finding symbols (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70196/new/ https://

[PATCH] D69959: [C-index] Fix test when using Debug target & MSVC STL

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Related to this discussion, I noticed `to_vector` in STLExtras.h: https://github.com/llvm/llvm-project/blob/e2369fd197d9e/llvm/include/llvm/ADT/STLExtras.h#L1329 I guess that would be the most i

[PATCH] D70101: [X86] Fix the implementation of __readcr3/__writecr3 to work in 64-bit mode

rnk added a comment. In D70101#1741565 , @craig.topper wrote: > So in order to match the long and int behavior, I need two different macros > to replace the argument? I think __INTPTR_TYPE__ will return int on 32-bit > targets so I can't use that for th

[PATCH] D70101: [X86] Fix the implementation of __readcr3/__writecr3 to work in 64-bit mode

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/D70101/new/ https://reviews.llvm.org/D70101 ___ cfe-commits mailing list cfe-commits@lists.l

[PATCH] D70280: Remove Support/Options.h, it is unused

rnk created this revision. rnk added a reviewer: beanz. Herald added subscribers: cfe-commits, hiraditya, mgorny. Herald added a reviewer: jfb. Herald added projects: clang, LLVM. It was added in 2014 in 732e0aa9fb84f1 with one use in Scalarizer.cpp. That one use was then removed when porting to t

[PATCH] D70280: Remove Support/Options.h, it is unused

This revision was automatically updated to reflect the committed changes. Closed by commit rG631be5c0d411: Remove Support/Options.h, it is unused (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70280/new/ https://reviews.llvm.org/D7

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk created this revision. rnk added reviewers: dblaikie, hans, thakis, rsmith. Herald added a subscriber: aprantl. Herald added a project: clang. DONOTSUBMIT, this patch is just for the purpose of discussion. It turns out that the debug info describing the Sema class is an appreciable percentage

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk added a comment. In D70340#1748712 , @thakis wrote: > I don't see any reason not to do this. What's there to discuss? I'm probably > missing something obvious. I guess I was thinking about enabling this only in +asserts builds, so we pay zero overh

[PATCH] D70196: [clang-include-fixer] Skip .rc files when finding symbols

rnk added a comment. We already have llvm-rc.exe which one could plausibly use to compile rc files, and I didn't like `binary.lower().endswith('rc.exe')` as a test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70196/new/ https://reviews.llvm.org/

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk added a comment. In D70340#1749073 , @thakis wrote: > With the overhead being the cost of a single vtable with one entry? Or is > there more? I guess I worry about the extra dead vtable pointer in Sema. But, I don't think it matters. I think we sho

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk updated this revision to Diff 229943. rnk added a comment. - comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70340/new/ https://reviews.llvm.org/D70340 Files: clang/include/clang/Sema/Sema.h clang/lib/Sema/Sema.cpp Index: clang/lib

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk added a comment. In D70340#1751148 , @hans wrote: > Nice! > > Silly questions, but for my own education: I thought the key function concept > only existed in the Itanium ABI, but from your numbers it sounds like it's a > concept, at least for debug i

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk added a comment. Oh, yeah, I forgot this causes tons of -Wdelete-non-virtual-dtor warnings, so I'll have to look into that before landing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70340/new/ https://reviews.llvm.org/D70340

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk updated this revision to Diff 230130. rnk added a comment. - add final, tweak comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70340/new/ https://reviews.llvm.org/D70340 Files: clang/include/clang/Sema/Sema.h clang/lib/Sema/Sema.cpp

[PATCH] D70340: Add a key method to Sema to optimize debug info size

rnk marked an inline comment as done. rnk added inline comments. Comment at: clang/include/clang/Sema/Sema.h:335 + /// A key method to reduce duplicate type info from Sema. + virtual void anchor(); hans wrote: > I worry that this is going to look obscure to m

[PATCH] D70340: Add a key method to Sema to optimize debug info size

This revision was automatically updated to reflect the committed changes. Closed by commit rG586f65d31f32: Add a key method to Sema to optimize debug info size (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70340/new/ https://revie

<    9   10   11   12   13   14   15   16   17   18   >