[PATCH] D77989: Allow disabling of vectorization using internal options

This revision was automatically updated to reflect the committed changes. Closed by commit rG33ffb62e23e7: Allow disabling of vectorization using internal options (authored by tejohnson). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77989/new/

[PATCH] D77058: [Clang] Add llvm.loop.unroll.disable to loops with -fno-unroll-loops.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. yep, LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77058/new/ https://reviews.llvm.org/D77058

[PATCH] D77484: [Vector] Pass VectLib to LTO backend so TLI build correct vector function list

tejohnson added a comment. In D77484#1965629 , @wenlei wrote: > > Ok then it does sound like these could be handled on a per-function basis, > > similar to how -fno-builtin* are handled. I.e. a function attribute to > > indicate the veclib, which would

[PATCH] D77632: [TLI] Per-function fveclib for math library used for vectorization

tejohnson added a comment. In D77632#1975619 , @mehdi_amini wrote: > The existing TLI provides a very convenient way to define a VecLib without > LLVM knowing about it ahead of time. This feature is important for any > embedded use of LLVM as a library

[PATCH] D77952: [TLI] Reduce copies for TLI and TLA

tejohnson added a comment. Some parts of this are dependent on the patch that got reverted, but I have some other questions below about the changes in BackendUtil.cpp. Comment at: clang/lib/CodeGen/BackendUtil.cpp:689 // Set up the per-function pass manager. - FPM.add(new

[PATCH] D77632: [TLI] Per-function fveclib for math library used for vectorization

tejohnson added a comment. In D77632#1976240 , @wenlei wrote: > In D77632#1974409 , @tejohnson wrote: > > > In D77632#1974363 , @wenlei wrote: > > > > > In D77632#1974015

[PATCH] D77952: [TLI] Reduce copies for TLI and TLA

tejohnson added a comment. Also, just a nit, because TLI is sometimes used to refer to the TargetLibraryInfo and occasionally to the TargetLibraryInfoImpl, and the latter is frequently referred to as TLII, could you change the description to say TLII or TLI Impl? Since this doesn't affect

[PATCH] D77952: [TLI] Reduce copies for TLI and TLA

tejohnson added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:689 // Set up the per-function pass manager. - FPM.add(new TargetLibraryInfoWrapperPass(*TLII)); + FPM.add(new TargetLibraryInfoWrapperPass(TargetTriple)); if (CodeGenOpts.VerifyModule)

[PATCH] D77989: Allow disabling of vectorization using internal options

tejohnson created this revision. tejohnson added reviewers: fhahn, wmi. Herald added subscribers: cfe-commits, dexonsmith, steven_wu, hiraditya. Herald added a project: clang. Currently, the internal options -vectorize-loops, -vectorize-slp, and -interleave-loops do not have much practical

[PATCH] D77952: [TLII] Reduce copies of TLII for TLA

tejohnson added a comment. lgtm with the VecLibDesc references removed. Comment at: llvm/lib/Analysis/TargetLibraryInfo.cpp:596 memcpy(AvailableArray, TLI.AvailableArray, sizeof(AvailableArray)); return *this; } wenlei wrote: > tejohnson wrote: > > This

[PATCH] D77925: Revert "[TLI] Per-function fveclib for math library used for vectorization"

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

[PATCH] D77484: [Vector] Pass VectLib to LTO backend so TLI build correct vector function list

tejohnson added a subscriber: gchatelet. tejohnson added a comment. We're trying to move towards encoding all of this in the IR. And in fact, I recently implemented a series of patches to make the TLI to be built per-function, and along with some patches from @gchatelet to encode -fno-builtin*

[PATCH] D77484: [Vector] Pass VectLib to LTO backend so TLI build correct vector function list

tejohnson added a comment. In D77484#1962667 , @wenlei wrote: > > Linking against two vectlibs may cause name conflicts or other issues. > > Of all three supported match libraries, all functions from Accelerate are > prefixed with `v`; all MASS library

[PATCH] D77484: [Vector] Pass VectLib to LTO backend so TLI build correct vector function list

