[PATCH] D101479: [Driver] Support libc++ in MSVC

rnk added a comment. The test doesn't seem to pass on Jenkins. Maybe we need a REQUIRES line or a fake sysroot or something for the test. Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:1332 + + auto AddLibcxxIncludePath = [&](StringRef Path) { +std::string Version =

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. In D100776#2703273 , @echristo wrote: > As is mentioned there are tradeoffs around this though: a) it does make it > harder to have clang generate code

[PATCH] D77598: Integral template argument suffix and cast printing

rnk added inline comments. Comment at: clang/lib/AST/TemplateBase.cpp:71-72 if (T->isBooleanType() && !Policy.MSVCFormatting) { Out << (Val.getBoolValue() ? "true" : "false"); } else if (T->isCharType()) { reikdas wrote: > rnk wrote: > > rsmith

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

rnk added a subscriber: akhuang. rnk added inline comments. Comment at: clang/include/clang/Sema/Sema.h:335 + /// A key method to reduce duplicate type info from Sema. + virtual void anchor(); erichkeane wrote: > rnk wrote: > > hans wrote: > > > I worry that

[PATCH] D99994: [CodeView] Add CodeView support for PGO debug information

rnk added a comment. In D4#2706515 , @Holman wrote: > Can someone help me get this checked in? Sure, I went ahead and pushed it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D4/new/

[PATCH] D99994: [CodeView] Add CodeView support for PGO debug information

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG77357208c46a: [CodeView] Add CodeView support for PGO debug information (authored by Holman, committed by rnk). Repository: rG LLVM Github

[PATCH] D100872: Use OpenFlags instead of boolean to set a file as text/binary

rnk added a subscriber: dexonsmith. rnk added a comment. +@dexonsmith who touched this API last. Comment at: clang/include/clang/Frontend/CompilerInstance.h:738-740 + createOutputFileImpl(StringRef OutputPath, llvm::sys::fs::OpenFlags Flags, bool

[PATCH] D100776: [clang/Basic] Make TargetInfo.h not use DataLayout again

rnk added subscribers: jyknight, echristo. rnk added a comment. Of the three people who commented on D17183 , you and I are on the only ones in favor of this approach. I think @echristo and @jyknight both preferred approach 2. I'd like to get at least an

[PATCH] D100488: [SystemZ][z/OS] Add IsText Argument to GetFile and GetFileOrSTDIN

rnk added a comment. In D100488#2689494 , @amccarth wrote: > Personally, I'm not a fan of boolean function parameters because of the > inline comments necessary to make the call site understandable. But it > appears to be consistent with LLVM Coding

[PATCH] D100467: [clang] [AArch64] Fix handling of HFAs passed to Windows variadic functions

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, micro readability suggestion Comment at: clang/lib/CodeGen/TargetInfo.cpp:5691 + // no special handling of HFAs/HVAs. + if (isHomogeneousAggregate(Ty, Base, Members) &&

[PATCH] D100468: [clang] [test] Share patterns in CodeGen/ms_abi_aarch64.c between cases. NFC.

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

[PATCH] D100361: [Driver] Make the findVCToolChainViaEnvironment case-insensitive

rnk added a comment. Example: https://github.com/llvm/llvm-project/blob/main/clang/unittests/Driver/ToolChainTest.cpp#L29 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100361/new/ https://reviews.llvm.org/D100361

[PATCH] D100361: [Driver] Make the findVCToolChainViaEnvironment case-insensitive

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm This code could greatly benefit from some unit tests similar in style to the GCC installation detection tests, but It would be unreasonable for me to insist on them. Repository: rG LLVM

[PATCH] D100374: [clang] [AArch64] Fix Windows va_arg handling for larger structs

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/lib/CodeGen/TargetInfo.cpp:6106 uint64_t Members = 0; IsIndirect = !isHomogeneousAggregate(Ty, Base, Members); } rnk wrote: >

[PATCH] D100374: [clang] [AArch64] Fix Windows va_arg handling for larger structs

rnk added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:6106 uint64_t Members = 0; IsIndirect = !isHomogeneousAggregate(Ty, Base, Members); } Do we need to worry about H[VF]As passed to variadic functions, or is there a special case

