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

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

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

[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

[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

[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

[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

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

[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

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

[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

[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

[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

[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

[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() ||

[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

[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

[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); }

[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

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

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

[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

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

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

[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

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

[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

[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

[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

[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: +

[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

[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

[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

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

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

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

rnk added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:1781 + + return (Ty->isIntegralOrEnumerationType() || Ty->isPointerType() || + Ty->isReferenceType()); I think we could improve readability here with some named variable

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

rnk added a comment. This seems like a new error that will be pretty easy to run into, especially in the funky MSVC virtual base case. @aaron.ballman proposed some kind of new breaking change announcement mechanism. Can you add a release note and send an announcement following that process? I

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

rnk added a comment. Thanks! > But I had > trouble figuring out how to exercise this functionality properly to add > test coverage and then compare that to MSVC itself... - I got very > confused/turned around trying to test this, so I've given up enough to > send what I have out for review, but

[PATCH] D133711: [Sema] Reject array element types whose alignments are larger than their sizes

rnk added a comment. I agree, there's no reason to change the i686 MSVC record layout algorithm. It was written, tested, done, it's fragile. We used to have some decent continuous integration testing for ABI issues like this, but as usual these things require care and feeding and it did not

[PATCH] D133711: [Sema] Reject array element types whose alignments are larger than their sizes

rnk added a comment. Usually the solution is to round up the size to the alignment. Can you provide godbolt examples of what GCC and MSVC do? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133711/new/ https://reviews.llvm.org/D133711

[PATCH] D132810: [clang][MinGW] Add `-mguard=cf` and `-mguard=cf-nochecks`

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

[PATCH] D132810: [clang][MinGW] Add `-mguard=cf` and `-mguard=cf-nochecks`

rnk added a comment. (I added @maskray based on the new code ownership structure proposed here: https://discourse.llvm.org/t/rfc-proposed-changes-to-clangs-code-ownership/64813) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132810/new/

[PATCH] D132810: [clang][MinGW] Add `-mguard=cf` and `-mguard=cf-nochecks`

rnk added a comment. +@maskray, should we use `-mguard=` as the GCC spelling of the MSVC `/GUARD:` flag? Short flags are nice, but it seems there may be a risk of GCC using `-mguard=` for something else at some point. I lean towards using `-mguard=` as is. Repository: rG LLVM Github

[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

[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

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

[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

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

[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

[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

[PATCH] D130123: Extend ptr32 support to be applied on typedef

rnk accepted this revision. rnk added a comment. In D130123#3708309 , @Ariel-Burton wrote: > - Add case to deal with ElaboratedTypes. Let's still stick with this code, but I at least feel justified raising the concern that perhaps we should look

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

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I don't have any concerns, this seems reasonable to me. Since Jan offered to test it, please wait to hear back before landing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D130123: Extend ptr32 support to be applied on typedef

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

[PATCH] D130123: Extend ptr32 support to be applied on typedef

rnk added a comment. In D130123#3698399 , @Ariel-Burton wrote: > Just to make sure that we're on the same page, you'd like to see a test that > confirms that clang is rejecting the `template void f(T __ptr32 > a)` example, and possibly one that

[PATCH] D130123: Extend ptr32 support to be applied on typedef

rnk added a comment. In D130123#3696752 , @Ariel-Burton wrote: > What is your expectation for your template code fragment? MSVC does not > accept it. Yes, I checked, MSVC rejects it, so clang should have test expectations to confirm that. It seems

[PATCH] D130123: Extend ptr32 support to be applied on typedef

rnk added a comment. That sounds reasonable to me, I confirmed that MSVC really only lets you apply these attributes directly to pointer types and to typedefs. Can you add a test for the other most common type sugar node, the template parameter? It looks like this: template void f(T

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

rnk added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:4316 + for (auto const : ClassDecl->bases()) { +QualType BT = cast(Base.getType())->getNamedType(); +const auto *BaseTemplate = dyn_cast(BT); Is this cast

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

rnk added a comment. Dmitri, do you know a good libclang point of contact for the new API? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130303/new/ https://reviews.llvm.org/D130303 ___ cfe-commits

[PATCH] D130123: Extend ptr32 support to be applied on typedef

rnk added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:7116 + for (;;) { +if (const TypedefType *TT = dyn_cast(Desugared)) { + Desugared = TT->desugar(); Ariel-Burton wrote: > rnk wrote: > > This seems like a good place to use

[PATCH] D130123: Extend ptr32 support to be applied on typedef

rnk added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:7116 + for (;;) { +if (const TypedefType *TT = dyn_cast(Desugared)) { + Desugared = TT->desugar(); This seems like a good place to use getSingleStepDesugaredType to look through all

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

rnk added subscribers: arphaman, dexonsmith. rnk added a comment. Seeking additional opinions on the PCH change: @dexonsmith @arphaman The use case makes sense to me. It seems reasonable to me that the compiler shouldn't use the same PCH file with mismatched defines on the command line.

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

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

[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") +

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

[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",

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

[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

[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

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

[PATCH] D126224: Add DWARF string debug to clang release notes.

rnk added a comment. I am reminded of the perennial problem of "optional" protobuf fields that, when omitted, will cause production crashes. Do you think it would be less disruptive to synthesize a name? I believe the string lives in a string pool, so naming all string literals `` seems like

[PATCH] D126023: [MSVC, ARM64] Add __writex18 intrinsics

rnk added a comment. I was unable to find any documentation for the meaning of AArch64 addrspace(256), and I wasn't able to figure it out after studying the code in llvm/lib/Target/AArch64 for ten minutes or so. The change seems fine, but please add some documentation as a follow-up. X86 has

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

rnk added a comment. I haven't given any input yet because I haven't yet seen a reduced example of the conforming code that is broken by this change. If someone can put together a small godbolt example that shows the issue affecting ADL, I'd appreciate it. I don't fully understand the code

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Looks good to me. I generally agree with Adrian's suggestions. Comment at: llvm/test/DebugInfo/COFF/global-no-strings.ll:1 +; RUN: llc < %s | FileCheck %s +; RUN: llc < %s

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

rnk added a comment. In D123534#3507891 , @hctim wrote: > Not sure about PDB. I did run a quick test with `gdb`, and very > unscientifically, didn't notice any difference in usability or errors between > pre- and post-this patch on a `clang` invocation

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

rnk added a comment. This seems reasonable, but I worry about the consequences of creating lots of unnamed global variables. What will gdb do with so many unnamed globals? What will the PDB linker do with all these unnamed globals? I can't answer these questions, and you're welcome to try and

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, thanks! In D124666#3490871 , @frederic-tingaud-sonarsource wrote: > By the way, in the current state of the code, there is no risk of such >

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

rnk added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:4310-4311 // If no results were found, try to correct typos. TypoCorrection Corr; MemInitializerValidatorCCC CCC(ClassDecl); rnk wrote: > We have to make sure our MS

[PATCH] D124842: [NFC][CUDA][HIP] rework mangling number for aux target

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

[PATCH] D124666: In MSVC compatibility mode, handle unqualified templated base class initialization

rnk requested changes to this revision. rnk added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:4310-4311 // If no results were found, try to correct typos. TypoCorrection Corr;

[PATCH] D124762: [WinEHPrepare] Avoid truncation of EH funclets with GNUstep ObjC runtime

rnk added a comment. Thanks! In D124762#3485478 , @sgraenitz wrote: > I guess testing must be split in two: > > - Clang wants to make sure the "funclet" bundle operand gets emitted for ObjC > ARC runtime calls on Windows. Maybe that fits into >

[PATCH] D122734: [CUDA][HIP] Fix mangling number for local struct

rnk added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:682 +/// Current name mangling is for device name in host compilation. +bool MangleDeviceNameInHostCompilation = false; + } CUDANameMangleCtx; yaxunl wrote: > rnk wrote: > > It

[PATCH] D124613: In MSVC compatibility mode, friend function declarations behave as function declarations

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm I plugged this into godbolt to confirm the behavior of MSVC: https://gcc.godbolt.org/z/v3P3WbxG3 This incompatibility has existed for a long time, right? Repository: rG LLVM Github

[PATCH] D124435: [X86] Always extend the integer parameters in callee

rnk added inline comments. Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:120 bool SRetAfterThis : 1; // isIndirect() bool InReg : 1; // isDirect() || isExtend() || isIndirect() bool CanBeFlattened: 1; // isDirect() Instead of

[PATCH] D120272: [CUDA] Add driver support for compiling CUDA with the new driver

rnk added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6223-6224 if (IsCuda || IsHIP) { -if (Args.hasFlag(options::OPT_fgpu_rdc, options::OPT_fno_gpu_rdc, false)) +if (Args.hasFlag(options::OPT_fgpu_rdc, options::OPT_fno_gpu_rdc, false) || +

[PATCH] D124032: [COFF, ARM64] Add __break intrinsic

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

[PATCH] D122734: [CUDA][HIP] Fix mangling number for local struct

rnk added inline comments. Herald added a subscriber: mattd. Comment at: clang/include/clang/AST/ASTContext.h:682 +/// Current name mangling is for device name in host compilation. +bool MangleDeviceNameInHostCompilation = false; + } CUDANameMangleCtx;

[PATCH] D123345: Treat `std::move`, `forward`, and `move_if_noexcept` as builtins.

rnk added a comment. Generally speaking, this sounds like a good idea to me. One time in 2019 I used -ftime-trace+ClangBuildAnalyzer and it told me that std::unique_ptr was the most expensive template because it is instantiated so

[PATCH] D123405: [dllexport] odr-use constexpr default args for constructor closures

rnk added a comment. Looks good to me after the fact. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123405/new/ https://reviews.llvm.org/D123405 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D122627: [HLSL] Fix MSFT Attribute parsing, add numthreads

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/include/clang/Basic/AttrDocs.td:6377 +are required. The ``X``, ``Y``, and ``Z`` values provided to the attribute +dictate the thread id. Total number of

[PATCH] D121733: Clean pathnames in FileManager.

rnk added a comment. Ignoring the ".." path components for the moment, is this patch good to go as is? It doesn't affect that behavior. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121733/new/ https://reviews.llvm.org/D121733

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