[PATCH] D135488: [codegen][WIP] Display stack layouts in console

rnk added a comment. I think I was confused in that conversation because I wasn't sure where in the pass pipeline this was going to run. I heard various statements like "lost after ISel" and to me, ISel is like the first step of codegen, it comes before register allocation. What you've done mak

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

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

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

rnk added a comment. In D136474#3883629 , @thakis wrote: > FWIW we do have precedent of deviating from cl.exe for determinism reasons. I > think having deterministic output is a good default. This is true, but the cc1 flag isn't inherently non-determini

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

rnk added a comment. In D136474#3884248 , @thakis wrote: >> This is true, but the cc1 flag isn't inherently non-deterministic. I would >> prefer to fix the determinism bugs instead of adding flags and more >> divergence. > > See https://reviews.llvm.org

[PATCH] D136474: [CodeView][clang] Disable emitting command line into CodeView by default and add flag to enable

rnk added a comment. In D136474#3888527 , @thakis wrote: > That sounds good to me, assuming that you have someone concrete in mind for > who "we" is who's removing the irrelevant flags :) But if nobody's in charge > of that at the moment, imho it makes

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

rnk added a comment. I realized I forgot some things I should've mentioned: - This probably deserves a release note, if it doesn't have one already that I missed or forgot about - We should tag #clang-vendors , since this does change ABI. You've alre

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

rnk added a comment. In D119051#3888906 , @erichkeane wrote: > I noticed in a downstream that this changes how we emit structs to IR > sometimes (see https://godbolt.org/z/74xeq9rTj for an example, but the > 'no-opaque-pointers' isn't necessary to caus

[PATCH] D136776: [clang codegen] Fix __try/__finally blocks in C++ constructors.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. From what I know, we could always build the GlobalDecl with the base ctor/dtor variant. The complete and deleting variants always delegate to the base variants, and we will only ever emit the SEH fu

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

rnk added a comment. Unless I'm missing something, I think Clang's behavior here is preferable to MSVC's. MSVC produces code which will not link. Clang turns the linker error into a compiler error, which is generally easier for the user to understand. To my knowledge, it is still true that ther

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

rnk added a subscriber: akhuang. rnk added a comment. @akhuang, can you help out with this? The address of dllimport symbols not being constexpr is a big pain point for our users too, and I'd love to fix it: https://github.com/protocolbuffers/protobuf/issues/10159 I am ready to be wrong here: if

[PATCH] D137073: [clang] Fix inline builtin functions of an __asm__ renamed function with symbol prefixes