[PATCH] D99994: [CodeView] Add CodeView support for PGO debug information

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

[PATCH] D100150: [Sanitizers] Add a flag -f[no-]sanitize-merge-traps

rnk created this revision. rnk added reviewers: pcc, phosek, rsmith, zequanwu. Herald added subscribers: jansvoboda11, dexonsmith, dang. rnk requested review of this revision. Herald added a project: clang. Without this flag, enabling optimizations causes clang to emit a single ubsantrap for

[PATCH] D99994: [CodeView] Add CodeView support for PGO debug information

rnk added a comment. Seems reasonable, but we still need a small IR test. Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:807 // TODO: Figure out which other flags need to be set. + if (MMI->getModule()->getProfileSummary(/* IsCS */ false) != nullptr) { +

[PATCH] D99994: [CodeView] Add CodeView support for PGO debug information

rnk added a comment. IMO it's best to avoid adding fields to DICompileUnit if at all possible. It's the "god object" / "katamari damacy" of module debug info. Is there something about the IR module that indicates if PGO data is present or not? We could check that instead. I looked, but I

[PATCH] D99199: Make -fcrash-diagnostics-dir control the Windows (mini-)dump location

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99199/new/ https://reviews.llvm.org/D99199 ___ cfe-commits mailing list

[PATCH] D99663: [clang-cl] [Sema] Do not prefer integral conversion over floating-to-integral for MS compatibility 19.28 and higher.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Slowly we will peel back the layers of compatibility hacks until both compilers conform. :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D94355: [Passes] Add relative lookup table converter pass

rnk added subscribers: aeubanks, rnk. rnk added inline comments. Comment at: llvm/test/Other/opt-O3-pipeline-enable-matrix.ll:322-323 ; CHECK-NEXT: Simplify the CFG +; CHECK-NEXT: Relative Lookup Table Converter +; CHECK-NEXT: FunctionPass Manager ; CHECK-NEXT:

[PATCH] D17183: Make TargetInfo store an actual DataLayout instead of a string.

rnk added a comment. I think 1 is OK as long as we can assert that things don't get out of sync. I like the idea of putting the prefix next to the data layout string. In the long run, splitting up Support might be reasonable, and pushing DataLayout down out of IR seems like something that

[PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

rnk added a comment. In D99426#2653725 , @MaskRay wrote: > This touches a lot of files. I am a bit worried that it would not be easy for > a contributor to know OF_TextWithCRLF is needed to make SystemZ happy. Right, we need to separate text-ness for

[PATCH] D98146: OpaquePtr: Turn inalloca into a type attribute

rnk accepted this revision. rnk added a comment. This looks good from my PoV but make sure all of David's oustanding concerns are addressed. I think we hoped that the preallocated feature would have replaced inalloca before the opaque type pointer transition happened, but there's no reason to

[PATCH] D99363: [Windows] Turn off text mode in TableGen and Rewriter to stop CRLF translation

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

[PATCH] D99363: [Windows] Turn off text mode in TableGen and Rewriter to stop CRLF translation

rnk requested changes to this revision. rnk added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Frontend/Rewrite/FrontendActions.cpp:190 + std::unique_ptr OS = CI.createDefaultOutputFile( +

[PATCH] D99199: Make -fcrash-diagnostics-dir control the Windows (mini-)dump location

rnk added a comment. Seems reasonable, but I have a concern about the cc1 line being replicable Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5127 +#ifdef _WIN32 + // Put (mini-)dumps in the same place as other crash diagnostics files. I guess I would

[PATCH] D96363: Mark output as text if it is really text

rnk added a comment. In D96363#2650999 , @abhina.sreeskantharajan wrote: > Right, adding a new flag like OF_TextNoCrlf is something I can look into as a > solution. However, if Windows is always using binary mode and has no use for > OF_Text flag,

[PATCH] D96363: Mark output as text if it is really text

rnk added a comment. This is why, in the past, we have taken the direction of opening all files as binary files. You have probably noticed all of the changes in this direction that you have to work around. I think the best solution here would be to add a new `OF_TextNoCrlf` mode that does what

[PATCH] D99107: [clang][CodeGen] Do not emit NVRO debug helper when not emitting debug info

rnk added a comment. In D99107#2642599 , @nick wrote: > Is this hack actually needed? I could not reproduce a problem with > https://bugs.chromium.org/p/chromium/issues/detail?id=860398#c13 repro, the > breakpoint fires for me and I see the variable. >

[PATCH] D99107: [clang][CodeGen] Do not emit NVRO debug helper when not emitting debug info

rnk added a comment. We try to uphold the invariant that -g flags do not affect generated code, so I don't think we should do this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99107/new/ https://reviews.llvm.org/D99107

[PATCH] D98868: [Driver] Add -print-runtime-dir

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm I'm somewhat surprised this doesn't exist already, but I looked at the -print-* options and don't see anything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D98472: Emit inline implementation of __builtin__wmemchr on MSVCRT platforms.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm w nit Comment at: clang/test/CodeGen/wmemchr.c:7 +const wchar_t *wmemchr_test(const wchar_t *s, const wchar_t c, size_t n) { + // CHECK: [[S:%.*]] = load + // CHECK:

[PATCH] D97687: [SEH] Fix capture of this in lambda 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/D97687/new/ https://reviews.llvm.org/D97687 ___

[PATCH] D97872: [clang] Don't assert in EmitAggregateCopy on trivial_abi types

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

[PATCH] D97872: [clang] Don't assert in EmitAggregateCopy on trivial_abi types

rnk added inline comments. Comment at: clang/test/CodeGenCXX/trivial_abi.cpp:267 +// PR42961 +Small (*fp)() = []() -> Small {}; Please check for the IR for the `__invoke` member of the lambda class. I think it's at least a bit interesting that we create two

[PATCH] D97941: [Reland] "Do not apply calling conventions to MSVC entry points"

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97941/new/ https://reviews.llvm.org/D97941 ___ cfe-commits mailing list

[PATCH] D97785: [SystemZ][z/OS] Distinguish between text and binary files on z/OS

rnk added a comment. Yes, we took steps to suppress CRLF generation. Part of that was to avoid Windows-only test failures from trailing CR whitespace interacting with tools like `diff`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97785/new/

[PATCH] D97941: [Reland] "Do not apply calling conventions to MSVC entry points"

rnk added a comment. Seems reasonable Comment at: clang/lib/Sema/SemaDecl.cpp:11206-11207 +} else if (FT->getCallConv() != CC_X86StdCall) { + // Default calling convention for WinMain, wWinMain and DllMain is + // __stdcall + FT = Context.adjustFunctionType(

[PATCH] D96709: Add Windows ehcont section support (/guard:ehcont).

rnk added inline comments. Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:499 CmdArgs.push_back("-guard:cf-"); +} else if (GuardArgs.equals_lower("ehcont")) { + CmdArgs.push_back("/guard:ehcont"); pengfei wrote: > rnk wrote: > > This is gone

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, but please make sure that Richard is happy CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95396/new/ https://reviews.llvm.org/D95396 ___

[PATCH] D95396: Improve static_assert/_Static_assert diagnostics

rnk added a comment. I guess my only concern is, what happens if MSVC fixes assert.h? Do we need to make the implicit `#define static_assert _Static_assert` conditional on the absence of any `static_assert` definition? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95396/new/

[PATCH] D96709: Add Windows ehcont section support (/guard:ehcont).

rnk accepted this revision. rnk added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Driver/ToolChains/MSVC.cpp:499 CmdArgs.push_back("-guard:cf-"); +} else if (GuardArgs.equals_lower("ehcont")) { +

[PATCH] D97534: SEH: capture 'this'

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97534/new/ https://reviews.llvm.org/D97534 ___ cfe-commits mailing list

[PATCH] D97447: Add GNU attribute 'retain'

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm with a minor doc tweak My reading of the discussion is that this is ready, but please wait for others if you are aware of any outstanding concerns. Comment at:

[PATCH] D97449: [Diagnose] Unify MCContext and LLVMContext diagnosing

rnk added a comment. This still feels a bit messy and I don't totally understand how it all fits together, but I'm super supportive of getting rid of the inline asm diagnostic handler and standardizing on DiagnosticHandler/DiagnosticInfo. Comment at:

[PATCH] D97447: Add GNU attribute 'retain'

rnk added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:74 +COFF and Mach-O targets (Windows and Apple platforms), this attribute prevents +the definition and its symbol from being removed by the linker. On ELF +targets, it has no effect on its own, and the

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm > I see that it's currently listed as Undocumented. We should fix that while > we're rehashing how this is supposed to work, and clarify what it does on > each platform. As I understand it,

[PATCH] D97446: Change some addUsedGlobal to addUsedOrCompilerUsedGlobal

rnk added a comment. I think the main user-facing feature here to think about is `__attribute__((used))`. I see that it's currently listed as Undocumented. We should fix that while we're rehashing how this is supposed to work, and clarify what it does on each platform. As I understand it,

[PATCH] D97437: Rewrite MSVC toolchain discovery with VFS

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm I think we also do some registry searching, which is not virtualized. I guess those are all absolute references and paths, so if you use clangd on the same machine, it should just work. It

[PATCH] D97411: [DebugInfo] Add an attribute to force type info to be emitted for types that are required to be complete.

rnk added a comment. I expected nodebug already applied to types, even though the documentation says it only affects variables and functions. We should update the docs. I think if we already have `nodebug` spelling, we don't need to make something general with modes. The "always emit" use case

[PATCH] D96638: [Driver][Windows] Support per-target runtimes dir layout for profile instr generate

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96638/new/ https://reviews.llvm.org/D96638 ___ cfe-commits mailing list

[PATCH] D97411: [DebugInfo] Add an attribute to force type info to be emitted for types that are required to be complete.

rnk added a comment. To help bikeshed the name, I can imagine a few other use cases: - an attribute to suppress type info emission, never emit full type info for this type (similar to nodebug / artificial attributes for functions) -- this could help optimize debug info size - an attribute to

[PATCH] D96638: [Driver][Windows] Support per-target runtimes dir layout for profile instr generate

rnk added inline comments. Comment at: clang/lib/Driver/ToolChain.cpp:468-469 // Check for runtime files in the new layout without the architecture first. - std::string CRTBasename = - buildCompilerRTBasename(*this, Args, Component, Type, /*AddArch=*/false); - for

[PATCH] D96638: [Driver][Windows] Support per-target runtimes dir layout for profile instr generate

rnk added inline comments. Comment at: clang/lib/Driver/ToolChain.cpp:446 case ToolChain::FT_Shared: -Suffix = TT.isOSWindows() - ? (TT.isWindowsGNUEnvironment() ? ".dll.a" : ".lib") +Suffix = Triple.isOSWindows() + ?

[PATCH] D96832: [Clang][Attributes] Allow not_tail_called attribute to be applied to virtual function.

rnk added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4109 - [[clang::not_tail_called]] int foo2() override; -}; }]; aaron.ballman wrote: > rnk wrote: > > aaron.ballman wrote: > > > Quuxplusone wrote: > > > > aaron.ballman

