[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

[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

[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

[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

[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

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

[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

[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

[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

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

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

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

[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

[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

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

[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

[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

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

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

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

[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

[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

[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

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

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

[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

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

[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

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

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

[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

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

[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

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:

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

[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

[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

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

[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

[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:5103 -if (Addr.getAlignment() < Align && +if (CallInfo.isDelegateCall()) { + NeedCopy = false; akhuang wrote: > I think the problem is that it tries to do a copy here

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

rnk added a comment. Can you please add a test case for the issue that caused the revert? I wanted to dig into that to try to understand why the extra copy was being emitted. I think you mentioned it has something to do with increasing the alignment. Repository: rG LLVM Github Monorepo

[PATCH] D153898: [DebugInfo] Enable debug info emission for extern variables in C++

rnk accepted this revision. rnk added subscribers: MaskRay, dblaikie. rnk added a comment. This revision is now accepted and ready to land. This seems reasonable to me, but I want to get a second opinion from @dblaikie and @maskray for some debug info and BPF perspective. Repository: rG LLVM

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

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

[PATCH] D152986: [clang] Allow 'nomerge' attribute for function pointers

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. In D152986#4426256 , @eddyz87 wrote: > In D152986#4425736 , @rnk wrote: > >> The purpose of the attribute is

[PATCH] D153179: [clang codegen] Fix ABI for HVA returns on AArch64 MSVC.

rnk added a comment. The other thing I'm getting at here is that our TargetInfo.cpp abstraction is pretty well built out, even if it's a huge mess. The C++ rules are generally platform neutral, and we don't usually have to resort to these kinds of triple checks, but sometimes it's the most

[PATCH] D153179: [clang codegen] Fix ABI for HVA returns on AArch64 MSVC.

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

[PATCH] D153179: [clang codegen] Fix ABI for HVA returns on AArch64 MSVC.

rnk added a comment. Interesting. In Clang, we basically layer the C++ rules over the C rules: if C++ aspects of a class allow it to be passed directly transparently, then we defer down to the C rules, which deal with HVAs, structs, and things like that. In D153179#4429813

[PATCH] D137872: Implement lambdas with inalloca parameters by forwarding to function without inalloca calling convention.

rnk added a comment. I'm really happy to see this limitation finally addressed! It's quite a bit of embarrassing tech debt that affects folks making Windows x86 builds. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137872/new/

[PATCH] D152986: [clang] Allow 'nomerge' attribute for function pointers

rnk added a comment. The purpose of the attribute is really limited to preserving source location information on instructions, and this isn't really a supported usage. The BPF backend and verifier needs to learn to tolerate valid LLVM transforms if it wants to be a real LLVM backend. Of

[PATCH] D152472: [Clang][MS] Remove assertion on BaseOffset can't be smaller than Size.

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

[PATCH] D152472: [Clang][MS] Remove assertion on BaseOffset can't be smaller than Size.

rnk added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:3727 +SmallVector Bases; +for (auto Base: Info.CXXInfo->BaseOffsets) { + Bases.push_back(Base.second.getQuantity()); Rather than iterating the map and sorting afterwards,

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

rnk added a comment. In D152604#4415403 , @MaskRay wrote: > I have thought about not applying `-fsanitize-address-globals-dead-stripping` > in `-fno-data-sections` mode, so that we won't have sections like `.bss.a`, > but the driver

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG651e5ae62d29: [MS] Fix passing aligned records by value in some cases (authored by rnk). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

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

rnk added a comment. I think there's a fair bit more cleanup and simplification to be done, see asanUsesGlobalsGC and the comments there. We could check CGOpts.DataSections right there, for example, and

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

rnk created this revision. rnk added reviewers: nikic, hans. Herald added subscribers: StephenFan, pengfei. Herald added a project: All. rnk requested review of this revision. Herald added a project: clang. It's not exactly clear what the meaning of TypeInfo::AlignRequirement is, so go directly

[PATCH] D152472: [Clang][MS] Remove assertion on BaseOffset can't be smaller than Size.

rnk added a comment. Please do extend the simple record layout dumping format. I think it should be relatively simple to add string dumping and parsing to clang/lib/Frontend/LayoutOverrideSource.cpp. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D152017: [DebugInfo] Add flag to only emit referenced member functions

rnk added a comment. In D152017#4401075 , @dblaikie wrote: > Yeah, mixed feelings about a default-on flag, so you have to use `-gno-*`. > Though one name in that form, might be `-gno-canonical-types`. > (pedantically C++ calls these things "member

[PATCH] D152017: [DebugInfo] Add flag to only emit referenced member functions

rnk added a comment. I tend to think that enabled-by-default options which have names that you are supposed to prefix with no are awkward[1], but the compiler ecosystem (GCC & Clang) has lots of those, and perhaps we should lean into it. I like `-g[no-]undefined-methods` the best of Paul's

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

rnk added subscribers: thieta, mstorsjo. rnk added a comment. + @mstorsjo @thieta Since we were talking about the complexity involved in choosing the correct ASan runtime, there is actually even more involved. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D152017: [DebugInfo] Add flag to only emit referenced member functions

rnk added a comment. This is pretty cool, even if it's just for data gathering. It's interesting that 29% of .debug_info and .debug_str is for describing class methods, that's pretty significant, even if it's only a 13% reduction across all debug info sections. Repository: rG LLVM Github

[PATCH] D151515: [CodeGen] add additional cast when checking call arguments

rnk added subscribers: efriedma, rjmccall, rnk. rnk added inline comments. Comment at: clang/lib/CodeGen/CGCall.cpp:4376 +.getTypePtr() +->getPointeeOrArrayElementType(); + const Type *CanonicalArgTy =

[PATCH] D151393: [CodeGen] Make __clang_call_terminate have an unwind table entry

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Makes sense to me too, but wait for approval by @efriedma Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151393/new/ https://reviews.llvm.org/D151393

[PATCH] D150817: Use windows baskslash on anonymous tag locations if using MSVCFormatting and it's not absolute path.

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

[PATCH] D150875: Make dereferencing a void* a hard-error instead of warn-as-error

rnk added a comment. In D150875#4353398 , @aaron.ballman wrote: > Note, there was an RFC: > https://discourse.llvm.org/t/rfc-can-we-stop-the-extension-to-allow-dereferencing-void-in-c/65708 Thanks, that addresses my concern. Please link to it from the

[PATCH] D150875: Make dereferencing a void* a hard-error instead of warn-as-error

rnk added a comment. Can you expand on the motivation for removing this extension? This doesn't seem to save a lot of code complexity, and it increases migration costs for our users. I'd like to have some motivation for putting them through the trouble of migrating. CHANGES SINCE LAST ACTION

[PATCH] D124642: [WIP] Add support for return from an SEH __finally block.

rnk added a comment. The approach makes sense to me, but can you include test cases to illustrate the IR and code that gets generated? Comment at: clang/lib/CodeGen/CGException.cpp:1833 +/// Find all local variable captures in the statement. +struct ReturnStmtFinder :

[PATCH] D150645: [Driver] Support multi /guard: options

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

[PATCH] D149997: [clang] [test] Narrow down MSVC specific behaviours from "any windows" to only MSVC/clang-cl

rnk added a comment. In D149997#4337548 , @mstorsjo wrote: > So, the reason why this failed, is that when invoked as `%clang_cc1` in a > MSVC/clang-cl style build, `_MSC_VER` isn't predefined, while `_WIN32` is. > When invoked via the Clang driver

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

rnk edited reviewers, added: hans, efriedma; removed: majnemer, rnk, aeubanks. rnk added a comment. Thanks for working on this, this is also an issue for our users. I've been out on leave. I replaced myself as a reviewer with Hans. CHANGES SINCE LAST ACTION

[PATCH] D139749: Headers: use C++ inline semantics in C++ mode

rnk added a subscriber: hans. rnk added a comment. This feels to me like we are still working around some incompatibilities between the MSVC intrin.h / intrin0.h model. I would prefer it if we could always use `static inline` consistently in our intrinsic headers so we don't have to worry

[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

[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

[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

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

[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

[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

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

[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] 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 = BlockColors.find(BB)->second; +assert(CV.size() == 1 && "non-unique color for block!"); +BasicBlock

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

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

[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

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

[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 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] 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[] = llvm::PoisonValue::get(I.getType()); +} else { Let's use getNullValue instead, just to avoid any complications for msan or

  1   2   3   4   5   6   7   8   9   10   >