[PATCH] D159541: [UEFI] X86_64 UEFI Clang Driver

phosek accepted this revision. phosek 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/D159541/new/ https://reviews.llvm.org/D159541

[PATCH] D157913: [Coverage] Allow Clang coverage to be used with debug info correlation.

phosek added a comment. We've been investigating a serious performance regression in our latest Clang roll that is affecting coverage. Whereas previously, we could process all coverage data in under an hour, now the job times out after 6 hours. After investigation, I found out that this change

[PATCH] D152206: [Basic] Support 64-bit x86 target for UEFI

phosek accepted this revision. phosek 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/D152206/new/ https://reviews.llvm.org/D152206

[PATCH] D152206: [Basic] Support 64-bit x86 target for UEFI

phosek added a comment. Is it possible to abandon this change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152206/new/ https://reviews.llvm.org/D152206 ___ cfe-commits mailing list

[PATCH] D159541: [UEFI] X86_64 UEFI Clang Driver

phosek added inline comments. Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:58-59 + + // Other UEFI binary subsystems that are currently unsupported: + // efi_boot_service_driver, efi_rom, efi_runtime_driver. + CmdArgs.push_back("-subsystem:efi_application");

[PATCH] D152206: [Basic] Support 64-bit x86 target for UEFI

phosek added a comment. Would it be possible to split up the LLVM changes into another change so we can land it separately from the Clang support? Comment at: llvm/lib/Target/X86/X86Subtarget.h:337 - bool isTargetWin64() const { return Is64Bit && isOSWindows(); } + bool

[PATCH] D158476: [driver] Search for compatible Android runtime directories

phosek accepted this revision. phosek 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/D158476/new/ https://reviews.llvm.org/D158476

[PATCH] D159292: [driver] Conditionally include installed libc++ headers for Android

phosek accepted this revision. phosek 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/D159292/new/ https://reviews.llvm.org/D159292

[PATCH] D157913: [Coverage] Allow Clang coverage to be used with debug info correlation.

phosek added inline comments. Comment at: llvm/lib/ProfileData/InstrProfCorrelator.cpp:42 +const char *InstrProfCorrelator::CovFunctionNameAttributeName = +"Cov Function Name"; I missed this earlier, but I think that spelling this out in full, that is

[PATCH] D152405: [WIP][clang] Add experimental option to omit the RTTI component from the vtable when -fno-rtti is used