rnk added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:5020 +std::string AsmName; +if (auto *A = FD->getAttr()) { + StringRef UserLabelPrefix = I think you can call CGM.getMangledName, but I might be wrong. It's worth doing if straightf

[PATCH] D137073: [clang] Fix inline builtin functions of an __asm__ renamed function with symbol prefixes

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

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

rnk added a subscriber: hans. rnk added a comment. Regarding the usage of isPOD in the MSVC ABI code, we should have a routine similar to `isTrivialForAArch64MSVC` that works for the other MSVC architectures that we support (x86, x64, and arm32). I don't think we should try to rely on definitio

[PATCH] D131910: [Windows] Put init_seg(compiler/lib) in llvm.global_ctors

rnk added a comment. Please update the AttrDocs.td file to document the relationship between init_seg and init_priority on Windows. Can we increase the threshold for library so that we can insert initia

[PATCH] D127233: [CodeGen] Sort llvm.global_ctors by lexical order before emission

rnk added subscribers: ayzhao, usaxena95, ilya-biryukov. rnk added a comment. I think this is a straightforward improvement. I would like to see it land. Do the other reviewers have any outstanding concerns? +cc other clang people @ayzhao @ilya-biryukov @usaxena95 Comment at:

[PATCH] D130303: Handle template parameter-dependent bit field widths in libclang

rnk added a subscriber: dexonsmith. rnk added a comment. Pinging alternative reviewer +@dexonsmith for a libclang API addition Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130303/new/ https://reviews.llvm.org/D130303 _

[PATCH] D131910: [Windows] Put init_seg(compiler/lib) in llvm.global_ctors

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

[PATCH] D130709: MSVC compatibility mode: fix error on unqualified templated base class initialization in case of partial specialization

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Yes, looks good to me, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130709/new/ https://reviews.llvm.org/D130709 ___ cfe-commits mailin

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

rnk added a subscriber: ayzhao. rnk added a comment. In D119051#3747201 , @dblaikie wrote: > So... my conclusion is that Clang's AArch64 appears to be correct for x86 as > well, and we should just rename the function and use it unconditionally, > remov

[PATCH] D132661: [clang] Make guard(nocf) attribute available only for Windows

rnk added inline comments. Comment at: clang/include/clang/Basic/Attr.td:373-375 // Specifies Operating Systems for which the target applies, based off the // OSType enumeration in Triple.h list OSes; Do we need customcode? Can we not use the OS list her

[PATCH] D132661: [clang] Make guard(nocf) attribute available only for Windows

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

[PATCH] D133920: [X86][fastcall][vectorcall] Move capability check before free register update

rnk added a comment. Thanks, I have more comments, sorry I didn't add them on the first review. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1785 - if (State.CC == llvm::CallingConv::X86_FastCall || - State.CC == llvm::CallingConv::X86_VectorCall || - State.CC == l

[PATCH] D133711: [Sema] Reject array element types whose sizes aren't a multiple of their alignments

rnk accepted this revision. rnk added a comment. Sound like a good plan. 👍 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133711/new/ https://reviews.llvm.org/D133711 ___ cfe-commits mailing list cfe-comm

[PATCH] D133920: [X86][fastcall][vectorcall] Move capability check before free register update

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

[PATCH] D131465: C++/ObjC++: switch to gnu++17 as the default standard

rnk added a comment. Right, last time I think MSVC moved first to C++14. I think it's OK for Clang to be the first mover to C++17. Ultimately, users have many options if they run into breakage: pass `/std:c++14`, if you are a vendor (Microsoft), reconfigure with `CLANG_DEFAULT_STD_CXX`. Maybe o

[PATCH] D134468: [Driver] Drop MSVC<2015 tweaking

rnk added a comment. I don't see the MSVC build requirement as being related to the version of MSVC that clang-cl supports targeting. Those can be separate. That said, this might be the right time to sunset this compatibility logic. If we do this, I'd like it to be a policy change. We should ad

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

rnk added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:815 // Prefer the PGO based weights over the likelihood attribute. // When the build isn't optimized the metadata isn't used, so don't generate I lean towards implementing the intended beha

[PATCH] D132379: [Support] Class for response file expansion

rnk added a comment. This makes sense to me. This probably affects nobody, but this reminds me of my first LLVM change: https://github.com/llvm/llvm-project/commit/fc8a2d5a8390952029e1c47a623e046b744f44d4 Comment at: llvm/include/llvm/Support/CommandLine.h:2098 +public: + Ex

[PATCH] D134468: [Driver] Ignore -fmsc-version= -fms-compatibility-version= values smaller than 19

rnk added a subscriber: aaron.ballman. rnk added a comment. There are a lot more hits for `isCompatibleWithMSVC` used with 2015 across the codebase. Do you want to handle them in a follow up? That's fine with me. I would like to get a second opinion before doing this. +@aaron.ballman Repositor

[PATCH] D131465: C++/ObjC++: switch to gnu++17 as the default standard

rnk added a comment. In D131465#3812387 , @aaron.ballman wrote: > Does `clang-cl` set the `-fms-compatibility` version? If so, then it seems > like we'd probably want to key it off `-fms-compatibility`? I think both driver share the logic for MSVC vers

[PATCH] D134441: [ObjC][ARC] Fix target register for call expanded from CALL_RVMARKER on Windows

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

[PATCH] D132379: [Support] Class for response file expansion

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

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

rnk added a comment. In D134456#3818952 , @aaron.ballman wrote: > I think that would be reasonable to at least consider; the default behavior > can be "the attribute wins" and this gives users an escape hatch to say "no, > I trust PGO more, let that wi

[PATCH] D134456: [PGO] Consider parent context when weighing branches with likelyhood.

rnk added a comment. In D134456#3819042 , @aaron.ballman wrote: > Alternatively, we could document that we don't follow the intent of the > standard feature and that's just the way things are, and add a command line > option to honor that intent. Howev

[PATCH] D134688: MSVC AArch64 ABI: Homogeneous aggregates

rnk added a comment. Nice! Mostly comment copy edits. Comment at: clang/lib/CodeGen/MicrosoftCXXABI.cpp:4478-4480 // MSVC Windows on Arm64 considers a type not HFA if it is not an // aggregate according to the C++14 spec. This is not consistent with the // AAPCS64, but

[PATCH] D134797: [X86][vectorcall] Make floating-type passed by value to match with MSVC

rnk added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1858-1859 } -return getIndirectResult(Ty, /*ByVal=*/false, State); +bool ByVal = IsVectorCall && Ty->isFloatingType(); +return getIndirectResult(Ty, ByVal, State); } I wou

