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

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

rnk added a comment. In D110257#3015641 , @hsmhsm wrote: > The logic at > https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/InlineFunction.cpp#L2093 > assumes that static allocas (within callee) are contiguous. No, it doesn't mak

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

rnk added a comment. In D110257#3016918 , @hsmhsm wrote: > That said, it is still good idea (and actually an explicitly not mandated > requirement) to maintain the contiguity of the static allocas at the top of > the basic block as one cluster, and it s

[PATCH] D110257: [CFE][Codegen] Do not break the contiguity of static allocas.

rnk added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:103 +if (!ArraySize) { + auto *EBB = AllocaInsertPt->getParent(); + auto Iter = AllocaInsertPt->getIterator(); hsmhsm wrote: > jdoerfert wrote: > > arsenm wrote: > > > Why is there

[PATCH] D110665: [clang] Don't use the AST to display backend diagnostics

rnk added inline comments. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:129 SmallVector LinkModules; +std::vector> +ManglingFullSourceLocs; As David says, this could be a plain hash map, maybe hash_code isn't hashable, but it must have some w

[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] 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 https://rev

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

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

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

[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=" + llvm::sys::path::filename(ClangRtProfilePath)

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

[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] 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 cfe-commits@

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

[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() + ? (Triple.isWindowsGN

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

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

[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 cfe-commits@lists.llv

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

[PATCH] D103452: [clang] Fix reading long doubles with va_arg on x86_64 mingw

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/test/CodeGen/mingw-long-double.c:56-58 + // GNU32: bitcast i8* %argp.cur to x86_fp80* + // GNU64: bitcast i8* %argp.cur to x86_fp80** + // MSC64: bitcast

[PATCH] D103372: [InstrProfiling] If no value profiling, make data variable private and (for Windows) use one comdat

rnk added a comment. I think MachO doesn't have comdats, so we need to leave the `__profd_` linkage as it was. I think private linkage also prevents atomization, which inhibits GC / dead stripping. I don't think this change really makes sense for MachO, so you could reasonably reland with that

[PATCH] D103806: [SystemZ][z/OS] Pass OpenFlags when creating tmp files

rnk added a comment. I see, thanks. Comment at: llvm/lib/Support/Path.cpp:1295 SmallString<128> ResultPath; - if (std::error_code EC = - createUniqueFile(Model, FD, ResultPath, OF_Delete, Mode)) + if (std::error_code EC = createUniqueFile(Model, FD, ResultPath, Fl

[PATCH] D103771: [clang][msvc] Define _HAS_STATIC_RTTI to 0, when compiling with -fno-rtti

rnk accepted this revision. rnk added a comment. Seems reasonable to me. Here is where it is defined in MSVC's yvals_core.h for the curious: https://github.com/microsoft/STL/blob/3cafa97eecdbfde41ea5c09126f877a7eb97f9e9/stl/inc/yvals_core.h#L569 CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D103452: [clang] Fix reading long doubles with va_arg on x86_64 mingw

rnk added inline comments. Comment at: clang/test/CodeGen/mingw-long-double.c:56-58 + // GNU32: bitcast i8* %argp.cur to x86_fp80* + // GNU64: bitcast i8* %argp.cur to x86_fp80** + // MSC64: bitcast i8* %argp.cur to double* mstorsjo wrote: > mstorsjo wrote: >

[PATCH] D103806: [SystemZ][z/OS] Pass OpenFlags when creating tmp files

rnk added inline comments. Comment at: llvm/lib/Support/Path.cpp:1295 SmallString<128> ResultPath; - if (std::error_code EC = - createUniqueFile(Model, FD, ResultPath, OF_Delete, Mode)) + if (std::error_code EC = createUniqueFile(Model, FD, ResultPath, Flags, Mode)

[PATCH] D103495: [static initializers] Don't put ordered dynamic initializers of static variables into global_ctors

rnk added subscribers: MaskRay, mstorsjo. rnk added a comment. + various .ctors stakeholders + @MaskRay + @mstorsjo This change was my idea, so I want to make sure there is buy in from other folks who use -fno-init-array. This has the potential to break user programs that rely on the order of

[PATCH] D103837: [clang] Apply MS ABI details on __builtin_ms_va_list on non-windows platforms on x86_64

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

[PATCH] D103495: [static initializers] Don't put ordered dynamic initializers of static variables into global_ctors

rnk added a subscriber: asbirlea. rnk added a comment. I think we do need to think about globalopt. Consider this example: extern "C" { int printf(const char *, ...); extern int sharedState; int sharedState = 0; int init1() { asm volatile(""); // block globalopt return ++sharedS

[PATCH] D103495: [static initializers] Emit global_ctors and global_dtors in reverse order when init_array is not used.

rnk added a subscriber: probinson. rnk added a comment. In D103495#2806097 , @jyknight wrote: > Won't this change cause weird effects with LTO, when you're merging multiple > TUs' global_ctors arrays before emitting? Won't this end up reversing the > or

[PATCH] D102736: Fix tmp files being left on Windows builds.

rnk added inline comments. Comment at: clang/lib/Frontend/CompilerInstance.cpp:857-858 + OS.reset(new llvm::raw_fd_ostream(Temp->FD, /*shouldClose=*/false, +Binary ? llvm::sys::fs::OF_None +

[PATCH] D103495: [static initializers] Emit global_ctors and global_dtors in reverse order when init_array is not used.

rnk accepted this revision. rnk added a comment. lgtm CHANGES SINCE LAST ACTION https://reviews.llvm.org/D103495/new/ https://reviews.llvm.org/D103495 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/li

[PATCH] D103772: [clang-cl] Reenable /Zc:twoPhase by default if targetting MSVC 2017 Update 3 or newer

rnk added a comment. If we look back at the original intention of our MSVC compatibility work, we tried to accept as much invalid code as necessary to parse "system headers". System headers were widely interpreted to bethe MSVC STL, ATL, and the Windows SDK. So, even if MSVC defaults to delayed

[PATCH] D68891: Remove unnecessary triple from test

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

[PATCH] D68891: Remove unnecessary triple from test

This revision was automatically updated to reflect the committed changes. Closed by commit rGe32a92c6fe8e: Remove unnecessary triple from test (authored by JohnTitor, committed by rnk). Changed prior to commit: https://reviews.llvm.org/D68891?vs=224680&id=352167#toc Repository: rG LLVM Githu

[PATCH] D104556: [InstrProfiling] Make CountersPtr in __profd_ relative

rnk added a comment. In D104556#2831787 , @MaskRay wrote: > Hmm, IMGREL (`IMAGE_REL_AMD64_ADDR32NB`) looks useful and is an alternative > solution to PC-relative relocations in ELF / relocation subtraction in Mach-O. > But leveraging it seems to need mor

[PATCH] D104556: [InstrProfiling] Make CountersPtr in __profd_ relative

rnk added a comment. In D104556#2837006 , @MaskRay wrote: > Is it possible to ask MSVC to add the 64-bit `IMAGE_REL_AMD64_REL64` and > `IMAGE_REL_ARM64_REL64`? > Newer compiler metadata techniques can benefit from using the same mechanism > for all thre

[PATCH] D99487: [CodeGen] Port basic block sections from ELF to COFF

rnk added inline comments. Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:1712 + COFF::IMAGE_SCN_MEM_READ | COFF::IMAGE_SCN_LNK_COMDAT, + SectionKind::getText(), COMDATSymName, + COFF::IMAGE_COMDAT_SELECT_NODUPLICATES, UniqueID);

[PATCH] D104058: ThinLTO: Fix inline assembly references to static functions with CFI

rnk added a comment. I think the new approach of skipping non C-ish identifier names is reasonable. Looks good to me, but wait for the more active reviewers to stamp it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104058/new/ https://reviews.llv

[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#2917340 , @SeTSeR wrote: > Hello! As far as I understand, Clang does not preserve info about namespace > for functions declared in namespaces with `-gline-numbers-only` flag. Is > there any way to retrieve this info for DWA

[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#2918559 , @SeTSeR wrote: > Thank you for the detailed answer. I'll discuss our use case with our team. > Should I create a separate ticket for this? Or maybe it would be better if I > submitted the PR adding this flag? It'

[PATCH] D108021: [dllexport] Instantiate default ctor default args

rnk added a comment. I think it looks good, but it needs a test. I worry that we might have the same bug for copy closures, but the fix will be different, since those are used in exception handling. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D10

[PATCH] D108013: [NFC] Rename AttributeList::has/getAttribute() -> has/getAttributeImpl()

rnk added a comment. There's a related change here: https://reviews.llvm.org/D105485 Honestly, I like this approach better. Adding a strongly-typed integer wrapper is a pretty heavyweight solution for a problem that seems solvable with better named methods. As a practical matter, consider spli

[PATCH] D108021: [dllexport] Instantiate default ctor default args

rnk added a comment. Thanks for the patch and test. Can I commit this for you? Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6012 +auto *CD = dyn_cast(MD); +if (CD && CD->isDefaultConstructor() && TSK == TSK_Undeclared) { + S.InstantiateDefaultCtorDefaultA

[PATCH] D106721: [AArch64] Implemnt MSVC __mulh and __umulh builtins and corresponding IR level intrinsics

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

[PATCH] D104871: [Docs] use -fprofile-generate for IR PGO and -fprofile-instr-generate for code coverage

rnk added inline comments. Comment at: clang/docs/UsersManual.rst:2117-2118 -Clang also supports profiling via instrumentation. This requires building a -special instrumented version of the code and has some runtime overhead during the profiling, but it provides more detailed

[PATCH] D104556: [InstrProfiling] Make CountersPtr in __profd_ relative

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. I'm happy with this. Comment at: compiler-rt/include/profile/InstrProfData.inc:79 +INSTR_PROF_DATA(const IntPtrT, IntPtrTy, CounterPtr, +ConstantExpr::getSub(Consta

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

rnk added inline comments. Comment at: clang/include/clang/Basic/Builtins.def:1059 +#undef strcasecmp +#undef strncasecmp + thakis wrote: > GMNGeoffrey wrote: > > chandlerc wrote: > > > thakis wrote: > > > > Why do we need this with bazel but not with other windo

[PATCH] D8467: C++14: Disable sized deallocation by default due to ABI breakage

rnk added a comment. In D8467#3095610 , @zixuan-wu wrote: > Hi, I am wondering could -fsized-deallocation this be enabled by default > nowadays in 2021? Yes, I think so. This should be a matter of adjusting the default in the driver. We should not bring

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

rnk added a comment. Please add a note to clang/docs/ReleaseNotes.rst about the behavior change. The clangd test failure seems related to this change, and the other ones could be as well. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6405 + // by default. + if (Arg *A =

[PATCH] D107049: [clang-repl] Re-implement clang-interpreter as a test case.

rnk added a subscriber: dblaikie. rnk added a comment. So, to back up a bit, do I understand correctly that this change adds tests to the check-clang test suite that JIT compiles C++ code for the host and throws C++ exceptions? Can we reconsider that? We have a policy of not running execution t

[PATCH] D112890: headers: optionalise some generated resource headers

rnk added a comment. I think this seems reasonable. Generally speaking, we have tests in clang that generate IR for targets that are not enabled in LLVM. It is kind of nice: you don't have to mark the IRGen tests with `REQUIRES: foo-registered-target`. If we want to test these ARM and RISCV in

[PATCH] D112399: Get Bazel building `//clang` on Windows with clang-cl.

rnk added inline comments. Comment at: utils/bazel/.bazelrc:81 +build:windows --copt=/Oi --host_copt=/Oi +build:windows --cxxopt=/Zc:rvalueCast --host_cxxopt=/Zc:rvalueCast + Try adding `/permissive-` to get more conforming behavior from clang-cl. If that doesn'

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

rnk accepted this revision. rnk added a comment. Looks good from my end. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112921/new/ https://reviews.llvm.org/D112921 ___ cfe-commits mailing list cfe-commit

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Sorry for the delay Comment at: clang/lib/Sema/TreeTransform.h:14576 !Second->getType()->isOverloadableType()) return getSema().CreateBuiltinArraySubscriptExp

[PATCH] D106585: Fix clang debug info irgen of i128 enums

rnk added a comment. Reviewing the code, I don't see any obvious sources of non-determinism (hash iteration). The only source I can imagine is uninitialized memory usage in the APInt. Let me know if this can be repro'd somehow. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D112492: [CUDA][HIP] Allow comdat for kernels

rnk added a subscriber: zequanwu. rnk added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4290-4293 - // Do not set COMDAT attribute for CUDA/HIP stub functions to prevent - // them being "merged" by the COMDAT Folding linker optimization. - if (D.hasAttr())

[PATCH] D112492: [CUDA][HIP] Allow comdat for kernels

rnk added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4290-4293 - // Do not set COMDAT attribute for CUDA/HIP stub functions to prevent - // them being "merged" by the COMDAT Folding linker optimization. - if (D.hasAttr()) -return false; --

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm I believe you have addressed John's comments, and I think the IR changes mainly affect AMDGPU users, so I don't think this will be too disruptive. Sorry about the delay, there's a bit of a bys

[PATCH] D113490: [NFC] Let Microsoft mangler accept GlobalDecl

rnk added inline comments. Comment at: clang/lib/AST/MicrosoftMangle.cpp:47 + if (auto *CD = dyn_cast(DC)) +GD = GlobalDecl(CD, Ctor_Complete); + else if (auto *DD = dyn_cast(DC)) I would prefer if you passed Ctor_Base and Dtor_Base here. I believe MSVC mo

[PATCH] D112492: [CUDA][HIP] Allow comdat for kernels

rnk added a comment. In D112492#3119892 , @tra wrote: > Yes, we do need to merge identical functions with **identical names** for > templates. > > The comdat-folding issue is different. IIUIC, it allows merging two functions > with identical code and **

[PATCH] D112492: [CUDA][HIP] Allow comdat for kernels

rnk added a comment. I think the key is the self-reference in the LEA instruction: > ; foo > .seh_proc "??$foo@H@@YAXH@Z" > ... > leaq"??$foo@H@@YAXH@Z"(%rip), %rcx > ... > ; foo > .seh_proc "??$foo@M@@YAXM@Z" > ... > leaq"??$foo@M@@YAXM@Z"(%rip), %rcx >

[PATCH] D106585: Fix clang debug info irgen of i128 enums

rnk added a subscriber: hans. rnk added a comment. In D106585#3122726 , @glandium wrote: > It seems to have been fixed in rG3c47c5ca13b8a502de3272e8105548715947b7a8 > . Are you sure yo

[PATCH] D8467: C++14: Disable sized deallocation by default due to ABI breakage

rnk added a comment. In D8467#3125386 , @rjmccall wrote: > Conceptually, this is (and will always be) a platform decision. On Apple > platforms, we have a formalized concept of deployment target, where specific > minimum OS versions support sized dealloc

[PATCH] D113707: [clang] Make -masm=intel affect inline asm style

rnk added a comment. This seems reasonable, but I worry about headers that contain AT&T style inline asm blobs, such as clang's own intrinsic headers. As you say, the directive is local. I would hate to end up in a place where we have to prefix every asm blob in clang/lib/Headers/*mmintrin.h wi

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

rnk added a comment. Let's not bring back the weak function thunk approach. That approach caused problems described in my commit, D8467 , which is why the code was removed. The next steps are to sort out right defaults for Apple and understand the libc++ test fa

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

[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] 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] 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: llvm/include/llvm/MC/MCCo

[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: clang/include

[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 cfe-commits@lists.llv

[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")) { + CmdArgs.push_back("/guar

[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/ https://r

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

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

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

[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 cfe-commits@

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

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

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

[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: [[C:%.*

[PATCH] D113707: [clang] Make -masm=intel affect inline asm style

rnk added a comment. One alternative would be to control this setting with a pragma stack, like we do for warnings, struct packing, etc. Something like: // foo.h #pragma clang asm_dialect push "att" static inline ... foo { asm("..."); } #pragma clang asm_dialect pop The pragma really wo

[PATCH] D113707: [clang] Make -masm=intel affect inline asm style

rnk added a comment. In D113707#3132812 , @thakis wrote: > Fairly big update. I'd like to punt on the pragma for now since this became a > lot more involved than expected already (see dependent patches; several of > them have already landed). No proble

[PATCH] D110257: [CFE][Codegen] Make sure to maintain the contiguity of all the static allocas

rnk added a comment. In D110257#3134001 , @JonChesterfield wrote: > So you won't articulate or document the new invariant and you think there's a > llvm-dev discussion that says we can't verify the invariant which you won't > reference, but means you w

[PATCH] D113490: [NFC] Let Microsoft mangler accept GlobalDecl

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/lib/AST/MicrosoftMangle.cpp:47 + if (auto *CD = dyn_cast(DC)) +GD = GlobalDecl(CD, Ctor_Complete); + else if (auto *DD = dyn_cast(DC))

[PATCH] D111457: [clang][test] Add lit helper for windows paths

rnk added a comment. These seem like generic substitutions that would be useful to anyone writing lit tests. We already have `%{pathsep}`: https://llvm.org/docs/CommandGuide/lit.html#substitutions These should live there as well, and be documented similarly, I think. Repository: rG LLVM Gith

[PATCH] D114394: Compile-time computation of string attribute hashes

rnk added subscribers: aeubanks, rnk. rnk added a comment. I think @aeubanks might be a good reviewer for this. I like the performance wins here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114394/new/ https://reviews.llvm.org/D114394 ___

[PATCH] D114082: [WIP] Normalize String Attributes

rnk added subscribers: void, rnk. rnk added a comment. > it is possible to list all internal keys in one place I think one of the original goals of adding support for string attributes (maybe @void will give some input) was specifically to avoid having one giant list with all the attributes sup

[PATCH] D114651: [clang-cl] Expose -Wall to clang-cl by unaliasing -Wall, keeping /Wall as alias to -Weverything

rnk added a comment. I will add that multiple users have run into this problem, and I think it might be more practical to consider unaliasing Wall altogether. Clang doesn't emit the same set of warnings as MSVC. Anyone seriously using `clang-cl /Wall` is going to receive a pile of `-WcxxNN-comp

[PATCH] D114651: [clang-cl] Expose -Wall to clang-cl by unaliasing -Wall, keeping /Wall as alias to -Weverything

rnk added a comment. It looks like this Wall -> Weverything alias is from my 2017 change: https://reviews.llvm.org/rGf9b08a382cc1e0669805991849ad69efbd4703e8 The commit message doesn't link to any bugs or any other motivating material. All I said is that this is being done for MSVC compatibility

[PATCH] D114576: [PR52549][clang-cl] Predefine _MSVC_EXECUTION_CHARACTER_SET

rnk added a comment. In D114576#3153189 , @mstorsjo wrote: > Would it be possible to move setting of this flag to somewhere else (e.g. > somewhere in Driver/ToolChains/MSVC.cpp), so that it would be picked up also > when building with the gcc-style driv

[PATCH] D111639: [Sema] check PseudoObject when rebuilding CXXOperatorCallExpr in template instantiation

rnk added a comment. Sounds good. This patch is still pending, right? I don't see it in the logs. Do you need someone to push it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111639/new/ https://reviews.llvm.org/D111639 _

[PATCH] D114902: [Attrs] Elaborate on the trivial_abi documentation

rnk created this revision. rnk added reviewers: aaron.ballman, rjmccall, dblaikie, ahatanak. rnk requested review of this revision. Herald added a project: clang. My team recently answered some questions about the trivial_abi attribute. This change attempts to distill those answers into user-focus

[PATCH] D102090: [CMake][ELF] Link libLLVM.so and libclang-cpp.so with -Bsymbolic-functions

rnk added a comment. In D102090#3169439 , @ebrevnov wrote: > While -Bsymbolic-funtions brings nice performance improvements it also > changes symbol resolution order. That means we effectively disabled > preemption for functions and all references from

[PATCH] D106585: Fix clang debug info irgen of i128 enums

rnk added a subscriber: aeubanks. rnk added a comment. Thanks for the reduction, it sounds like there is something wrong with the way DIEnumerator is uniqued in the LLVMContext. I probably don't have time to follow up on this, but maybe @dblaikie and @aeubanks can help out. Repository: rG LL

[PATCH] D106585: Fix clang debug info irgen of i128 enums

rnk added a comment. This usage of isSameValue seems suspicious: https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/LLVMContextImpl.h#L389 It seems to allow the possibility that APInts of differing bitwidth compare equal, but the hash value incorporates the bitwidth, so they may be inse

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