[PATCH] D150930: [Driver] Accept and ignore -fno-lifetime-dse argument

MaskRay added a comment. In D150930#4656909 , @brunodf wrote: > Hi, > > I found this review request and I just want to comment that I find it strange > that it was rejected. > > @MaskRay I understand that using a compile_commands.json configured for gcc

[PATCH] D154396: [clang] Add support for SerenityOS

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:25 + +static bool getPIE(const ArgList &Args, const ToolChain &TC) { + if (Args.hasArg(options::OPT_static, options::OPT_shared, I've simplified `Gnu.cpp` a bit for handling diff

[PATCH] D138846: MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt

MaskRay added a comment. In D138846#4657246 , @hans wrote: >> I just saw @glandium's earlier comment: >> >>> Code built with older versions of LLVM (e.g. rust) and running with the >>> updated runtime crash at startup with this change. >> >> This is the

[PATCH] D91442: [clang][Driver] Handle risvc in Baremetal.cpp.

MaskRay added inline comments. Herald added subscribers: wangpc, luke, sunshaoce, arichardson. Herald added a project: All. Comment at: clang/lib/Driver/ToolChains/RISCVToolchain.cpp:40 + const llvm::opt::ArgList &Args) { + if (Args.getLastArg

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

MaskRay added a comment. I'll land this patch next week if there is no objection. This seems very useful to a few parties and the current behavior is opt-in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108905/new/ https://reviews.llvm.org/D10890

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

MaskRay marked an inline comment as done. MaskRay added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4435-4448 struct CallEndCatch final : EHScopeStack::Cleanup { CallEndCatch(bool MightThrow) : MightThrow(MightThrow) {} bool MightThrow; vo

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

MaskRay updated this revision to Diff 557968. MaskRay marked an inline comment as done. MaskRay edited the summary of this revision. MaskRay added a comment. Update clang/docs/UsersManual.rst and clang/docs/ReleaseNotes.rst Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow

MaskRay added a comment. In D108905#4655694 , @ChuanqiXu wrote: > Oh, I am not saying the legacy and old comment. I mean you need to touch > ReleaseNotes.rst and UserManual.rst since we add a new flag. Also we need > either add a TODO/FIXME saying we ne

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that all exception objects' destructors are non-throwing

MaskRay updated this revision to Diff 557975. MaskRay marked an inline comment as done. MaskRay retitled this revision from "[ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that an exception object' destructor is nothrow" to "[ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to as

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that all exception objects' destructors are non-throwing

MaskRay updated this revision to Diff 557976. MaskRay marked an inline comment as done. MaskRay added a comment. remove unused err_ arguments. fix and test `throw new B` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108905/new/ https://reviews.llvm

[PATCH] D154396: [clang] Add support for SerenityOS

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:31 + Arg *Last = Args.getLastArg(options::OPT_pie, options::OPT_no_pie, + options::OPT_nopie); + return Last ? Last->getOption().matches(options::OPT_pie) : true; --

[PATCH] D154396: [clang] Add support for SerenityOS

MaskRay added inline comments. Comment at: clang/test/Driver/serenity.cpp:2 +// UNSUPPORTED: system-windows + +/// Test a cross compiler. https://github.com/MaskRay/Config/wiki/LLVM#clanglibdriver The test fails in a `-DCLANG_DEFAULT_RTLIB=compiler-rt -DCLANG_D

[PATCH] D154396: [clang] Add support for SerenityOS

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:164 +if (crtend_path.empty()) { + const char *crtend = (IsShared || IsPIE) ? "crtendS.o" : "crtend.o"; + crtend_path = TC.GetFilePath(crtend); crt* files are not tes

[PATCH] D108905: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that all exception objects' destructors are non-throwing

This revision was automatically updated to reflect the committed changes. Closed by commit rGc0a73918bfdd: [ItaniumCXXABI] Add -fassume-nothrow-exception-dtor to assume that all… (authored by MaskRay). Changed prior to commit: https://reviews.llvm.org/D108905?vs=557976&id=558008#toc Repository

[PATCH] D154396: [clang] Add support for SerenityOS

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.cpp:76 + if (!IsStatic || IsStaticPIE) +CmdArgs.push_back("--eh-frame-hdr"); + ADKaster wrote: > MaskRay wrote: > > This is not tested > Hm. this also seems like incorrect logic.

[PATCH] D128620: [Driver][test] Add libclang_rt.profile{{.*}}.a tests for NetBSD

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. It's best to switch to LLVM_ENABLE_PER_TARGET_RUNTIME_DIR=on layout at some point. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128620/new/

[PATCH] D128667: [WIP] Add Zstd ELF support

MaskRay added a comment. The lld/ change should be separate. And it needs tests. We should have several tests like mixed zlib and zstd input. OutputSections should not have a new member. I can handle the lld/ELF part. > Context not available. See https://llvm.org/docs/Phabricator.html#request

[PATCH] D128667: [WIP] Add Zstd ELF support

MaskRay added a comment. Tips: you may run `git diff -U0 --no-color --relative 'HEAD^' -- | path/to/llvm-project/clang/tools/clang-format/clang-format-diff.py -p1 -i` to clang-format your local change. Comment at: lld/ELF/Driver.cpp:957 +if (compression::zlib::isAvailable

[PATCH] D128465: Zstandard as a second compression method to LLVM

MaskRay added a comment. In D128465#3612948 , @phosek wrote: > I think this patch should be broken into at least two: > > 1. Refactor `llvm/include/llvm/Support/Compression.h` and > `llvm/lib/Support/Compression.cpp` to introduce a generic interface and

[PATCH] D128667: [WIP] Add Zstd ELF support

MaskRay added a comment. `llvm-objcopy --decompress-debug-sections clang.zstd clang.zstd.uncompress` doesn't work. % /tmp/out/custom1/bin/llvm-objcopy --decompress-debug-sections clang.zstd clang.zstd.uncompress /tmp/out/custom1/bin/llvm-objcopy: error: 'clang.zstd': '.debug_loc': zlib err

[PATCH] D128333: [clang][flang] Disable defaulting to `-fpie` for LLVM Flang

MaskRay added a comment. In D128333#3613665 , @awarzynski wrote: > @MaskRay, thank for taking a look! > > In D128333#3605745 , @MaskRay wrote: > >> gfortran defaults to PIE as well. > > While we strive to be comp

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

MaskRay added inline comments. Comment at: clang/include/clang/Driver/Options.td:1143 MarshallingInfoFlag>; +def fstrict_flex_arrays_EQ : Joined<["-"], "fstrict-flex-arrays=">,Group, + MetaVarName<"">, Values<"0,1,2,3">, Comment at: clang

[PATCH] D126864: [clang] Introduce -fstrict-flex-arrays= for stricter handling of flexible arrays

MaskRay added a comment. In D126864#3614235 , @sberg wrote: > In D126864#3612149 , @sberg wrote: > >> see https://reviews.llvm.org/D128643 "[clang] -fsanitize=array-bounds: treat >> all trailing size-1 arrays as

[PATCH] D128048: Add a new clang option "-ftime-trace-path"

MaskRay added inline comments. Comment at: clang/include/clang/Driver/Options.td:2819 "'per-pass-run': one report for each pass invocation">; def ftime_trace : Flag<["-"], "ftime-trace">, Group, HelpText<"Turn on time profiler. Generates JSON file based on output

[PATCH] D128757: [Driver][test] Add -fuse-ld= option tests for NetBSD

MaskRay added a comment. I am unsure it is useful to add a test for all OSxfuse_ld_value combinations, especially for an ELF OS like NetBSD which shares many characteristics with other ELF OSes. It will be overwhelming. The option may be better added in a netbsd specific file. Repository: rG

[PATCH] D128333: [clang][flang] Disable defaulting to `-fpie` for LLVM Flang

MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: flang/test/Driver/no-pie.f90:3 + +!- +! RUN COMMANDS awarzynski wrote: > MaskRay wrote: > > The `! RUN COMMANDS` and `EXPECTED

[PATCH] D128783: Check for more -fsanitize=array-bounds regressions

MaskRay added a comment. Do we need a C test (just add a `-x c` RUN line)? @serge-sans-paille Do you think we may likely make C++ stricter than C? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128783/new/ https://reviews.llvm.org/D128783

[PATCH] D128612: RISC-V big-endian support implementation

MaskRay added a comment. lld/ELF change should be dropped from this change. Don't use `config->endianness`. I feel sad that for little-endian users who don't use big-endian, every write now is slightly slower due to a check ;-) Comment at: clang/lib/Basic/Targets/RISCV.cpp:12

[PATCH] D128754: Refactor LLVM compression namespaces

MaskRay added inline comments. Comment at: llvm/docs/ReleaseNotes.rst:183-191 +* Refactor compression namespaces across the project, making way for a possible + introduction of alternatives to zlib compression in the llvm toolchain. + Changes are as follows: + * Remove crc32 f

[PATCH] D128726: [RISCV][NFC] Move static global variables into static variable in function.

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. > It's violate coding guideline in LLVM coding standard[1], because the the > initialization order is nondeterministic and that might increase the launch > time of programs. nondeterministi

[PATCH] D128612: RISC-V big-endian support implementation

MaskRay added a comment. In D128612#3618167 , @gbenyei wrote: > In D128612#3617955 , @MaskRay wrote: > >> lld/ELF change should be dropped from this change. Don't use >> `config->endianness`. >> I feel sad that f

[PATCH] D128625: [RISCV][Driver] Fix baremetal `GCCInstallation` paths

MaskRay added a comment. The description seems unclear to me. Is a `riscv64-unknown-linux-gnu` GCC installation selected while the requested target triple is `riscv64-unknown-elf`? This could be an instance of https://discourse.llvm.org/t/rfc-fix-loose-behaviors-of-clang-target/60272 (`[RFC] F

[PATCH] D128612: RISC-V big-endian support implementation

MaskRay added inline comments. Comment at: clang/lib/Basic/Targets/RISCV.h:144 + +StringRef LayoutEndianness = Triple.isLittleEndian() ? "e" : "E"; + gbenyei wrote: > MaskRay wrote: > > You may use a `char` and possibly fold this into the expression below. >

[PATCH] D128465: Zstandard as a second compression method to LLVM

MaskRay added inline comments. Comment at: llvm/cmake/modules/FindZSTD.cmake:1 +# Copyright (c) Meta Platforms, Inc. and affiliates. +# How did you derive this? The file seems contributed by you (I don't think facebook/zstd has such a file). It should not have

[PATCH] D128571: [X86] Support `_Float16` on SSE2 and up

MaskRay requested changes to this revision. MaskRay added inline comments. This revision now requires changes to proceed. Herald added a subscriber: StephenFan. Comment at: clang/test/CodeGen/X86/Float16-arithmetic.c:1 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown \ +// RUN

[PATCH] D128571: [X86] Support `_Float16` on SSE2 and up

MaskRay added inline comments. Comment at: clang/lib/Basic/Targets/X86.cpp:357 +// Turn on _float16 for x86 (feature sse2) +HasFloat16 = SSELevel >= SSE2; `_Float16` `for x86` convey no extra information since this file is for x86. Repository: rG L

[PATCH] D128571: [X86] Support `_Float16` on SSE2 and up

MaskRay accepted this revision. MaskRay added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Basic/Targets/X86.cpp:357 +// Turn on _float16 for x86 (feature sse2) +HasFloat16 = SSELevel >= SSE2; MaskRay wrote: >

[PATCH] D128783: [test] Check for more -fsanitize=array-bounds regressions

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. `clang/test/SemaCXX/array-bounds.cpp` has `tailpad` and `metaprogramming` tests which may be added here. Happy when @serge-sans-paille is happy. CHANGES SINCE LAST ACTION https://reviews

[PATCH] D128953: [NFC] refactor compression namespaces making way for a possible introduction of alternatives to zlib compression in the llvm toolchain.

MaskRay added a comment. > [NFC] refactor compression namespaces making way for a possible introduction > of alternatives to zlib compression in the llvm toolchain. I'd use `[NFC] Refactor llvm::zlib namespace` and place supplementary wording to the body. `in the llvm toolchain` is redundant i

[PATCH] D128953: [NFC] refactor compression namespaces making way for a possible introduction of alternatives to zlib compression in the llvm toolchain.

MaskRay added inline comments. Comment at: llvm/lib/ProfileData/InstrProf.cpp:154 +OS << ("profile uses " + compression::profile::AlgorithmName + + " compression but the profile reader was built " + "without " + + compression::profile::AlgorithmName + " su

[PATCH] D128953: [NFC] Refactor llvm::zlib namespace

MaskRay accepted this revision. MaskRay added a comment. LGTM. Happy when @phosek is happy Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128953/new/ https://reviews.llvm.org/D128953 ___ cfe-commits maili

[PATCH] D119296: KCFI sanitizer

MaskRay added a comment. > It uses LLVM prefix data to store a type identifier for each function and > injects verification code before indirect calls. Is "prefix data" stale now? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119296/new/ https://

[PATCH] D119296: KCFI sanitizer

MaskRay added a comment. There are 30+ comments not marked as "done". Having so many is distracting to reviewers as it's unclear whether this is in a ready-for-review state. Comment at: clang/include/clang/Basic/Features.def:233 FEATURE(modules, LangOpts.Modules) +FEATURE(kcf

[PATCH] D119296: KCFI sanitizer

MaskRay requested changes to this revision. MaskRay added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2280 +void CodeGenModule::setKCFIType(const FunctionDecl *FD, llvm::Function *F) { + + if (isa(FD) && !cast(F

[PATCH] D119296: KCFI sanitizer

MaskRay added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.h:1434 + + /// Emit KCFI type identifier constants and remove unused identifiers + void FinalizeKCFITypes(); End with a period. This function can be lower-case as well. There is no stro

[PATCH] D128754: [llvm] Remove unused and redundant crc32 funcction from llvm::compression::zlib namespace

MaskRay accepted this revision. MaskRay added a comment. Thanks! Comment at: llvm/lib/Support/Compression.cpp:25 using namespace llvm; using namespace llvm::compression; if a previous patch introduces a blank line between two `using`. please drop it from t

[PATCH] D119296: KCFI sanitizer

MaskRay added inline comments. Comment at: llvm/lib/Target/X86/X86AsmPrinter.cpp:125 + + // Emit int3 padding to allow runtime patching of the preamble. + EmitAndCountInstruction(MCInstBuilder(X86::INT3)); A previous comment isn't done. This doesn't explain tha

[PATCH] D129009: [Analysis][LTO] Fix LTO for aliased IFuncs.

MaskRay added a comment. LG but the test needs a change. Comment at: clang/test/Analysis/alias-indirect-function-lto.c:1 +// RUN: %clang_analyze_cc1 -flto -c +void f() __attribute__((ifunc("g"))); The test is added to the wrong place. clang Analysis != llvm Ana

[PATCH] D129009: [LTO] Fix LTO for aliased IFuncs

MaskRay added a comment. > As https://github.com/llvm/llvm-project/issues/56290 indicates Write the description, followed by `Fix(es)? https://github.com/llvm/llvm-project/issues/56290` on a separate line. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.

[PATCH] D129009: [LTO] Fix LTO for aliased IFuncs

MaskRay requested changes to this revision. MaskRay added inline comments. This revision now requires changes to proceed. Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:655 + if (isa(Aliasee)) { +return; + } https://llvm.org/docs/CodingStandards.ht

[PATCH] D129009: [LTO] Fix LTO for aliased IFuncs

MaskRay added inline comments. Comment at: llvm/test/LTO/Resolution/X86/alias-indirect-function-lto.ll:1 +; RUN: opt -module-summary -o %t.bc %s + Testing a command does not crash does not make good use of the test. Please check basic properties. And add a file-

[PATCH] D111081: [clang] [MinGW] Fix paths on Gentoo

MaskRay added a comment. Does Gentoo patch upstream to do something different? (`.../g++-v10/...`) Well, I wish that Gentoo does not do this. This adds complexity to Clang which in practice tries to provide some drop-in replacement ability. CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D129160: libclang.so: Make SONAME the same as LLVM version

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. LGTM. I have seen multiple reports that distributors found this behavior change surprising. I agree that having this more complex symbol versioning with libclang.so probably doesn't improve

[PATCH] D129009: [LTO] Fix LTO for aliased IFuncs

MaskRay added a comment. I feel obliged to point out that the sheer number of updates is too large. Every update triggers an email to all reviewers and subscribers, so please be mindful. Personally I don't mind how many updates I receive, but others may do. It's usually not too bad to have too m

[PATCH] D128953: [NFC] Refactor llvm::zlib namespace

MaskRay accepted this revision. MaskRay added inline comments. Comment at: llvm/unittests/Support/CompressionTest.cpp:21 using namespace llvm; +using namespace llvm::compression; Delete blank line between two `using` CHANGES SINCE LAST ACTION https://revie

[PATCH] D128953: [NFC] Refactor llvm::zlib namespace

MaskRay added inline comments. Comment at: llvm/include/llvm/Support/Compression.h:27 +static constexpr std::string AlgorithmName = "zlib"; static constexpr int NoCompression = 0; Is it still used? Prefer StringRef if the string is backed from some storage.

[PATCH] D128953: [NFC] Refactor llvm::zlib namespace

MaskRay added a comment. I think this can be pushed now. You need to remove the variable > In file included from > /var/lib/buildkite-agent/builds/llvm-project/llvm/lib/MC/ELFObjectWriter.cpp:41: > /var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/Support/Compression.h:27:30: > err

[PATCH] D119296: KCFI sanitizer

MaskRay added inline comments. Comment at: clang/lib/Driver/SanitizerArgs.cpp:63 SanitizerKind::Unreachable | SanitizerKind::Return; -static const SanitizerMask AlwaysRecoverable = -SanitizerKind::KernelAddress | SanitizerKind::KernelHWAddress; +static const SanitizerMas

[PATCH] D129009: [LTO] Fix LTO for aliased IFuncs

MaskRay added a comment. OK, I think the approach is fine. Comment at: llvm/include/llvm/IR/GlobalIFunc.h:97 + + /// Method to apply specific operation to all resolver-related values. + /// If resolver target is already a global object, then apply the operation to --

[PATCH] D128754: [llvm] Remove unused and redundant crc32 funcction from llvm::compression::zlib namespace

MaskRay accepted this revision. MaskRay added a comment. You may push this patch and its prerequisite while other patches are still to be reviewed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128754/new/ https://reviews.llvm.org/D128754 ___

[PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

MaskRay accepted this revision. MaskRay added a comment. Please check both `LLVM_ENABLE_ZSTD={on,off}` work. Ideally, check that when zstd' cmake (`libzstd-dev` on Debian) is unavailable, `LLVM_ENABLE_ZSTD=on` works (there is no zstd functionality but the cmake invocation should succeed). ===

[PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

MaskRay added a comment. Consider adding `have_zstd` to `compiler-rt/test/lit.common.cfg.py` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465 ___ cfe-commits mai

[PATCH] D129009: [LTO] Fix LTO for aliased IFuncs

MaskRay added inline comments. Comment at: llvm/test/LTO/Resolution/X86/alias-indirect-function-lto.ll:15 + +@foo = ifunc i32 (i32), i32 (i32)* ()* @foo_resolver + Replace all these `xxx*` to `ptr` (opaque pointers). See recent clang's codegen. Repository: rG

[PATCH] D121141: [Clang] Add `-funstable` flag to enable unstable and experimental features: follow-up fixes

MaskRay added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:276 + +Enable unstable and experimental language and library features. + The doc is auto-generated by tablegen (https://discourse.llvm.org/t/clang-driver-options-td-docs-usersman

[PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

MaskRay added inline comments. Comment at: lld/test/lit.site.cfg.py.in:21 config.have_zlib = @LLVM_ENABLE_ZLIB@ +config.have_zstd = @LLVM_ENABLE_ZSTD@ config.have_libxar = @LLVM_HAVE_LIBXAR@ This needs a change in lld/test/CMakeLists.txt Looks like you haven't

[PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

MaskRay added inline comments. Comment at: lld/test/lit.site.cfg.py.in:21 config.have_zlib = @LLVM_ENABLE_ZLIB@ +config.have_zstd = @LLVM_ENABLE_ZSTD@ config.have_libxar = @LLVM_HAVE_LIBXAR@ MaskRay wrote: > This needs a change in lld/test/CMakeLists.txt > > L

[PATCH] D128465: [llvm] cmake config groundwork to have ZSTD in LLVM

MaskRay reopened this revision. MaskRay added a comment. This revision is now accepted and ready to land. This patch has changed a lot from what I have reviewed. The CMake change should be added along with `llvm::compression::zstd::*` functions. Otherwise the change just introduces some CMake var

[PATCH] D129424: [LinkerWrapper] Forward `-mllvm` options to the linker wrapper

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. > This patch adds the ability to use -mllvm options in the linker wrapper when > performing bitcode linking or the module compilation. This is done by passing > in the LLVM argument to the c

[PATCH] D129061: [Lex] Diagnose macro in command lines

MaskRay added inline comments. Comment at: clang/include/clang/Driver/Options.td:664 HelpText<"Define to (or 1 if omitted)">; +def DriverDefine : JoinedOrSeparate<["-"], "driver-define">, Group, +Flags<[CC1Option, FlangOption, FC1Option]>, MetaVarName<"=">, -

[PATCH] D122370: Split up large test files under clang/test/CodeGen/RISCV

MaskRay added a comment. I assume this is to improve test parallelism. Do you have runtime comparison? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122370/new/ https://reviews.llvm.org/D122370 ___ cfe-c

[PATCH] D122377: [PowerPC][Linux] Support 16-byte lock free atomics on pwr8 and up

MaskRay added inline comments. Comment at: clang/lib/Basic/Targets/PPC.h:449 +// For layout on Linux, we support up to 16 bytes. +if (getTriple().isOSLinux() && hasFeature("quadword-atomics")) { + MaxAtomicPromoteWidth = 128; Many `isOSLinux()` usage

[PATCH] D122407: [ASan] Reland of D116182 to always link asan_static library.

MaskRay accepted this revision. MaskRay added a comment. This revision is now accepted and ready to land. State in the description whether this is a partial or full revert of D121405 ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[PATCH] D122424: [clang] [Driver] Add clang's relative `../lib` path only when in build tree

MaskRay added a comment. Is `addPathIfExists(D, D.Dir + "/../" + OSLibDir, Paths);` from D108286 the issue? If yes, I lean toward reverting D108286 and possibly unsupporting `LIBCXX_LIBDIR_SUFFIX`. The multilib style `lib32`

[PATCH] D122407: [ASan] Reland of D116182 to always link asan_static library.

MaskRay added a comment. In D122407#3406066 , @MaskRay wrote: > State in the description whether this is a partial or full revert of D121405 > ? The commit message does not mention D121405

[PATCH] D122444: [Driver][Linux] Remove D.Dir+"/../lib" from default search paths for LLVM_ENABLE_RUNTIMES builds

MaskRay created this revision. MaskRay added reviewers: mgorny, sunlin. Herald added a subscriber: StephenFan. Herald added a project: All. MaskRay requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. The rule was added in 2014 to support -stdli

[PATCH] D121992: [Clang] [Driver] Add option to set alternative toolchain path

MaskRay added a comment. Why is --overlay-platform-toolchain added instead of using `-isystem` and `-L`? > In some cases, we need to set alternative toolchain path other than the > default with system (headers, libraries, dynamic linker prefix, ld path, > etc.), e.g., to pick up newer component

[PATCH] D121992: [Clang] [Driver] Add option to set alternative toolchain path

MaskRay added a comment. In D121992#3407220 , @qiucf wrote: > Hi, > >> Why is --overlay-platform-toolchain added instead of using -isystem and -L? >> >> The functionality overlaps with -B. Unsure why introduce a new mechanism. > > We may want to use an ex

[PATCH] D122444: [Driver][Linux] Remove D.Dir+"/../lib" from default search paths for LLVM_ENABLE_RUNTIMES builds

MaskRay marked an inline comment as done. MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Linux.cpp:316 + if (StringRef(D.Dir).startswith(SysRoot) && + D.getVFS().exists(D.Dir + "/../lib/libc++.so")) addPathIfExists(D, D.Dir + "/../lib", Paths);

[PATCH] D122336: [InstrProfiling] No runtime hook for unused funcs

MaskRay added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:569 + if (!containsProfilingIntrinsics(M)) { +if (!CoverageNamesVar || !NeedsRuntimeHook) { + return MadeChange; https://llvm.org/docs/CodingStandards.html

[PATCH] D122444: [Driver][Linux] Remove D.Dir+"/../lib" from default search paths for LLVM_ENABLE_RUNTIMES builds

This revision was automatically updated to reflect the committed changes. MaskRay marked an inline comment as done. Closed by commit rGafaefb671fe1: [Driver][Linux] Remove D.Dir+"/../lib" from default search paths for… (authored by MaskRay). Repository: rG LLVM Github Monorepo CHANGES SINCE LA

[PATCH] D120639: [RISCV] Pass -mno-relax to assembler when -fno-integrated-as specified

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:764 CmdArgs.push_back(MArchName.data()); +if (!Args.hasFlag(options::OPT_mrelax, options::OPT_mno_relax)) + CmdArgs.push_back("-mno-relax"); Avoid using the error-prone d

[PATCH] D121302: [HIP] Fix -fno-gpu-sanitize

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/HIPAMD.cpp:165 // Diagnose unsupported sanitizer options only once. + if (!Args.hasFlag(options::OPT_fgpu_sanitize, options::OPT_fno_gpu_sanitize)) +return; Note: the third argument of

[PATCH] D122524: [clang][AVR] Generate link warnings properly

MaskRay added inline comments. Comment at: clang/test/Driver/avr-toolchain.c:39 + +// RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree -mmcu=atmega328 2>&1 | FileCheck --check-prefix=CHECK5 %s +// CHECK5-NOT: warning: no target microcontroller specified on comm

[PATCH] D122524: [clang][AVR] Generate link warnings properly

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/AVR.cpp:379 !Args.hasArg(options::OPT_nodefaultlibs) && + !Args.hasArg(options::OPT_S) && !Args.hasArg(options::OPT_c /* does not apply when not linking */)) { This is insuf

[PATCH] D122553: [Driver][AVR] Fix warn_drv_avr_stdlib_not_linked condition

MaskRay created this revision. MaskRay added reviewers: aykevl, benshi001. Herald added subscribers: StephenFan, Jim, dylanmckay. Herald added a project: All. MaskRay requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Many options (-fsyntax-on

[PATCH] D122524: [clang][AVR] Generate link warnings properly

MaskRay added a comment. I think it is excessive to add so many RUN lines. I do not understand much about AVR -mcpu. That said, I created D122553 for what I think should be done for the `-c/-S/-fsyntax-only` condition. More tests would just be excessive. CHA

[PATCH] D122553: [Driver][AVR] Fix warn_drv_avr_stdlib_not_linked condition

MaskRay updated this revision to Diff 418493. MaskRay added a comment. fix file header Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122553/new/ https://reviews.llvm.org/D122553 Files: clang/lib/Driver/ToolChains/AVR.cpp clang/lib/Driver/ToolC

[PATCH] D122556: [RISCV] Add definitions for Xiangshan processors.

MaskRay added inline comments. Comment at: clang/test/CodeGen/RISCV/riscv-metadata.c:18 +// Test if Xiangshan processors work with -target-cpu +// RUN: %clang_cc1 -triple riscv64 -emit-llvm -o - -target-cpu xiangshan-yanqihu %s | FileCheck -check-prefix=EMPTY-LP64 %s -

[PATCH] D122556: [RISCV] Add definitions for Xiangshan processors.

MaskRay added inline comments. Comment at: clang/test/Driver/riscv-cpus.c:146 +// MCPU-XS-NANHU: "-target-feature" "+m" "-target-feature" "+a" "-target-feature" "+f" "-target-feature" "+d" +// MCPU-XS-NANHU: "-target-feature" "+c" "-target-feature" "+zba" "-target-feature" "+zb

[PATCH] D121984: [RISCV][NFC] Moving RVV intrinsic type related util to llvm/Support

MaskRay added a comment. Seems that there is a circular dependency problem: 7c7e7770b7176f452d40f4e7ac545aaee1b19c4d (reverted). I'd suggest that we revert this patch. Having target specific stuff in LLVMSupport is always f

[PATCH] D121984: [RISCV][NFC] Moving RVV intrinsic type related util to llvm/Support

MaskRay added a comment. Ah, sorry, I did not see that the end state of @akuegel's 3 commits fixed the circular dependency. However, I think my other concern still applies: the layering seems weird that the RISCV stuff is placed in LLVMSupport. Any chance it can be fixed? You may consider a mor

[PATCH] D121984: [RISCV][NFC] Moving RVV intrinsic type related util to llvm/Support

MaskRay added a comment. The original patch added 600 lines while the subject just said "Move". This may be fishy as well. Having TableGen related `void RVVIntrinsic::emitCodeGenSwitchBody(raw_ostream &OS) const {` in LLVMSupport is also weird. So I think this patch deserves more discussion an

[PATCH] D122592: [OpenMP] Fix library path missing when using OpenMP

MaskRay accepted this revision. MaskRay added a comment. Another idea is to refactor `tools::addOpenMPRuntime` to use the full path of `libomp.so` or `libomp.a` instead of `-LXXX -lomp`. The two ways have identical semantics when `libomp.so` has a `DT_SONAME` tag. In the long term, perhaps libom

[PATCH] D122553: [Driver][AVR] Fix warn_drv_avr_stdlib_not_linked condition

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG16524d2f1bdc: [Driver][AVR] Fix warn_drv_avr_stdlib_not_linked condition (authored by MaskRay). Changed prior to commit: https://reviews.llvm.org/

[PATCH] D122553: [Driver][AVR] Fix warn_drv_avr_stdlib_not_linked condition

MaskRay added inline comments. Comment at: clang/lib/Driver/ToolChains/AVR.cpp:451 + !Args.hasArg(options::OPT_nodefaultlibs)) { +if (CPU.empty()) { + // We cannot link any standard libraries without an MCU specified. benshi001 wrote: > I think we s

[PATCH] D122588: [RISCV] Make PATH empty when testing --gcc-toolchain is multilib_riscv_elf_sdk

MaskRay added a comment. Thanks for the patch. I think it's correct, though I'd add `[test]` to indicate this patch is test only and therefore not change any code. If the test no longer works on Windows, add `// UNSUPPORTED: system-windows` Does riscv32-toolchain.c need a similar change? Please

[PATCH] D122592: [OpenMP] Fix library path missing when using OpenMP

MaskRay added a comment. In D122592#3411795 , @ye-luo wrote: >> using the multiarch directory > > If we can cross compile libomp and libomptarget to the target system. We may > have > lib/x86_64-unknown-linux-gnu/libomp.so > lib/aarch64-unknown-linux-gnu

[PATCH] D122592: [OpenMP] Fix library path missing when using OpenMP

MaskRay added inline comments. Comment at: clang/test/OpenMP/library-path.c:1 +// RUN: %clang -### -fopenmp=libomp -v %s 2>&1 | FileCheck %s + Suggest the style in `linux-cross.cpp`: use `-ccc-install-dir %S/Inputs/basic_linux_tree/usr/bin` to specify InstalledD

[PATCH] D122592: [OpenMP] Fix library path missing when using OpenMP

MaskRay added inline comments. Comment at: clang/test/OpenMP/library-path.c:1 +// RUN: %clang -### -fopenmp=libomp -v %s 2>&1 | FileCheck %s + MaskRay wrote: > Suggest the style in `linux-cross.cpp`: use `-ccc-install-dir > %S/Inputs/basic_linux_tree/usr/bin` to

[PATCH] D122524: [clang][AVR] Emit proper warnings

MaskRay added a comment. In D122524#3411872 , @benshi001 wrote: > In D122524#3410542 , @MaskRay wrote: > >> I think it is excessive to add so many RUN lines. I do not understand much >> about AVR -mcpu. That said

<    3   4   5   6   7   8   9   10   11   12   >