tejohnson added a comment. In D77484#1962581 , @hoyFB wrote: > In D77484#1962445 , @tejohnson wrote: > > > We're trying to move towards encoding all of this in the IR. And in fact, I > > recently implemented a

[PATCH] D77058: [Clang] Add llvm.loop.unroll.disable to loops with -fno-unroll-loops.

tejohnson added a comment. I think this is a good approach, rather than a per-function attribute, since as mentioned this will be preserved through inlining. @dexonsmith, does that seem reasonable to you? I missed the original patch and agree with you that we don't want to fix this in LTO by

[PATCH] D78035: [PoC][RISCV] enable LTO/ThinLTO on RISCV

tejohnson added inline comments. Comment at: llvm/test/LTO/RISCV/mabi-invalid.ll:10 +; RUN: opt -module-summary -o %t2.summary %S/Inputs/foo_ilp32.ll +; RUN: llvm-lto2 run -r %t2.summary,foo,plx -r %t1.summary,main,plx -r %t1.summary,foo, -o %t3 %t1.summary %t2.summary 2>&1 |

[PATCH] D77925: Revert "[TLI] Per-function fveclib for math library used for vectorization"

tejohnson added a comment. In D77925#2016299 , @wenlei wrote: > @mehdi_amini @tejohnson When can we re-land this (with tweaks)? I'm under the > impression that a test case demonstrating the 3rd party usage will be added > very soon after this revert,

[PATCH] D77989: Allow disabling of vectorization using internal options

tejohnson added a comment. In D77989#1979772 , @wmi wrote: > In D77989#1979725 , @wmi wrote: > > > The patch makes the reasoning about when the internal flags will take > > effect much easier. > > > > One question

[PATCH] D75655: [Docs] Document -lto-whole-program-visibility

tejohnson added a comment. In D75655#1920036 , @evgeny777 wrote: > @pcc > > > That case seems somewhat questionable to me. If the symbols are being > > exported, it is presumably for the purpose of allowing the symbols to be > > used outside of the

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

tejohnson marked an inline comment as done. tejohnson added inline comments. Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73 c1->f(); - // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2" + // ITANIUM: type.test{{.*}}!"_ZTS2C2" // MS: type.test{{.*}}!"?AUC2@@"