[PATCH] D96638: [Driver][Windows] Support per-target runtimes dir layout for profile instr generate

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96638/new/ https://reviews.llvm.org/D96638 ___ cfe-commits mailing list

[PATCH] D96832: [Clang][Attributes] Allow not_tail_called attribute to be applied to virtual function.

rnk added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:4083-4084 - - // not_tail_called has no effect on an indirect call even if the call can - // be resolved at compile time. - return (*fn)(a); This paragraph about indirect

[PATCH] D96838: Add GNU attribute 'retain'

rnk added a comment. It should be possible to make retain work on COFF for internal linkage things by emitting a non-comdat section, even when /Gy / -ffunction/data-sections are enabled. That's complicated if the thing being retained is in a comdat group, but I think it's doable. If the group

[PATCH] D96638: [Driver][Windows] Support per-target runtimes dir layout for profile instr generate

rnk added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:797 CmdArgs.push_back(Args.MakeArgString( - "--dependent-lib=" + TC.getCompilerRTBasename(Args, "profile"))); + "--dependent-lib=" +

[PATCH] D97098: [Utils] Add an option to specify number of cores to use in creduce-clang-crash.py

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

[PATCH] D77598: Integral template argument suffix and cast printing

rnk added inline comments. Comment at: clang/lib/AST/TemplateBase.cpp:71-72 if (T->isBooleanType() && !Policy.MSVCFormatting) { Out << (Val.getBoolValue() ? "true" : "false"); } else if (T->isCharType()) { rsmith wrote: > rsmith wrote: > > rnk

[PATCH] D97098: [Utils] Add an option to specify number of cores to use in creduce-clang-crash.py

rnk added inline comments. Comment at: clang/utils/creduce-clang-crash.py:81 self.creduce_flags = ["--tidy"] +if core_number > 0: + self.creduce_flags = ["--n", str(core_number)] I think we should try to pick a good default here. I think we can do

[PATCH] D97101: [Coverage] Emit zero count gap region between statements if first statements contains return, throw, goto, co_return

rnk added a comment. Are the check-profile presubmit test failures related to this patch? Do we have to revert D85036 in the same patch in order to keep the integration tests green? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

rnk added inline comments. Comment at: llvm/docs/LangRef.rst:1220 +``alignstack()`` +This indicates the alignment that should be considered by the backend when +assigning this parameter to a stack slot during calling convention chill wrote: > rnk wrote:

[PATCH] D95745: Support unwinding from inline assembly

rnk added a comment. Basically, this seems totally reasonable and I think this is the right direction. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95745/new/ https://reviews.llvm.org/D95745 ___

[PATCH] D95911: [Docs] Add some documentation for constructor homing, a debug info optimization (-fuse-ctor-homing)

rnk accepted this revision. rnk added a comment. lgtm too In D95911#2538292 , @dblaikie wrote: > (non-action idle thoughts: Might be worth revisiting this documentation to > make it a bit more direct/clearer that many of these optimizations (at least >

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

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Looks good with one suggestion. Comment at: clang/include/clang/AST/ASTContext.h:540 llvm::MapVector MangleNumbers; llvm::MapVector StaticLocalNumbers;

[PATCH] D95918: [Coverage] Propogate counter to condition of conditional operator

rnk added a comment. Nice. I believe there are some integration tests in compiler-rt/test/profile. First, please run them and make sure they pass. Second, @vsk, do you think it's worth adding an integration test for this, seeing as the bug is really only observable when you put all the

[PATCH] D95534: clang-cl: Invent a /winsysroot concept

rnk added subscribers: smeenai, compnerd. rnk added a comment. +People who have spent time on cross-compilation Windows sysroots in the past: @smeenai @compnerd Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95534/new/

[PATCH] D95482: [PATCH] [clang] Fix bug 48848 by removing assertion related to recoverFromMSUnqualifiedLookup

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. In the absence of a response, let's land this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95482/new/ https://reviews.llvm.org/D95482

[PATCH] D95673: [dllimport] Honor always_inline when deciding whether a dllimport function should be available for inlining (PR48925)

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

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

rnk added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:395-400 /// The number used to indicate this lambda expression for name /// mangling in the Itanium C++ ABI. unsigned ManglingNumber : 31; +/// The device side mangling number. +

[PATCH] D95482: [PATCH] [clang] Fix bug 48848 by removing assertion related to recoverFromMSUnqualifiedLookup

rnk added a subscriber: erik.pilkington. rnk added a comment. @erik.pilkington , you added this assert in http://github.com/llvm/llvm-project/commit/3cdc317342d8c2b36de2839ea6ebefec17cb271e, are you OK with this? I'm OK with the change. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D95499: [NFC] Disallow unused prefixes under clang/test/CodeGenCXX

rnk accepted this revision. rnk added a subscriber: akhuang. rnk added a comment. This revision is now accepted and ready to land. lgtm I believe @akhuang's recent change to this file made it so debug info names are unqualified in all modes. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D95187: [DebugInfo][CodeView] Use as the display name for lambdas.

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

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

rnk added a comment. In D91913#2526317 , @hvdijk wrote: > My concern isn't with the revert itself, it's without waiting for approval. > That's a crystal clear LLVM developer policy violation: changes need to be > approved, or obvious, or by developers

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

rnk added a comment. OK, thanks for the direction. We'll try to port code to use the device you listed. I see you think we need a new warning for this, a clang 12 release note, and then this can reland. @hvdijk , that plan looks good to me, and I'll let you and Richard sort it out. I don't

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

rnk added a comment. In D91913#2525993 , @hvdijk wrote: > @rnk Taking it upon yourself to revert this without approval and without > communication on all branches, especially given the earlier suggestion by > @rsmith to only revert this on the LLVM 12

[PATCH] D91913: Suppress non-conforming GNU paste extension in all standard-conforming modes

rnk added a comment. We need a solution to our problem in the short term, though: there must be a code pattern that can be used in -std=c++14 modes to accomplish what the `, ## __VA_ARGS__` code pattern accomplishes with GCC extensions. As of right now, my understanding is that `__VA_OPT__` is

[PATCH] D95187: [DebugInfo][CodeView] Use as the display name for lambdas.

rnk added inline comments. Comment at: clang/include/clang/AST/Mangle.h:92 + virtual StringRef getLambdaString(const CXXRecordDecl *Lambda) = 0; + akhuang wrote: > rnk wrote: > > I think I would prefer to have this return a number. I think CGDebugInfo > >

[PATCH] D95187: [DebugInfo][CodeView] Use as the display name for lambdas.

rnk added a comment. I see a potential issue, but I think this looks good otherwise. Comment at: clang/lib/AST/ItaniumMangle.cpp:210 +if (Number == 0) + return getAnonymousStructId(Lambda); +return Number; This has the potential to create a

[PATCH] D95187: [DebugInfo][CodeView] Use as the display name for lambdas.

rnk added a comment. I see a variety of presubmit failures in Harbormaster that seem related, so make sure to check those. Comment at: clang/include/clang/AST/Mangle.h:92 + virtual StringRef getLambdaString(const CXXRecordDecl *Lambda) = 0; + I think I

[PATCH] D93668: [clang] Override the Fuchsia platform ABI using the triple environment

rnk added a comment. I guess the spelling -fc++-abi= is intuitive, discoverable, and easy to use. I'd be OK going back to that, as long as the compiler knows which ABIs work on which targets and rejects the unsupported configurations. Regarding the idea of separating the platform ABI from the

[PATCH] D92136: [clang] Enhanced test for --relocatable-pch, and corresponding fixes for Windows

rnk added a comment. Seems reasonable Comment at: clang/lib/Frontend/VerifyDiagnosticConsumer.cpp:534 +#ifdef _WIN32 +// Support special case of Windows absolute file paths rnk wrote: > I think looking for a slash after the colon is a bit more

[PATCH] D82862: [ThinLTO] Always parse module level inline asm with At dialect

rnk added a comment. In D82862#2512908 , @uweigand wrote: > So why do you want GNU inline asm for clang-cl anyway? I thought the whole > point of clang-cl was to be compatible with the Microsoft Visual Studio > compiler, which I understand only

[PATCH] D95001: [CodeView] Emit function types in -gline-tables-only.

rnk added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1050-1052 + // Don't include a linkage name in line tables only, except to differentiate + // between lambdas. + if (CGM.getCodeGenOpts().hasReducedDebugInfo() || RD->isLambda()) We

[PATCH] D94639: [DebugInfo][CodeView] Change in line tables only mode to emit parent/context scopes for functions, using declarations for types

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm In D94639#2497031 , @dblaikie wrote: > (I'm generally good with this - llvm & clang changes should be committed > separately & maybe the linkage part

[PATCH] D94646: [clang][MSVC] Fix missing MSInheritanceAttr in template specialization.

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

[PATCH] D94646: [clang][MSVC] Fix missing MSInheritanceAttr in template specialization.

rnk added a comment. Thanks, this seems like the right spot. Comment at: clang/lib/Sema/SemaTemplate.cpp:9767 +if (Context.getTargetInfo().getCXXABI().isMicrosoft() && +Def->hasAttr()) { Let's remove this condition. There will only ever be

[PATCH] D43002: [CodeView] Emit S_OBJNAME record

rnk added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:212 + /// Output filename used in the COFF debug information. + std::string COFFOutputFilename; + aganea wrote: > rnk wrote: > > Let's bikeshed the name a bit. This thing is the

[PATCH] D94364: [clang] Allow specifying the aapcs and aapcs-vfp for windows on arm

rnk added a comment. I don't see how supporting this attribute would break interop with MSVC. MSVC doesn't support these attributes, only GCC and Clang do. Presumably in the wine environment, some headers using these _GNUC extensions are being included somewhere, and IMO we should accept the

[PATCH] D94639: [DebugInfo][CodeView] Change in line tables only mode to emit parent/context scopes for functions, using declarations for types

rnk added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1050-1052 + // Don't include a linkage name in line tables only. + if (CGM.getCodeGenOpts().hasReducedDebugInfo()) +Identifier = getTypeIdentifier(Ty, CGM, TheCU); I see Amy included

[PATCH] D94639: [DebugInfo][CodeView] Change in line tables only mode to emit parent/context scopes for functions, using declarations for types

rnk added a comment. In D94639#2496969 , @akhuang wrote: > In D94639#2496950 , @dblaikie wrote: > >> How does any of this deal with overloading? I guess for either solution >> (qualified name or real scopes) you

[PATCH] D94537: [IR] move nomerge attribute from function declaration/definition to callsites

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, thanks! Comment at: clang/lib/CodeGen/CGCall.cpp:1973 } - if (!AttrOnCallSite && TargetDecl->hasAttr())

[PATCH] D92751: [clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm, thanks Comment at: llvm/test/CodeGen/AArch64/arm64-windows-calls.ll:117 + ret %struct.Pod %x1 +; CHECK: ldp d0, d1, [x0] +} peterwaller-arm wrote: > rnk

[PATCH] D94325: [DebugInfo][CodeView] Use a fancier function display name when using line tables only to better differentiate between functions.

rnk added a comment. I put some comments on https://bugs.llvm.org/show_bug.cgi?id=48432#c4 about this. I'm not sure we should keep going this direction. Thanks for getting numbers, though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94325/new/

[PATCH] D93668: [clang] Add -ffuchsia-c++-abi flag to explicitly use the Fuchsia C++ ABI

rnk added a comment. In D93668#2485719 , @mcgrathr wrote: > It's fine to have a target tuple translate to a default C++ ABI. > But the C++ ABI selection is fundamentally not a target flavor thing. It's > just a C++ ABI thing. > So using the target tuple

[PATCH] D92751: [clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate

rnk added inline comments. Comment at: clang/lib/CodeGen/CGCXXABI.h:143 /// Returns how an argument of the given record type should be passed. virtual RecordArgABI getRecordArgABI(const CXXRecordDecl *RD) const = 0; nit: I would group the new virtual

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