[PATCH] D135027: [Clang][MinGW][cygwin] Fix __declspec with -fdeclspec enabled

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

[PATCH] D134688: MSVC AArch64 ABI: Homogeneous aggregates

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

[PATCH] D133817: MSVC ABI: Looks like even non-aarch64 uses the MSVC/14 definition for pod/aggregate passing

rnk added a comment. lgtm In D133817#3794429 , @dblaikie wrote: > So, I could leave a test case and some comments, or follow-up with a separate > fix for the homogenous aggregate issue? I guess it comes down to writing that > separate function to do th

[PATCH] D134797: [X86][vectorcall] Make floating-type passed by value to match with MSVC

rnk added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1858-1859 } -return getIndirectResult(Ty, /*ByVal=*/false, State); +bool ByVal = IsVectorCall && Ty->isFloatingType(); +return getIndirectResult(Ty, ByVal, State); } pengf

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

rnk added a comment. Relatedly, if we put this POD behavior query on TargetInfo, I wonder if it makes sense to move the tail padding predicate from TargetCXXABI to TargetInfo: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/TargetCXXABI.h#L282 The switch there is basical

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

rnk added inline comments. Comment at: clang/include/clang/Basic/TargetInfo.h:953 + }; + virtual TailPaddingUseRules getTailPaddingUseRules() const { +return UseTailPaddingUnlessPOD03; The virtual method works, but it does seem to have caused a behavior cha

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

rnk added inline comments. Comment at: clang/lib/AST/DeclCXX.cpp:774-775 +if ((!Constructor->isDeleted() && !Constructor->isDefaulted()) || +(getLangOpts().getClangABICompat() <= + LangOptions::ClangABI::Ver15 || Target.isPS() || Target.isOSDarwin

[PATCH] D135326: Half-done attempt to move tail padding callback from TargetCXXABI to TargetInfo

rnk added a comment. I think in the end, perhaps it is not worth disturbing this code. Comment at: clang/lib/Basic/Targets/OSTargets.h:169 +if (T.getOS() == llvm::Triple::WatchOS || +this->getCXXABI().getKind() == TargetCXXABI::AppleARM64) + return TargetInfo::

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

rnk added a comment. +#clang-vendors for ABI changes Comment at: clang/include/clang/Basic/TargetInfo.h:1569 + virtual bool areDefaultedSMFStillPOD(const LangOptions&) const; + Please add a doc comment for this,

[PATCH] D135551: [clang] replace `assert(0)` with `llvm_unreachable` NFC

rnk added a comment. I think the disagreement here highlights the need to have a serious discussion about the future of error handling across the LLVM project. As you say, it sounds like you're not going to reach agreement on this code review, so maybe the best short term next step is to land t

[PATCH] D136474: [CodeView][clang] Add flag to disable emitting command line into CodeView

rnk accepted this revision. rnk added a comment. I'm happy with this if it works for everyone else. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136474/new/ https://reviews.llvm.org/D136474 ___ cfe-comm

[PATCH] D136998: Try to implement lambdas with inalloca parameters by inlining the call operator function.

rnk added inline comments. Comment at: clang/lib/CodeGen/CGClass.cpp:3064 +if (I.getName().equals("this")) { + VMap[&I] = llvm::PoisonValue::get(I.getType()); +} else { Let's use getNullValue instead, just to avoid any complications for msan or ubsa

[PATCH] D136998: Try to implement lambdas with inalloca parameters by inlining the call operator function.

rnk added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3590-3593 + getTarget().getCXXABI().isMicrosoft() && + llvm::any_of(cast(FD)->parameters(), [&](ParmVarDecl *P) { +return isInAllocaArgument(getCXXABI(), P->getType()); +

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

rnk added a comment. In D137107#3905443 , @zahiraam wrote: > extern int __declspec(dllimport) next(int n); > int main () { > extern int _declspec(dllimport) val; > constexpr int& val_ref = val; > int i = next(val_ref); > return i; >

[PATCH] D136998: Try to implement lambdas with inalloca parameters by inlining the call operator function.

rnk added a subscriber: efriedma. rnk added a comment. +@efriedma, can you review this as a Clang CodeGen owner, according to the recently updated CodeOwners.rst file? https://github.com/llvm/llvm-project/blob/main/clang/CodeOwners.rst#clang-llvm-ir-generation Comment at: clan

[PATCH] D136998: Try to implement lambdas with inalloca parameters by inlining the call operator function.

rnk added a comment. In D136998#3906874 , @efriedma wrote: > Should we try to use this codepath for variadic lambdas as well? Yes! > Do we want to try to unify our cloning code? > CodeGenFunction::GenerateVarArgsThunk has code doing something similar.

[PATCH] D137806: [AST] Fix class layout when using external layout under MS ABI.

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

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

rnk added subscribers: efriedma, hans. rnk added a comment. Sorry for the delay, dev meeting. I think we should try to add some reviewers for IR gen to help, I don't have a lot of time to be responsive: https://github.com/llvm/llvm-project/blob/main/clang/CodeOwners.rst#clang-llvm-ir-generation +

[PATCH] D138122: Lift EHPersonalities from Analysis to IR (NFC)

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Sounds good to me. I think we just put it in Analysis in an attempt to keep IR small and focused, and particularly to avoid putting more target-specific logic in IR. Repository: rG LLVM Github M

[PATCH] D137939: [CGObjC] Open cleanup scope before SaveAndRestore CurrentFuncletPad and push CatchRetScope early

rnk added a comment. Thanks, I can see the bug here: https://gcc.godbolt.org/z/1xjMYarT9 You can see how storeStrong cleanup uses the catchpad funclet when it should not. Comment at: clang/test/CodeGenObjCXX/arc-exceptions-seh.mm:36 +// CHECK: call +// CHECK:

[PATCH] D137939: [CGObjC] Open cleanup scope before SaveAndRestore CurrentFuncletPad and push CatchRetScope early

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

[PATCH] D127641: [clang-cl][MSVC] Enable /Zc:alignedNew for C++17 and /Zc:sizedDealloc by default

rnk added a subscriber: luken-google. rnk added a comment. +@luken-google Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6497 + options::OPT_fno_aligned_allocation) && +LanguageStandard == "-std=c++17") + CmdArgs.push_back("-faligned-allo

[PATCH] D126676: [clang] Disallow differences in defines used for creating and using PCH

rnk added a comment. I don't have a great answer here, but yes, dllexport macro norms sort of run directly counter to the normal ways that people use PCH. It seems like projects will need to have a library module / PCH file for building a DLL and then a PCH / module for consuming the DLL. Rep

[PATCH] D127259: [CodeGen] guarantee templated static variables are initialized in the reverse instantiation order

rnk added a subscriber: aaron.ballman. rnk added a comment. I think Richard had some concerns in the other review that this may not be enough to really guarantee initialization order within the TU. I couldn't say either way, I shouldn't review this code. Conceptually, this change seems small en

[PATCH] D128406: clang: Tweak behaviour of warn_empty_while_body and warn_empty_if_body

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

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

rnk added a subscriber: alexander-shaposhnikov. rnk added a comment. I'm somewhat supportive of the goal here, but I think there are still some underlying issues. First, why should these guarantees be limited to instantiations and not inline variables? Such as: int f(); inline int gv1 = f(

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

rnk added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:563 + } else if (IsInstantiation || getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR || D->hasAttr()) { @rsmith , if inline global variable initializ

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

rnk added inline comments. Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:563 + } else if (IsInstantiation || getContext().GetGVALinkageForVariable(D) == GVA_DiscardableODR || D->hasAttr()) { rsmith wrote: > rnk wrote: > > @rsmith , if i

[PATCH] D126559: [MSVC] Fix pragma alloc_text failing for C files

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

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

rnk added a comment. I think it's fine to implement this, but I wanted to share some of the motivation for the current state of things. In our experience, the majority of uses of pragma optimize were to work around MSVC compiler bugs. So, instead of honoring them, we felt it was best to ignore

[PATCH] D125723: [MSVC] Add support for MSVC pragma optimize

rnk added a comment. In D125723#3553664 , @aaron.ballman wrote: > In D125723#3550956 , @steplong > wrote: > >> Appreciate the help! It's not clear to me how to go from the strings "Os", >> "foo1", "foo2", "foo3

[PATCH] D126412: Fix a buglet in remove_dots().

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, sorry for the delay, do you need someone to push this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126412/new/ https://reviews.llvm.org/D12641

[PATCH] D126412: Fix a buglet in remove_dots().

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG35ab2a11bb55: Fix a buglet in remove_dots(). (authored by ppluzhnikov, committed by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D126341: Order implicitly instantiated global variable's initializer by the reverse instantiation order

rnk added a comment. Well, I guess we're out of luck, but that seems like a very poorly considered requirement from the standard. If we can't use comdats for inline variables, every time you include a header with a dynamically initialized variable, it will generate extra initialization code in

[PATCH] D159351: [Sema] Change order of displayed overloads in diagnostics

rnk added a subscriber: cjdb. rnk added a comment. +@cjdb , do you have time to review this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159351/new/ https://reviews.llvm.org/D159351 ___ cfe-commits mai

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

rnk closed this revision. rnk added a subscriber: MaskRay. rnk added a comment. Closing, we left the test alone, it still uses `-fuse-ld=lld-link2`. Perhaps in the future we should reconsider this, but that's how things stand now, and we aren't going to land this patch as is. + @MaskRay , who ha

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

rnk closed this revision. rnk added a comment. Let's close it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69766/new/ https://reviews.llvm.org/D69766 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

rnk added a subscriber: akhuang. rnk added a comment. Regarding upgrade-datalayout2.ll, I don't think we need to be too constrained by it. @akhuang , do you recall why you added it? In other words, I think your direction is reasonable, we should go forward with this. Repository: rG LLVM Git

[PATCH] D155713: [clang] Fix interaction between dllimport and exclude_from_explicit_instantiation

rnk added a comment. Thanks for the patch! The other similar functionality that exists is `/Zc:dllexportInlines-`, which we could track down the implementation of to try to share logic, but I'm not sure about that. I should defer to @hans for a more thorough code review. Co

[PATCH] D156143: Add Adrian and David as owners for debug info

rnk created this revision. rnk added reviewers: aaron.ballman, aprantl, dblaikie. Herald added a project: All. rnk requested review of this revision. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D156143 Files: clang/CodeOwners.rst Index: clang

[PATCH] D156143: Add Adrian and David as owners for debug info

rnk updated this revision to Diff 543618. rnk added a comment. - Fix escaping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156143/new/ https://reviews.llvm.org/D156143 Files: clang/CodeOwners.rst Index: clang/CodeOwners.rst ==

[PATCH] D156143: Add Adrian and David as owners for debug info

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG47d178939e96: Add Adrian and David as owners for debug info (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[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/CGClass.cpp:3095 + StringRef CallOpName = CallOpFn->getName(); + std::string ImplName = ("__impl" + CallOpName).str(); + akhuang wrote: > The old code tried to find the "" part of the function name and >

[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/CGClass.cpp:3095 + StringRef CallOpName = CallOpFn->getName(); + std::string ImplName = ("__impl" + CallOpName).str(); + akhuang wrote: > rnk wrote: > > akhuang wrote: > > > The old code tried to find the

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

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/lib/CodeGen/CGClass.cpp:3094 + // reason the name doesn't look as expected then just tack __impl to the + // front. + StringRef CallOpName = CallOpFn->get

[PATCH] D157566: [SEH] fix assertion when -fasy-exceptions is used.

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

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

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Thanks! My concerns are addressed, but please confirm that Eli's are too. Comment at: clang/lib/Sema/SemaDecl.cpp:14254 int SectionFlags = ASTContext::PSF_Read; -if (var->

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

rnk added a comment. What are the next steps here? Have concerns been addressed? The CI failures seem unrelated (libcxx tests is an AIX bot out of disk, clang CI is an interpreter test that seems unrelated, but please confirm). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D150646: [clang][X86] Add __cpuidex function to cpuid.h

rnk added a comment. I think MSVC defines `_MSC_EXTENSIONS` under one of its compatibility modes as well, so it's not a good indicator of "is this compiler MSVC-like". I think we should be exposing the `__cpudex` builtin any time we are being MSVC-like, not under `fms-compatibility`. Would that

[PATCH] D157794: [Driver] Make /Zi and /Z7 aliases of -g rather than handling them specially

rnk added a subscriber: smeenai. rnk added inline comments. Comment at: clang/test/Driver/cl-options.c:567 -// This test was super sneaky: "/Z7" means "line-tables", but "-gdwarf" occurs -// later on the command line, so it should win. Interestingly the cc1 arguments -// came o

[PATCH] D157794: [Driver] Make /Zi and /Z7 aliases of -g rather than handling them specially

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. So, as I understand the discussion, after this patch, it will still be possible to get dwarf and codeview in the same compile, but users will have to resort to cc1 flags. I think that's a good end

[PATCH] D155713: [clang] Fix interaction between dllimport and exclude_from_explicit_instantiation

rnk added a comment. > That doesn't handle the second of your test cases though, where dllimport is > put on the member directly: > ... > It's not clear to me how we'd want to handle that. I don't think it comes up > in libc++, and I can't think of any code that would want to do that either, >

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

rnk added a comment. I don't think this is the right thing to do, I think I recall saying as much on some other review. The MSVC docs say that `/Za`/`/Ze` control `_MSC_EXTENSION`, and as I understand it, `/Za`

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

rnk added a comment. So, I'm strongly in favor of the feature, but I want to get feedback on the name. The major pro is that it maps directly to LLVM IR: the attribute does exactly what it says. The major con is that the name isn't really related to what the user wants to do, which is to merge

[PATCH] D157334: [clang] Define _MSC_EXTENSIONS on -gnu if -fms-extensions is set

rnk added a comment. > Also assuming that /Ze is similar to our -fms-extensions which I believe is > the case, but not completely sure I believe that /Ze has more in common with `-fms-compatibility` than `-fms-extensions`, but I could be wrong. Also, they may just be completely different thing

[PATCH] D61670: [clang] [MinGW] Add the option -fno-autoimport

rnk accepted this revision. rnk added subscribers: aeubanks, jyknight. rnk added a comment. This revision is now accepted and ready to land. lgtm cc +@aeubanks @jyknight to consider using the code model for this purpose Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D158233: [SEH] Fix wrong argument passes to the call of OutlinedFinally.

rnk added inline comments. Comment at: clang/lib/CodeGen/CGCleanup.cpp:882 if (!Scope.hasBranchThroughs() && !HasFixups && !HasFallthrough && - Scope.getNumBranchAfters() == 1) { + !getLangOpts().EHAsynch && Scope.getNumBranchAfters() == 1) { ass

[PATCH] D158233: [SEH] Fix wrong argument passes to the call of OutlinedFinally.

rnk added inline comments. Comment at: clang/lib/CodeGen/CGCleanup.cpp:882 if (!Scope.hasBranchThroughs() && !HasFixups && !HasFallthrough && - Scope.getNumBranchAfters() == 1) { + !getLangOpts().EHAsynch && Scope.getNumBranchAfters() == 1) { ass

[PATCH] D61670: [clang] [MinGW] Add the option -fno-autoimport

rnk added a comment. In D61670#4604486 , @mstorsjo wrote: > In D61670#4604145 , @rnk wrote: > >> cc +@aeubanks @jyknight to consider using the code model for this purpose > > Hmm, I don't quite understand this comme

[PATCH] D158233: [SEH] Fix wrong argument passes to the call of OutlinedFinally.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, thanks! Comment at: clang/lib/CodeGen/CGCleanup.cpp:881 + // to indicate abnormal termination) + const FunctionDecl *FD = dyn_cast_or_null(CurFuncDecl); if (

[PATCH] D153156: [Clang] CWG1473: do not err on the lack of space after operator""

rnk added a comment. I went ahead and pushed a revert of this, since this change was pretty disruptive, at least in our experience. I could be convinced that it's worth sunsetting this extension for accepting C-style format string macro code, but that doesn't seem like the original intent of th

[PATCH] D158346: [clang-tidy] readability-container-size-empty - detect missing usage of .empty() on string_literals

rnk added inline comments. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability/container-size-empty.cpp:26 +namespace string_literals{ +string operator""s(const char *, size_t); +} I discovered that this test started to fail when I landed my rever

[PATCH] D86310: [X86] Align i128 to 16 bytes in x86 datalayouts

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I re-read the code review, and I think most folks are in favor of this change, but I may have missed some. Many concerns were raised, so please wait for approval from @efriedma as well before landin

[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) I think this is not compatible with MSVC. MSVC uses simple logic, it does

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