[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

tejohnson updated this revision to Diff 251893. tejohnson added a comment. Includes fixe for 2-stage clang bootstrap test failures and an expanded fix for Chromium issue, plus new tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73242/new/

[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

tejohnson marked 2 inline comments as done. tejohnson added a comment. PTAL Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:1999 +WholeProgramDevirtResolution *Res = nullptr; +if (ExportSummary && isa(S.first.TypeID)) + // Create the type id summary

[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

tejohnson reopened this revision. tejohnson added a comment. This revision is now accepted and ready to land. Updating with fixes since this was reverted in 80bf137fa132ea33204e98bbefa924afe9258a4e . Repository: rG LLVM

[PATCH] D86958: [Docs] Add/update release notes for D71913 (LTO WPD changes)

tejohnson added a comment. In D86958#2274040 , @hans wrote: > Please go ahead and commit to the branch. Sorry, I thought I already did! Just pushed the commit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D87636: [ThinLTO] add post-thinlto-merge option to -lto-embed-bitcode

tejohnson added inline comments. Comment at: llvm/lib/LTO/LTOBackend.cpp:621 +"command line arguments are not available"); +llvm::EmbedBitcodeInModule(Mod, llvm::MemoryBufferRef(), + /*EmbedBitcode*/ true,

[PATCH] D87636: [ThinLTO] add post-thinlto-merge option to -lto-embed-bitcode

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM, just needs a comment update as noted below and also an update to the patch title. Comment at: llvm/lib/LTO/LTOBackend.cpp:368 + LLVM_DEBUG( +

[PATCH] D87622: [MemProf] Rename HeapProfiler to MemProfiler for consistency (NFC)

tejohnson added a comment. In D87622#2271861 , @MaskRay wrote: > LG. (Nit: technically this is not NFC: it affects command line options and > runtime function names (ABI changes).) Fair enough, will remove that. Repository: rG LLVM Github Monorepo

[PATCH] D87622: [MemProf] Rename HeapProfiler to MemProfiler for consistency

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG226d80ebe20e: [MemProf] Rename HeapProfiler to MemProfiler for consistency (authored by tejohnson). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D87622: [MemProf] Rename HeapProfiler to MemProfiler for consistency (NFC)

tejohnson created this revision. tejohnson added a reviewer: davidxl. Herald added subscribers: jfb, hiraditya, mgorny. Herald added projects: clang, LLVM. tejohnson requested review of this revision. This is consistent with the clang option added in 7ed8124d46f94601d5f1364becee9cee8538265e

[PATCH] D87622: [MemProf] Rename HeapProfiler to MemProfiler for consistency

tejohnson added a comment. In D87622#2271931 , @davidxl wrote: > LGTM (if the option is documented, the documentation part also needs to be > updated). It isn't documented yet, since the generated code won't work until the runtime is in. I will be

[PATCH] D86958: [Docs] Add/update release notes for D71913 (LTO WPD changes)

tejohnson created this revision. tejohnson added a reviewer: hans. Herald added subscribers: dexonsmith, inglorion. Herald added a reviewer: MaskRay. Herald added projects: clang, LLVM. tejohnson requested review of this revision. This adds documentation for the options added / changed by D71913

[PATCH] D86958: [Docs] Add/update release notes for D71913 (LTO WPD changes)

tejohnson updated this revision to Diff 289238. tejohnson added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86958/new/ https://reviews.llvm.org/D86958 Files: clang/docs/ReleaseNotes.rst lld/docs/ReleaseNotes.rst

[PATCH] D88349: Fix inconsistent flag for disabling Dead Virtual Function Elimination

tejohnson added a comment. Can you add a test? You can probably just beef up clang/test/CodeGenCXX/virtual-function-elimination.cpp to test this option as well Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88349/new/

[PATCH] D89087: [MemProf] Pass down memory profile name with optional path from clang

tejohnson created this revision. tejohnson added a reviewer: davidxl. Herald added subscribers: wenlei, dang, hiraditya. Herald added projects: clang, LLVM. tejohnson requested review of this revision. Similar to -fprofile-generate=, add -fmemory-profile= which takes a directory path. This is

[PATCH] D89087: [MemProf] Pass down memory profile name with optional path from clang

tejohnson updated this revision to Diff 298849. tejohnson added a comment. Herald added a project: Sanitizers. Herald added a subscriber: Sanitizers. As discussed in review for D89086 , move test changes to this patch since they are only required here.

[PATCH] D88349: Fix inconsistent flag for disabling Dead Virtual Function Elimination

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM with the test change suggested below. Comment at: clang/test/CodeGenCXX/virtual-function-elimination.cpp:2 // RUN: %clang_cc1 -triple x86_64-unknown-linux -flto

[PATCH] D88349: Fix inconsistent flag for disabling Dead Virtual Function Elimination

tejohnson accepted this revision. tejohnson added a comment. LGTM for test - thanks! Yeah, it looks like that cleanup patch inadvertently fixed this issue as a side effect. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88349/new/

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

tejohnson updated this revision to Diff 287832. tejohnson marked 2 inline comments as done. tejohnson added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85948/new/ https://reviews.llvm.org/D85948 Files:

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

tejohnson marked 18 inline comments as done. tejohnson added a comment. I think I've addressed all the comments. Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:48 +// Size of memory mapped to a single shadow location. +static const uint64_t

[PATCH] D83845: [LTO/WPD] Remove special type test handling for -flto-visibility-public-std

tejohnson added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83845/new/ https://reviews.llvm.org/D83845 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D77925: Revert "[TLI] Per-function fveclib for math library used for vectorization"

tejohnson added a comment. In D77925#2229484 , @mehdi_amini wrote: > Overall that would likely work for XLA. Something I'd like to mention though > in response to: > >> The veclib type is also tied to the accepted values for -fveclib, which is a >> list

[PATCH] D85810: [clang] Pass-through remarks options to linker

tejohnson added a comment. Looks pretty good but I think it can be simplified in one place as I note below. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:596 + // Handle remark diagnostics on screen options: '-Rpass-*'. + if (usesRpassOptions(Args)) +

[PATCH] D85808: [Remarks][2/2] Expand remarks hotness threshold option support in more tools

tejohnson added inline comments. Comment at: llvm/lib/Analysis/OptimizationRemarkEmitter.cpp:103 BFI = ().getBFI(); - else +// Get hotness threshold from PSI. This should only happen once. +if (Context.isDiagnosticsHotnessThresholdSetFromPSI()) {

[PATCH] D75655: [Docs] Document -lto-whole-program-visibility

tejohnson added a comment. @pcc ping. I'd like to get something in for LLVM 11 if at all possible. How about for now I make the last sentence something like "This is useful in situations where it is not safe to specify``-fvisibility=hidden`` at compile time, for example when bitcode objects

[PATCH] D75655: [Docs] Document -lto-whole-program-visibility

This revision was automatically updated to reflect the committed changes. Closed by commit rG72bdb41a06a2: [Docs] Document --lto-whole-program-visibility (authored by tejohnson). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75655/new/

[PATCH] D75655: [Docs] Document -lto-whole-program-visibility

tejohnson updated this revision to Diff 287808. tejohnson marked an inline comment as done. tejohnson added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75655/new/ https://reviews.llvm.org/D75655 Files:

[PATCH] D75655: [Docs] Document -lto-whole-program-visibility

tejohnson marked an inline comment as done. tejohnson added a comment. In D75655#2237474 , @pcc wrote: > I think that the part of the description beginning "This can be used when..." > is somewhat misleading since you could pretty much say the same thing

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

tejohnson updated this revision to Diff 288191. tejohnson added a comment. Remove unnecessary check Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85948/new/ https://reviews.llvm.org/D85948 Files: clang/include/clang/Basic/CodeGenOptions.def

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

tejohnson added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/HeapProfiler.cpp:563 +return false; + if (!ClDebugFunc.empty() && ClDebugFunc == F.getName()) +return false; MaskRay wrote: > `ClDebugFunc != F.getName()` Your suggestion

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

tejohnson added a comment. In D85948#2242370 , @tejohnson wrote: > In D85948#2242352 , @lebedev.ri > wrote: > >> This really needs some docs changes. > > Good point, I'll send a patch for that later today.

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

tejohnson added a comment. In D85948#2242352 , @lebedev.ri wrote: > This really needs some docs changes. Good point, I'll send a patch for that later today. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7ed8124d46f9: [HeapProf] Clang and LLVM support for heap profiling instrumentation (authored by tejohnson). Repository: rG LLVM Github Monorepo

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

tejohnson added a comment. Thanks for the comments @vitalybuka and @davidxl, will address those shortly. In D85948#2217476 , @davidxl wrote: > one nit: since the same instrumentation can be used to profiling global > variable accesses (especially those

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

tejohnson updated this revision to Diff 285777. tejohnson marked an inline comment as done. tejohnson added a comment. address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85948/new/ https://reviews.llvm.org/D85948 Files:

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

tejohnson marked 12 inline comments as done. tejohnson added a comment. Addressed everyone's comments as well as the clang tidy warnings. I had hoped to get the runtime part ready and uploaded today, but it looks like I won't make it. I'll be out all next week, so will get that sent when I'm

[PATCH] D87949: [ThinLTO] Option to bypass function importing.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. lgtm with comment typo fix Comment at: llvm/include/llvm/LTO/LTOBackend.h:59 + +/// Distributed ThinlTO: load the referenced modules, keeping their buffers +/// alive

[PATCH] D87943: [clang] Remove profile availabile check for fsplit-machine-functions.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. lgtm with a couple of minor suggestions Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4919 if (Triple.isX86() && Triple.isOSBinFormatELF()) { if

[PATCH] D87949: [ThinLTO] Option to bypass function importing.

tejohnson added a comment. Minor comments. Comment at: clang/test/CodeGen/thinlto_embed_bitcode.ll:17 +; RUN: rm %t1.bc %t2.bc +; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t-redo.o -x ir %t-embedded.bc -c -fthinlto-index=%t.o.thinlto.bc -mllvm

[PATCH] D81242: [StackSafety] Run ThinLTO

tejohnson added a comment. I just noticed that generateParamAccessSummary is taking a bit over 5% of the ThinLTO thin link step in an internal build (and will soon be more than 5% as I found another analysis that is hogging compile time that I'm going to work on fixing). This is currently the

[PATCH] D81242: [StackSafety] Run ThinLTO

tejohnson added a comment. In D81242#2190041 , @vitalybuka wrote: > In D81242#2188467 , @tejohnson wrote: > >> In D81242#2188441 , @vitalybuka >> wrote: >> >>> In

[PATCH] D81242: [StackSafety] Run ThinLTO

tejohnson added a comment. In D81242#2183383 , @tejohnson wrote: > I just noticed that generateParamAccessSummary is taking a bit over 5% of the > ThinLTO thin link step in an internal build (and will soon be more than 5% as > I found another analysis

[PATCH] D85808: [remarks] Optimization remarks hotness filtering from profile summary

tejohnson added a comment. In D85808#2212297 , @weiwang wrote: > This is the 3rd of 3 dependent patches: > > 1. [lld] Enable remarks hotness filtering in lld: > https://reviews.llvm.org/D85809 > 2. [clang] Pass-through remarks options to lld: >

[PATCH] D85948: [HeapProf] Clang and LLVM support for heap profiling instrumentation

tejohnson created this revision. tejohnson added reviewers: vitalybuka, kcc, eugenis, davidxl. Herald added subscribers: dang, jfb, hiraditya, mgorny. Herald added projects: clang, LLVM. tejohnson requested review of this revision. See RFC for background:

[PATCH] D85808: [remarks] Optimization remarks hotness filtering from profile summary

tejohnson added inline comments. Comment at: lld/ELF/Config.h:280 + // If threshold option is not specified, it is disabled by default. + llvm::Optional optRemarksHotnessThreshold = 0; weiwang wrote: > tejohnson wrote: > > Since this field is being added in

[PATCH] D81242: [StackSafety] Run ThinLTO

tejohnson added a comment. In D81242#2188441 , @vitalybuka wrote: > In D81242#2183383 , @tejohnson wrote: > >> Is the stack safety analysis meant to be always on with ThinLTO? > > During compilation most of the

[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

This revision was automatically updated to reflect the committed changes. Closed by commit rG6014c46c80ca: Restore [WPD/LowerTypeTests] Delay lowering/removal of type tests until after… (authored by tejohnson). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D71913: [LTO/WPD] Enable aggressive WPD under LTO option

tejohnson marked an inline comment as done. tejohnson added inline comments. Herald added a reviewer: MaskRay. Comment at: clang/test/CodeGenCXX/lto-visibility-inference.cpp:73 c1->f(); - // ITANIUM-NOT: type.test{{.*}}!"_ZTS2C2" + // ITANIUM: type.test{{.*}}!"_ZTS2C2"

[PATCH] D83845: [LTO/WPD] Remove special type test handling for -flto-visibility-public-std

tejohnson created this revision. tejohnson added a reviewer: pcc. Herald added subscribers: cfe-commits, dexonsmith, steven_wu, hiraditya, inglorion. Herald added a project: clang. As discussed in follow up comments on D71913 , the special case handling for

[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

tejohnson marked an inline comment as done. tejohnson added a comment. In D73242#1963451 , @evgeny777 wrote: > This needs to be rebased Sorry for the delay, I've been side tracked on other work. Rebased and beefed up the test as requested.

[PATCH] D73242: [WPD/LowerTypeTests] Delay lowering/removal of type tests until after ICP

tejohnson updated this revision to Diff 273730. tejohnson added a comment. Rebase and beef up test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73242/new/ https://reviews.llvm.org/D73242 Files: llvm/include/llvm/IR/ModuleSummaryIndex.h

[PATCH] D81223: Size LTO (1/3): Standardizing the use of OptimizationLevel

tejohnson added a comment. Sorry I haven't had a chance to review this yet. Added @mtrofin who recently added the OptimizationLevel class that this is leveraging, and @yamauchi who has also been looking at size optimizations in llvm. At a first glance it isn't clear to me how much of this is

[PATCH] D85810: [clang] Pass-through remarks options to lld

tejohnson added inline comments. Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:71 + + bool isLLD = llvm::sys::path::filename(LinkerPath) == "ld.lld" || + llvm::sys::path::stem(LinkerPath) != "ld.lld"; MaskRay wrote: > Checking the path is

[PATCH] D92673: [ThinLTO] Remove unused symbol declarations when possible

tejohnson created this revision. tejohnson added a reviewer: wmi. Herald added subscribers: wenlei, arphaman, steven_wu, hiraditya, inglorion, Prazek. tejohnson requested review of this revision. Herald added projects: clang, LLVM. Herald added a subscriber: cfe-commits. Follow on to D42816

[PATCH] D92673: [ThinLTO] Remove unused symbol declarations when possible

tejohnson abandoned this revision. tejohnson added a comment. Subsumed by D92804 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92673/new/ https://reviews.llvm.org/D92673

[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility

This revision was automatically updated to reflect the committed changes. Closed by commit rG0768b0576a93: Avoid redundant work when computing vtable vcall visibility (authored by tejohnson). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91676/new/

[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility

tejohnson added a comment. Adding another reviewer - @wmi can you take a look? This is a straightforward compile time fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91676/new/ https://reviews.llvm.org/D91676

[PATCH] D91812: [ThinLTO/WPD] Enable -wholeprogramdevirt-skip in ThinLTO backends

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG6e4c1cf29388: [ThinLTO/WPD] Enable -wholeprogramdevirt-skip in ThinLTO backends (authored by tejohnson). Repository: rG LLVM Github Monorepo

[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility

tejohnson added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:1301-1302 + // has no effect on the min visibility computed below by the recursive caller. + if (!Visited.insert(RD).second) +return llvm::GlobalObject::VCallVisibilityTranslationUnit; +

[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility

tejohnson updated this revision to Diff 307404. tejohnson added a comment. Improve comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91676/new/ https://reviews.llvm.org/D91676 Files: clang/lib/CodeGen/CGVTables.cpp

[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility

tejohnson added inline comments. Comment at: clang/lib/CodeGen/CGVTables.cpp:1301-1302 + // has no effect on the min visibility computed below by the recursive caller. + if (!Visited.insert(RD).second) +return llvm::GlobalObject::VCallVisibilityTranslationUnit; +

[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility

tejohnson added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91676/new/ https://reviews.llvm.org/D91676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D85808: [Remarks][2/2] Expand remarks hotness threshold option support in more tools

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. lgtm with a couple of minor nits noted below that you can fix before submitting Comment at: clang/include/clang/Basic/CodeGenOptions.h:353 + /// The threshold is an

[PATCH] D73418: [WPD] Emit vcall_visibility metadata for MicrosoftCXXABI

tejohnson added a comment. In D73418#2461943 , @ayermolo wrote: > @tejohnson I was wondering where does this @anon.{{.*}} gets set, or in > general where can I find more information about it. > > When I step through the code in emitVTableTypeMetadata,

[PATCH] D91585: [NPM] Move more O0 pass building into PassBuilder

tejohnson added inline comments. Comment at: llvm/lib/Passes/PassBuilder.cpp:2365 +Matches[1] != "lto") { + MPM.addPass(buildO0DefaultPipeline(L, Matches[1] != "default")); return Error::success(); I think it would be clearer to check against

[PATCH] D91585: [NPM] Move more O0 pass building into PassBuilder

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

[PATCH] D91585: [NPM] Move more O0 pass building into PassBuilder

tejohnson added inline comments. Comment at: llvm/lib/Passes/PassBuilder.cpp:2365 +Matches[1] != "lto") { + MPM.addPass(buildO0DefaultPipeline(L, Matches[1] != "default")); return Error::success(); tejohnson wrote: > I think it would be

[PATCH] D91812: [ThinLTO/WPD] Enable -wholeprogramdevirt-skip in ThinLTO backends

tejohnson created this revision. tejohnson added reviewers: pcc, evgeny777. Herald added subscribers: cfe-commits, steven_wu, hiraditya, inglorion, Prazek. Herald added projects: clang, LLVM. tejohnson requested review of this revision. Previously this option could be used to skip

[PATCH] D91410: [llvm][clang][mlir] Add checks for the return values from Target::createXXX to prevent protential null deref

tejohnson accepted this revision. tejohnson added a comment. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91410/new/ https://reviews.llvm.org/D91410 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D91567: [llvm][inliner] Reuse the inliner pass to implement 'always inliner'

tejohnson added a comment. From a ThinLTO perspective, no specific concerns as the buildModuleSimplificationPipeline is invoked in both the pre and post LTO link pipelines, so they both get an equivalent change. But there is an issue for regular LTO, noted below. Comment

[PATCH] D91410: [llvm][clang][mlir] Add checks for the return values from Target::createXXX to prevent protential null deref

tejohnson added a comment. In D91410#2401751 , @OikawaKirie wrote: > In D91410#2400018 , @tejohnson wrote: > >> > > > >> ... the new checking is a mix of assert and fatal errors. Is that intended? > > No. The

[PATCH] D85808: [Remarks][2/2] Expand remarks hotness threshold option support in more tools

tejohnson added a comment. Thanks for adding the Driver test. I was thinking of something to test the CompilerInvocation changes, similar to your test using opt, that ensures the option has the desired behavior when invoked via clang. Looks like there is an existing test

[PATCH] D91410: [llvm][clang][mlir] Add checks for the return values from Target::createXXX to prevent protential null deref

tejohnson added a comment. I was reviewing the llvm/lib/* changes and note that the new checking is a mix of assert (debug or assert enabled compiler only) and fatal errors (both debug and release compilers). Is that intended? If these are not likely due to user input issues, then perhaps they

[PATCH] D85808: [Remarks][2/2] Expand remarks hotness threshold option support in more tools

tejohnson added a comment. Largely LGTM, except that the clang changes are missing a corresponding test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85808/new/ https://reviews.llvm.org/D85808 ___

[PATCH] D91585: [NPM] Move more O0 pass building into PassBuilder

tejohnson added a comment. Thanks, generally this seems to be good cleanup. Question on one of the changes below though. Comment at: llvm/lib/Passes/PassBuilder.cpp:2365 + // Don't do anything for (thin)lto backend compiles at O0. + if (Matches[1] != "thinlto" &&

[PATCH] D91676: Avoid redundant work when computing vtable vcall visibility

tejohnson created this revision. tejohnson added reviewers: pcc, ostannard. Herald added a project: clang. tejohnson requested review of this revision. Add a Visited set to avoid repeatedly processing the same base classes in complex class hierarchies. This cut down the compile time of one source

[PATCH] D90366: [ThinLTO] Strenghten the test for .llvmcmd embedding

tejohnson added a comment. The motivation and effects of the change are unclear to me. Does this mean that the .llvmcmd section is always emitted? Or always emitted whenever any bitcode embedding is requested? Comment at: clang/test/CodeGen/thinlto_embed_bitcode.ll:28 ;

[PATCH] D89087: [MemProf] Pass down memory profile name with optional path from clang

tejohnson added a comment. In D89087#2359962 , @davidxl wrote: > There should be a related LLVM side of changes. Is it in a different patch? That's here too. See the change in llvm/lib/Transforms/Instrumentation/MemProfiler.cpp and a related test (they

[PATCH] D89087: [MemProf] Pass down memory profile name with optional path from clang

tejohnson updated this revision to Diff 302119. tejohnson added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89087/new/ https://reviews.llvm.org/D89087 Files: clang/include/clang/Basic/CodeGenOptions.def

[PATCH] D89087: [MemProf] Pass down memory profile name with optional path from clang

tejohnson updated this revision to Diff 302112. tejohnson added a comment. Rebase and fix bad merge Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89087/new/ https://reviews.llvm.org/D89087 Files: clang/include/clang/Basic/CodeGenOptions.def

[PATCH] D89087: [MemProf] Pass down memory profile name with optional path from clang

tejohnson marked 3 inline comments as done. tejohnson added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1049 + } else if (Args.hasArg(OPT_fmemory_profile)) +Opts.MemoryProfileOutput = MemProfileBasename; MaskRay wrote: >

<    1   2   3   4   5   6   7   >