phosek accepted this revision. phosek added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/include/clang/Basic/LangOptions.def:453 +LANGOPT(OmitRTTIComponent, 1, 0, +"Use an ABI-incompatible v-table layout that omits the RTTI

[PATCH] D152405: [WIP][clang] Add experimental option to omit the RTTI component from the vtable when -fno-rtti is used

phosek added inline comments. Comment at: clang/include/clang/Driver/Options.td:2249-2256 +def fexperimental_omit_rtti_component : + Flag<["-"], "fexperimental-omit-rtti-component">, + Group, Flags<[CC1Option]>, + HelpText<"Omit the RTTI component from virtual tables">; +def

[PATCH] D157913: [Coverage] Allow Clang coverage to be used with debug info correlation.

phosek added a comment. In D157913#4626007 , @zequanwu wrote: >> It seems that the `__llvm_prf_names` is retained in this mode. What is the >> overhead of this section generally? Can we instead use debug info to lookup >> function names? > > With debug

[PATCH] D107115: [CMake][Fuchsia] Use standalone unwinder in Fuchsia toolchain

phosek abandoned this revision. phosek added a comment. Herald added subscribers: ekilmer, abrachet. Herald added a project: All. Superseded by D127887 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107115/new/

[PATCH] D159293: [driver] Perform fallback target searches for stdlib

phosek accepted this revision. phosek 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/D159293/new/ https://reviews.llvm.org/D159293

[PATCH] D158572: [clang][modules] Use relative offsets for input files

phosek added a comment. In D158572#4615234 , @phosek wrote: > In D158572#4615231 , @jansvoboda11 > wrote: > >> @phosek BTW can you confirm whether these started failing with this patch or >> with D158573

[PATCH] D158822: [Fuchsia] Make __start_* / __stop_* symbols hidden

This revision was automatically updated to reflect the committed changes. Closed by commit rG53ee31c9871d: [Fuchsia] Make __start_* / __stop_* symbols hidden (authored by phosek). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158822/new/

[PATCH] D158822: [Fuchsia] Make __start_* / __stop_* symbols hidden

phosek updated this revision to Diff 553404. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158822/new/ https://reviews.llvm.org/D158822 Files: clang/lib/Driver/ToolChains/Fuchsia.cpp clang/test/Driver/fuchsia.c clang/test/Driver/fuchsia.cpp

[PATCH] D158822: [Fuchsia] Make __start_* / __stop_* symbols hidden

phosek updated this revision to Diff 553403. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158822/new/ https://reviews.llvm.org/D158822 Files: clang/lib/Driver/ToolChains/Fuchsia.cpp clang/test/Driver/fuchsia.c Index:

[PATCH] D158822: [Fuchsia] Make __start_* / __stop_* symbols hidden

phosek updated this revision to Diff 553402. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158822/new/ https://reviews.llvm.org/D158822 Files: clang/lib/Driver/ToolChains/Fuchsia.cpp Index: clang/lib/Driver/ToolChains/Fuchsia.cpp

[PATCH] D158822: [Fuchsia] Make __start_* / __stop_* symbols hidden

phosek created this revision. phosek added a reviewer: mcgrathr. Herald added a subscriber: abrachet. Herald added a project: All. phosek requested review of this revision. Herald added subscribers: cfe-commits, MaskRay. Herald added a project: clang. This is the intended semantics, see for more

[PATCH] D158475: [driver] Refactor getRuntimePaths. NFC

phosek accepted this revision. phosek 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/D158475/new/ https://reviews.llvm.org/D158475

[PATCH] D158572: [clang][modules] Use relative offsets for input files

phosek added a comment. In D158572#4615231 , @jansvoboda11 wrote: > @phosek BTW can you confirm whether these started failing with this patch or > with D158573 ? I'm trying to do a local build now to find out.

[PATCH] D158572: [clang][modules] Use relative offsets for input files

phosek added a comment. After this change, all libc++ `clang_modules_include.gen.py` tests started failing on our Linux builders: Script: -- : 'COMPILED WITH'; /b/s/w/ir/x/w/llvm_build/./bin/clang++

[PATCH] D140925: [CMake] Use Clang to infer the target triple

phosek added a comment. SGTM this was previously blocked on other changes, but those all landed so I can go ahead, rebase and land it today. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140925/new/ https://reviews.llvm.org/D140925

[PATCH] D157879: [clang] Report missing designated initializers in C++

phosek added a comment. Note that there's an ongoing discussion on the GCC bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96868 whether `-Wmissing-field-initializers` should apply to both C and C++ or just C. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D155290: [PGO] Use Unique Profile Files when New Processes are Forked

phosek added a comment. Can we avoid the use of `pthread_atfork` and the dependency on pthreads? Using pthreads inside the profile runtime implementation is problematic in the case where you want to instrument the libc itself which is used on some platforms and it's one of the reasons why we

[PATCH] D136664: [CMake] Support building crt with the bootstrapping build

phosek abandoned this revision. phosek added a comment. Herald added a subscriber: ekilmer. Superseded by D153989 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136664/new/ https://reviews.llvm.org/D136664

[PATCH] D157149: [Option] Add "Visibility" field and clone the OptTable APIs to use it

phosek added inline comments. Comment at: llvm/include/llvm/Option/OptParser.td:153 class Flags flags> { list Flags = flags; } +class Vis vis> { list Vis = vis; } class Group { OptionGroup Group = group; } Would it be possible to spell this out in full, that

[PATCH] D156363: [Driver] -###: exit with code 1 if hasErrorOccurred

phosek added a comment. We see 14 failing tests on our Linux and macOS builders, and 6 failing tests on Windows builders: - `Clang :: Driver/csky-toolchain.c` - `Clang :: Driver/env.c` - `Clang :: Driver/fsanitize.c` - `Clang :: Driver/gcc-install-dir.cpp` - `Clang :: Driver/gcc-toolchain.cpp`

[PATCH] D150946: [clang][Interp] PointerToIntegral casts

phosek added a comment. In D150946#4538469 , @aaron.ballman wrote: > In D150946#4536971 , @phosek wrote: > >> This test is failing on our Windows builders with the following error: >> >> Script: >> -- >> :

[PATCH] D150946: [clang][Interp] PointerToIntegral casts

phosek added a comment. This test is failing on our Windows builders with the following error: Script: -- : 'RUN: at line 1'; c:\b\s\w\ir\x\w\llvm_build\bin\clang.exe -cc1 -internal-isystem c:\b\s\w\ir\x\w\llvm_build\lib\clang\18\include -nostdsysteminc

[PATCH] D155337: [CMake] Include riscv32-unknown-elf target in Fuchsia toolchain

phosek marked an inline comment as done. phosek added inline comments. Comment at: clang/cmake/caches/Fuchsia-stage2.cmake:10 set(_FUCHSIA_ENABLE_PROJECTS "bolt;clang;clang-tools-extra;lld;llvm;polly") -set(LLVM_ENABLE_RUNTIMES "compiler-rt;libcxx;libcxxabi;libunwind" CACHE

[PATCH] D155337: [CMake] Include riscv32-unknown-elf target in Fuchsia toolchain

This revision was automatically updated to reflect the committed changes. Closed by commit rG28f5322770a1: [CMake] Include riscv32-unknown-elf runtimes in Fuchsia toolchain (authored by phosek). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D143617: [Clang][CMake] Support perf, LBR, and Instrument CLANG_BOLT options

phosek added inline comments. Comment at: clang/utils/perf-training/bolt.lit.cfg:12 +if config.clang_bolt_mode.lower() == "instrument": +clang_binary = "clang-bolt.inst" +else: # perf or LBR This name would ideally be passed through the generated

[PATCH] D155337: [CMake] Include riscv32-unknown-elf target in Fuchsia toolchain

phosek created this revision. Herald added subscribers: ekilmer, VincentWu, abrachet, vkmr, sameer.abuasal, s.egerton, Jim, benna, psnobl, abidh, rogfer01, shiva0217, kito-cheng, simoncook, asb, arichardson. Herald added a project: All. phosek requested review of this revision. Herald added

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

phosek added inline comments. Comment at: flang-rt/CMakeLists.txt:17-23 +# Check if flang-rt is built as a standalone project. +if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR OR FLANG_RT_STANDALONE_BUILD) + project(FlangRT C CXX) + set(CMAKE_INCLUDE_CURRENT_DIR ON) +

[PATCH] D153989: [compiler-rt] Move crt into builtins

phosek added a comment. In D153989#4494278 , @phosek wrote: > Thanks for this heads up, this should be addressed in > rG926f3759ec62a8f170e76a60316cc0bdd9dd2ec9 > . That didn't work,

[PATCH] D153989: [compiler-rt] Move crt into builtins

phosek added a comment. In D153989#4494012 , @nathanchance wrote: > This change appears to break `-DCOMPILER_RT_BUILD_CRT=OFF`: > > $ cmake -B build -S llvm -DCMAKE_BUILD_TYPE=Release > -DCOMPILER_RT_BUILD_CRT=OFF

[PATCH] D153989: [compiler-rt] Move crt into builtins

phosek added a comment. In D153989#4492229 , @arichardson wrote: > I think reduplication of build logic makes sense but these are conceptually > separate things. The compiler rt builtins are needed in many cases where the > C start-up code isn't (e.g.

[PATCH] D153989: [compiler-rt] Move crt into builtins

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. phosek marked an inline comment as done. Closed by commit rGdae9d1b52469: [compiler-rt] Move crt into builtins (authored by phosek). Changed prior to commit:

[PATCH] D154869: [Flang] [FlangRT] Implement FlangRT library as solution to Flang's runtime LLVM integration

phosek added inline comments. Comment at: flang-rt/CMakeLists.txt:17-23 +# Check if flang-rt is built as a standalone project. +if (CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR OR FLANG_RT_STANDALONE_BUILD) + project(FlangRT C CXX) + set(CMAKE_INCLUDE_CURRENT_DIR ON) +

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

phosek added inline comments. Comment at: clang/lib/Driver/ToolChains/Serenity.h:38 + +class LLVM_LIBRARY_VISIBILITY Serenity : public ToolChain { +public: Have you considered inheriting from `Generic_GCC` to allow more code reuse? Repository: rG LLVM Github

[PATCH] D143052: [CMake] Replace llvm_check_linker_flag and llvm_check_compiler_linker_flag

phosek added a comment. In D143052#4468485 , @ldionne wrote: > What is the status of this? We now require CMake 3.20, can this be rebased > and landed in some form? The issue I ran into is that `check_linker_flag` annoyingly behaves differently from

[PATCH] D153989: [compiler-rt] Move crt into builtins

phosek added a comment. In D153989#4465759 , @smeenai wrote: > Sorry, it's been a week :D I assume that crt doesn't need the builtins to be > available for its configure (the way the rest of compiler-rt does)? If so, > this LGTM. Great question, I

[PATCH] D153989: [compiler-rt] Move crt into builtins

phosek added a comment. @smeenai Do you have any opinion on this change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153989/new/ https://reviews.llvm.org/D153989 ___ cfe-commits mailing list

[PATCH] D154014: [SpecialCaseList] Use Globs instead of Regex

phosek added a comment. If we want to support both regular expressions and glob patterns permanently, then a solution like `#!regex` or `#!glob` is likely the way to go. If we want to allow soft-transition, then we could do something like `#!special-case-list-v2`, where v1 would support

[PATCH] D152206: [Basic] Support 64-bit x86 target for UEFI

phosek added a comment. For comments that are already address, it's customary to mark those as "Done" to make it clear. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152206/new/ https://reviews.llvm.org/D152206

[PATCH] D152206: [Basic] Support 64-bit x86 target for UEFI

phosek added inline comments. Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:57 + CmdArgs.push_back("-nologo"); + + if (Args.hasArg(options::OPT_g_Group, options::OPT__SLASH_Z7)) We should be also passing `-subsystem:efi_application`. Note that COFF also has

[PATCH] D153989: [compiler-rt] Move crt into builtins

phosek updated this revision to Diff 535452. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153989/new/ https://reviews.llvm.org/D153989 Files: compiler-rt/CMakeLists.txt compiler-rt/cmake/builtin-config-ix.cmake compiler-rt/lib/CMakeLists.txt

[PATCH] D153989: [compiler-rt] Move crt into builtins

phosek created this revision. phosek added reviewers: smeenai, beanz. Herald added subscribers: Enna1, abrachet, dberris. Herald added a project: All. phosek requested review of this revision. Herald added projects: clang, Sanitizers. Herald added subscribers: Sanitizers, cfe-commits. On Linux

[PATCH] D153931: [Fuchsia] Enable libcxx filesystem on Windows build.

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

[PATCH] D148869: [Driver][BareMetal] Support --emit-static-lib in BareMetal driver

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. phosek marked an inline comment as done. Closed by commit rG5614d1a0a10d: [Driver][BareMetal] Support --emit-static-lib in BareMetal driver (authored by phosek).

[PATCH] D148869: [Driver][BareMetal] Support --emit-static-lib in BareMetal driver

phosek marked an inline comment as done. phosek added inline comments. Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:170 bool BareMetal::handlesTarget(const llvm::Triple ) { return isARMBareMetal(Triple) || isAArch64BareMetal(Triple) ||

[PATCH] D152762: [clang][docs] Update SanitizerSpecialCaseList docs

phosek added a comment. In D152762#4421702 , @ellis wrote: > In D152762#4421630 , @phosek wrote: > >> This is unrelated to this change but related to the issue this change is >> addressing. The use of regular

[PATCH] D152762: [clang][docs] Update SanitizerSpecialCaseList docs

phosek added a comment. This is unrelated to this change but related to the issue this change is addressing. The use of regular expressions for special case list with the special handling of `*` is error-prone, I've seen many people having issues it. Furthermore, regular expression for

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

phosek accepted this revision. phosek 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/D152604/new/ https://reviews.llvm.org/D152604

[PATCH] D152473: [clang-format] Add test case for issue 63170

phosek added a comment. I understand the concerns and I apologize if my LGTM came out as disrespectful, but there has been an issue reported with the original change over two days ago, including a reproducer, and given given that this issue

[PATCH] D152473: [clang-format] Add test case for issue 63170

phosek accepted this revision. phosek 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/D152473/new/ https://reviews.llvm.org/D152473

[PATCH] D143587: [Docs] Multilib design

phosek accepted this revision. phosek added a comment. LGTM Comment at: clang/docs/Multilib.rst:231 + +However, an exception is the normalization of -march. +-march for Arm architectures contains a list of enabled and disabled extensions Use backticks to

[PATCH] D152418: [clang] set python3 as required build dependency

This revision was automatically updated to reflect the committed changes. Closed by commit rG96962d5512fb: [clang] set python3 as required build dependency (authored by phosek). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152418/new/

[PATCH] D143075: BareMetal ToolChain multilib layering

phosek accepted this revision. phosek added a comment. LGTM Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:236-240 + if (!SelectedMultilibs.empty()) { +return llvm::iterator_range< +llvm::SmallVector::const_reverse_iterator>( +

[PATCH] D152353: [NFC][Driver] Change MultilibBuilder flags argument order

phosek accepted this revision. phosek added a comment. This revision is now accepted and ready to land. LGTM thanks! Comment at: clang/include/clang/Driver/MultilibBuilder.h:80 + /// \p Negate defines whether the flag is negated and therefore disallowed. + MultilibBuilder

[PATCH] D142932: Multilib YAML parsing

phosek accepted this revision. phosek added a comment. In D142932#4402981 , @michaelplatings wrote: > @phosek thanks for your suggestion, that's now implemented. In practise for > LLVM Embedded Toolchain for Arm we haven't yet needed `NoMatchFlags` so

[PATCH] D143059: [Driver] Enable selecting multiple multilibs

phosek accepted this revision. phosek added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143059/new/ https://reviews.llvm.org/D143059 ___ cfe-commits mailing list

[PATCH] D142986: Enable multilib.yaml in the BareMetal ToolChain

phosek accepted this revision. phosek added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142986/new/ https://reviews.llvm.org/D142986 ___ cfe-commits mailing list

[PATCH] D142933: [Driver] Add -print-multi-flags-experimental option

phosek accepted this revision. phosek added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142933/new/ https://reviews.llvm.org/D142933 ___ cfe-commits mailing list

[PATCH] D152418: [clang] set python3 as required build dependency

phosek accepted this revision. phosek 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/D152418/new/ https://reviews.llvm.org/D152418

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

phosek added a comment. We're planning to default to `-msmall-data-limit=0` for Android and Fuchsia so I'm supportive of this change because it means less complexity and fewer differences between platforms. I think it would be worth bringing up this topic at the RISC-V LLVM sync-up call

[PATCH] D152206: [Basic] Support 64-bit x86 target for UEFI

phosek added a comment. This needs a lit test. Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:92-97 + StringRef Linker = + Args.getLastArgValue(options::OPT_fuse_ld_EQ, CLANG_DEFAULT_LINKER); + if (Linker.empty() || Linker.equals_insensitive("lld")) +Linker =

[PATCH] D143075: BareMetal ToolChain multilib layering

phosek added inline comments. Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:242-246 + // No multilibs selected so return a single default multilib. + static llvm::SmallVector Default = {Multilib()}; + return llvm::iterator_range< +

[PATCH] D142986: Enable multilib.yaml in the BareMetal ToolChain

phosek added inline comments. Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:176 + +#define MULTILIB_YAML_FILENAME "multilib.yaml" + This is not idiomatic, I couldn't find any instance of `#define` for string literals anywhere in LLVM. I believe the

[PATCH] D142933: Add -print-multi-selection-flags-experimental option

phosek added inline comments. Comment at: clang/include/clang/Driver/Options.td:4261 def print_multi_lib : Flag<["-", "--"], "print-multi-lib">; +def print_multi_selection_flags : Flag<["-", "--"], "print-multi-selection-flags-experimental">, + HelpText<"Print the flags used

[PATCH] D142933: Add -print-multi-selection-flags-experimental option

phosek added inline comments. Comment at: clang/lib/Driver/ToolChain.cpp:250-285 + // Enumerate boolean flags we care about for the purposes of multilib here. + // There must be a smarter way to do it but this gets us started. + const struct HasFlag { +ID Pos, Neg; +

[PATCH] D142932: Multilib YAML parsing

phosek added a comment. The only remaining concern I have is about the YAML schema, specifically the naming of `Regex`, `MatchFlags` and `NoMatchFlags` fields. Specifically, I think it something like this might be cleaner: FlagMap: - Match:

[PATCH] D151437: [NFC][Driver] Change MultilibBuilder interface

phosek added inline comments. Comment at: clang/include/clang/Driver/MultilibBuilder.h:80 + /// \p Flag must be a flag accepted by the driver. + MultilibBuilder (bool Required, StringRef Flag); I think it would be cleaner to swap the order of arguments and

[PATCH] D152318: [Fuchsia] Add llvm-strings to Fuchsia clang build

phosek accepted this revision. phosek 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/D152318/new/ https://reviews.llvm.org/D152318

[PATCH] D131594: [Triple] Add triple for UEFI

phosek accepted this revision. phosek added a comment. This revision is now accepted and ready to land. Can you also provide description for this change (this should be the same you'll use for the Git message). Comment at: llvm/include/llvm/MC/TargetRegistry.h:568 +

[PATCH] D131594: Add Clang UEFI target to support "x86_64-unknown-uefi" triple

phosek added a comment. I'd omit the lit test cases for now since they're not necessary. There should be a test case in https://github.com/llvm/llvm-project/blob/86b6ac5d54d7fe5cd21beff64b5c2194b1368bdf/llvm/unittests/TargetParser/TripleTest.cpp though. Repository: rG LLVM Github Monorepo

[PATCH] D131594: WORK IN PROGRESS Add Clang UEFI target to support "x86_64-unknown-uefi" triple

phosek added a comment. I'd split the `llvm/` changes into a separate change. Comment at: clang/lib/Driver/ToolChains/UEFI.cpp:58-59 + !C.getDriver().IsCLMode() && !C.getDriver().IsFlangMode()) { +CmdArgs.push_back("-defaultlib:libcmt"); +

[PATCH] D152057: [CMake][Fuchsia] Add LLVM_ENABLE_HTTPLIB to Stage 2 build

phosek accepted this revision. phosek 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/D152057/new/ https://reviews.llvm.org/D152057

[PATCH] D151595: [BOLT][CMake] Redo the build and install targets

This revision was automatically updated to reflect the committed changes. Closed by commit rGf3269a94e729: [BOLT][CMake] Redo the build and install targets (authored by phosek). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151595/new/

[PATCH] D151595: [BOLT][CMake] Redo the build and install targets

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf99a7d3e3809: [BOLT][CMake] Redo the build and install targets (authored by phosek). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D151595: [BOLT][CMake] Redo the build and install targets

phosek marked 2 inline comments as done. phosek added inline comments. Comment at: bolt/test/CMakeLists.txt:40 llvm-bolt - llvm-boltdiff llvm-bolt-heatmap Amir wrote: > We have a number of dependencies on llvm-boltdiff and perf2bolt, e.g. in > internal

[PATCH] D151595: [BOLT][CMake] Redo the build and install targets

phosek updated this revision to Diff 527243. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151595/new/ https://reviews.llvm.org/D151595 Files: bolt/CMakeLists.txt bolt/cmake/modules/AddBOLT.cmake bolt/test/CMakeLists.txt

[PATCH] D151595: [BOLT][CMake] Redo the build and install targets

phosek added a comment. @Amir does this change look good to you? Comment at: bolt/tools/heatmap/CMakeLists.txt:8 -add_bolt_tool(llvm-bolt-heatmap +add_bolt_executable(llvm-bolt-heatmap heatmap.cpp smeenai wrote: > Why this change? This tool wasn't

[PATCH] D151595: [BOLT][CMake] Redo the build and install targets

phosek updated this revision to Diff 527084. phosek marked an inline comment as done. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151595/new/ https://reviews.llvm.org/D151595 Files: bolt/CMakeLists.txt bolt/cmake/modules/AddBOLT.cmake

[PATCH] D151595: [BOLT][CMake] Redo the build and install targets

phosek updated this revision to Diff 526948. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151595/new/ https://reviews.llvm.org/D151595 Files: bolt/CMakeLists.txt

[PATCH] D150939: [CMake] Remove BOLT from Fuchsia toolchain

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGaf4da3d74630: [CMake] Remove BOLT from Fuchsia toolchain (authored by phosek). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D150939: [CMake] Remove BOLT from Fuchsia toolchain

phosek updated this revision to Diff 526836. phosek added a reviewer: haowei. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150939/new/ https://reviews.llvm.org/D150939 Files: clang/cmake/caches/Fuchsia-stage2.cmake Index:

[PATCH] D151186: [Driver] Properly handle -pie and -nopie on Fuchsia

phosek added a comment. In D151186#4369203 , @MaskRay wrote: > In D151186#4369181 , @phosek wrote: > >> That alias is already recognized by the GNU driver. I'm sharing the >> implementation between the two

[PATCH] D151186: [Driver] Properly handle -pie and -nopie on Fuchsia

phosek added a comment. That alias is already recognized by the GNU driver. I'm sharing the implementation between the two drivers. Would you prefer to reject `-nopie` when targeting Fuchsia? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D143075: BareMetal ToolChain multilib layering

phosek added inline comments. Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:228 + +llvm::SmallVector BareMetal::getOrderedMultilibs() const { + // Get multilibs in reverse order because they're ordered most-specific last. Can you use

[PATCH] D142986: Enable multilib.yaml in the BareMetal ToolChain

phosek added inline comments. Comment at: clang/lib/Driver/ToolChains/BareMetal.cpp:180 + SmallString<128> SysRootDir(D.Dir); + llvm::sys::path::append(SysRootDir, "../lib/clang-runtimes"); + Defer to `llvm::sys::path::append` for joining the path components

[PATCH] D142932: Multilib YAML parsing

phosek added inline comments. Comment at: clang/include/clang/Driver/Multilib.h:150-152 + bool parseYaml(llvm::MemoryBufferRef, + llvm::SourceMgr::DiagHandlerTy = nullptr, + void *DiagHandlerCtxt = nullptr); Rather than

[PATCH] D146757: [Driver] Enable defining multilib flags verbatim

phosek added a comment. I apologize about the delayed response. I had time to think about this a bit more and it's not really clear to me if we need to preserve the "plus/minus" flag syntax. Looking through the history, it seems that it was introduced in D2538

[PATCH] D151186: [Driver] Properly handle -pie and -nopie on Fuchsia

phosek updated this revision to Diff 524602. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151186/new/ https://reviews.llvm.org/D151186 Files: clang/lib/Driver/ToolChains/CommonArgs.cpp clang/lib/Driver/ToolChains/CommonArgs.h

[PATCH] D151186: [Driver] Properly handle -pie and -nopie on Fuchsia

phosek updated this revision to Diff 524601. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151186/new/ https://reviews.llvm.org/D151186 Files: clang/lib/Driver/ToolChains/CommonArgs.cpp clang/lib/Driver/ToolChains/CommonArgs.h

[PATCH] D151186: [Driver] Properly handle -pie and -nopie on Fuchsia

phosek updated this revision to Diff 524599. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151186/new/ https://reviews.llvm.org/D151186 Files: clang/lib/Driver/ToolChains/CommonArgs.cpp clang/lib/Driver/ToolChains/CommonArgs.h

[PATCH] D151186: [Driver] Properly handle -pie and -nopie on Fuchsia

phosek created this revision. phosek added a reviewer: phosek. Herald added a subscriber: abrachet. Herald added a project: All. phosek requested review of this revision. Herald added subscribers: cfe-commits, MaskRay. Herald added a project: clang. Prior to this patch we would ignore -pie and

[PATCH] D150752: [BOLT][CMake] Use correct output paths and passthrough necessary options

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9e6e3375f12e: [BOLT][CMake] Use correct output paths and passthrough necessary options (authored by phosek). Repository: rG LLVM Github Monorepo

  1   2   3   4   5   6   7   8   9   10   >