[PATCH] D45217: [ThinLTO] Pass -save-temps to LTO backend for distributed ThinLTO builds

tejohnson updated this revision to Diff 142668. tejohnson marked 2 inline comments as done. tejohnson added a comment. Addressed comments Repository: rC Clang https://reviews.llvm.org/D45217 Files: include/clang/Driver/Options.td include/clang/Frontend/CodeGenOptions.h lib/CodeGen/Back

[PATCH] D45217: [ThinLTO] Pass -save-temps to LTO backend for distributed ThinLTO builds

tejohnson marked 2 inline comments as done. tejohnson added inline comments. Comment at: include/clang/Driver/Options.td:2255 HelpText<"Save intermediate compilation results.">; -def save_temps : Flag<["-", "--"], "save-temps">, Flags<[DriverOption]>, +def save_temps : Flag<["

[PATCH] D45217: [ThinLTO] Pass -save-temps to LTO backend for distributed ThinLTO builds

tejohnson updated this revision to Diff 142669. tejohnson marked 2 inline comments as done. tejohnson added a comment. Address comments Repository: rC Clang https://reviews.llvm.org/D45217 Files: include/clang/Driver/Options.td include/clang/Frontend/CodeGenOptions.h lib/CodeGen/Backen

[PATCH] D45217: [ThinLTO] Pass -save-temps to LTO backend for distributed ThinLTO builds

This revision was automatically updated to reflect the committed changes. Closed by commit rL330194: [ThinLTO] Pass -save-temps to LTO backend for distributed ThinLTO builds (authored by tejohnson, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llv

[PATCH] D45217: [ThinLTO] Pass -save-temps to LTO backend for distributed ThinLTO builds

tejohnson added inline comments. Comment at: cfe/trunk/lib/Driver/ToolChains/Clang.cpp:3270 + if (const Arg *A = Args.getLastArg(options::OPT_save_temps_EQ)) +Args.AddLastArg(CmdArgs, options::OPT_save_temps_EQ); bjope wrote: > A is not used, so this does

[PATCH] D45217: [ThinLTO] Pass -save-temps to LTO backend for distributed ThinLTO builds

tejohnson added inline comments. Comment at: cfe/trunk/lib/Driver/ToolChains/Clang.cpp:3270 + if (const Arg *A = Args.getLastArg(options::OPT_save_temps_EQ)) +Args.AddLastArg(CmdArgs, options::OPT_save_temps_EQ); tejohnson wrote: > bjope wrote: > > A is no

[PATCH] D45771: [Driver] Support for -save-stats in AddGoldPlugin.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM with comment nit Comment at: lib/Driver/ToolChains/CommonArgs.h:107 llvm::opt::OptSpecifier Group); +SmallString<128> getStatsFileN

[PATCH] D46464: [ThinLTO] Support opt remarks options with distributed ThinLTO backends

tejohnson created this revision. tejohnson added a reviewer: pcc. Herald added subscribers: eraman, inglorion, mehdi_amini. Passes down the necessary code ge options to the LTO Config to enable -fdiagnostics-show-hotness and -fsave-optimization-record in the ThinLTO backend for a distributed build

[PATCH] D46464: [ThinLTO] Support opt remarks options with distributed ThinLTO backends

tejohnson updated this revision to Diff 145321. tejohnson added a comment. Address comments Repository: rC Clang https://reviews.llvm.org/D46464 Files: lib/CodeGen/BackendUtil.cpp lib/Frontend/CompilerInvocation.cpp test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll Inde

[PATCH] D46464: [ThinLTO] Support opt remarks options with distributed ThinLTO backends

tejohnson marked an inline comment as done. tejohnson added a comment. Fixed the output file as suggested. Also note that this test will fail and can't go in until after https://reviews.llvm.org/D46387 Repository: rC Clang https://reviews.llvm.org/D46464 __

[PATCH] D46464: [ThinLTO] Support opt remarks options with distributed ThinLTO backends

tejohnson updated this revision to Diff 145348. tejohnson added a comment. Update test for change to pass -1 as the Task ID for distributed backends, and to reflect companion llvm change (https://reviews.llvm.org/D46488). Repository: rC Clang https://reviews.llvm.org/D46464 Files: lib/Code

[PATCH] D46464: [ThinLTO] Support opt remarks options with distributed ThinLTO backends

tejohnson added inline comments. Comment at: test/CodeGen/thinlto-diagnostic-handler-remarks-with-hotness.ll:14 +; RUN: %clang -O2 -x ir %t.o -fthinlto-index=%t.thinlto.bc -fsave-optimization-record -fdiagnostics-show-hotness -o %t2.o -c +; RUN: cat %t2.opt.yaml.thin.0.yaml | Fi

[PATCH] D46464: [ThinLTO] Support opt remarks options with distributed ThinLTO backends

This revision was automatically updated to reflect the committed changes. Closed by commit rL331592: [ThinLTO] Support opt remarks options with distributed ThinLTO backends (authored by tejohnson, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm

[PATCH] D47906: [ThinLTO] Add testing of summary index parsing to a couple CFI tests

tejohnson added a comment. ping (assuming this is ok, but since I sent for review...) Repository: rC Clang https://reviews.llvm.org/D47906 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-

[PATCH] D47906: [ThinLTO] Add testing of summary index parsing to a couple CFI tests

This revision was automatically updated to reflect the committed changes. Closed by commit rC335618: [ThinLTO] Add testing of summary index parsing to a couple CFI tests (authored by tejohnson, committed by ). Changed prior to commit: https://reviews.llvm.org/D47906?vs=150402&id=152899#toc Rep

[PATCH] D69327: [Clang][ThinLTO] Add a cache for compile phase output.

tejohnson added a comment. I haven't read through the patch in detail yet, but from the description it sounds like a cache is being added for the compile step outputs, e.g. the bitcode output object of the "clang -flto=thin -c" step? Typically the build system takes care of incremental build ha

[PATCH] D69327: [Clang][ThinLTO] Add a cache for compile phase output.

tejohnson added a comment. I was just typing up a response similar to @steven_wu 's response... I don't think clang should be in the business of caching the outputs of a prior clang invocation, the build system should and usually does avoid re-executing if the inputs have not changed. Note that

[PATCH] D69327: [Clang][ThinLTO] Add a cache for compile phase output.

tejohnson added a comment. In D69327#1719295 , @tejohnson wrote: > I was just typing up a response similar to @steven_wu 's response... I don't > think clang should be in the business of caching the outputs of a prior clang > invocation, the build system

[PATCH] D69327: [Clang][ThinLTO] Add a cache for compile phase output.

tejohnson added a comment. We rely on the minimized bitcode from that option in our builds, so it won't be going away. We could add it as a driver option, but it doesn't sound like that will solve the ccache issue. I'm not very familiar with that cache support, or if there is a way to express o

[PATCH] D69406: [clang][ThinLTO] Promote cc1 -fthin_link_bitcode to driver -fthinlto_link_bitcode

tejohnson added a comment. Thanks. Prefer the old name since it is already in use and since this is for the "thin link" phase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69406/new/ https://reviews.llvm.org/D69406

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

[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#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 seri

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

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

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

tejohnson added a comment. Needs testing of the inline handling, and of LTO linking IR with different attributes (which is going to hit your assert, see below). Comment at: clang/lib/CodeGen/CGCall.cpp:1987 + // Attach "vect-lib" attribute to function based on '-fveclib' se

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

tejohnson added a comment. Great, thanks! A few minor comments below. Looks like it needs a clang-format too. Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:272 +else + VectLibrary = Impl.ClVectorLibrary; + Suggest moving the implementatio

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

tejohnson added inline comments. Comment at: llvm/include/llvm/Analysis/TargetLibraryInfo.h:272 +else + VectLibrary = Impl.ClVectorLibrary; + wenlei wrote: > tejohnson wrote: > > Suggest moving the implementation of this constructor to the .cpp file, in

[PATCH] D77632: [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. I think one check is missing in the test, see comment below. Comment at: llvm/test/Transforms/Inline/veclib-compat.ll:28 + %rslt = call i32 @callee_massv(i8 123)

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

tejohnson added a comment. In D77632#1974363 , @wenlei wrote: > In D77632#1974015 , @nikic wrote: > > > This change causes a ~0.5% compile-time regressions: > > http://llvm-compile-time-tracker.com/compare.php?from

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

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

[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] 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 effect.

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

tejohnson added a comment. In D77632#1976308 , @mehdi_amini wrote: > In D77632#1976231 , @wenlei wrote: > > > And agree with @tejohnson, if the openness is a feature, it should be > > covered in tests, otherwise it

[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] D77989: Allow disabling of vectorization using internal options

tejohnson marked an inline comment as done. tejohnson added inline comments. Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7618-7621 +: InterleaveOnlyWhenForced(Opts.InterleaveOnlyWhenForced | + !EnableLoopInterleaving), + Vect

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

tejohnson updated this revision to Diff 257562. tejohnson added a comment. Address comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77989/new/ https://reviews.llvm.org/D77989 Files: clang/test/CodeGen/thinlto-loop-vectorize-pm.c clang/tes

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

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

[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 https://reviews.ll

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

[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] 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. Repository:

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

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

[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, the

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

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

[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 D81242#218

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

[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: http://lists.llvm.org/pipermail/llvm-dev/2

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

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

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

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

[PATCH] D40748: [ThinLTO] Enable importing of aliases as copy of aliasee

tejohnson created this revision. Herald added a subscriber: inglorion. Clang side changes to go with LLVM change to import aliases as a copy of their aliasee. Simply refactor out some handling that is moved to LLVM for use elsewhere. Depends on https://reviews.llvm.org/D40747. Repository: rC

[PATCH] D131306: [llvm][misexpect] Track provenance of branch weights

tejohnson added subscribers: xur, davidxl. tejohnson added a comment. @davidxl @xur for review and thoughts. I'm a little wary of requiring that both pieces of metadata be carried together, as that seems very brittle to maintain in the compiler. What would happen if the MD_expected didn't get p

[PATCH] D131306: [llvm][misexpect] Track provenance of branch weights

tejohnson added a comment. In D131306#3756074 , @paulkirth wrote: > In D131306#3756009 , @tejohnson > wrote: > >> @davidxl @xur for review and thoughts. >> >> I'm a little wary of requiring that both pieces of me

[PATCH] D131306: [llvm][misexpect] Track provenance of branch weights

tejohnson added a comment. In D131306#3756153 , @paulkirth wrote: > In D131306#3756087 , @tejohnson > wrote: > >> Well I was thinking the extra field would be optional as well and could be >> removed. But unders

[PATCH] D128142: [MemProf] Memprof profile matching and annotation

tejohnson marked 12 inline comments as done. tejohnson added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:1252 +AllocInfo->Info.getMinLifetime()); + SmallVector StackIds; + std::set StackHashSet; ---

[PATCH] D128142: [MemProf] Memprof profile matching and annotation

tejohnson updated this revision to Diff 456710. tejohnson marked 5 inline comments as done. tejohnson added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128142/new/ https://reviews.llvm.org/D128142 Files: clang/lib/F

[PATCH] D128142: [MemProf] Memprof profile matching and annotation

tejohnson marked 3 inline comments as done. tejohnson added inline comments. Comment at: llvm/include/llvm/Analysis/MemoryProfileInfo.h:1 +//===- llvm/Analysis/MemoryProfileInfo.h - memory profile info ---*- C++ -*-==// +// snehasish wrote: > Can you split out t

[PATCH] D128142: [MemProf] Memprof profile matching and annotation

tejohnson updated this revision to Diff 441529. tejohnson marked 3 inline comments as done. tejohnson added a comment. Rebase on top of D128854 which now includes the extracted Analysis utilities. I have not yet addressed the other comments on this patch. Repo

[PATCH] D122587: [clang][NFC] Extract the EmitAssemblyHelper::TargetTriple member

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

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1554 + Twine(*Opts.DiagnosticsMisExpectTolerance), SA); + for (StringRef Sanitizer : serializeSanitizerKinds(Opts.SanitizeRecover)) paulkirth wrote: > I really

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson added a comment. In D115907#3412685 , @paulkirth wrote: > @tejohnson thanks for pointing me to the document. I knew it had something to > do w/ CC1 but missed that this was well documented. I was equally unaware of the documentation of this un

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson added inline comments. Comment at: llvm/lib/Transforms/Utils/MisExpect.cpp:150 +// TODO: when clang allows c++17, use std::clamp instead +uint32_t clamp(uint64_t value, uint32_t hi, uint32_t low) { + if (value > hi) Nit: seems more intuitive to pass lo

[PATCH] D123026: [clang][NFC] Extract the EmitAssemblyHelper::emitLTOSummary method

tejohnson added a comment. I'd prefer to keep the setting of the module flags inline here. The main reason is that the EnableSplitLTOUnit module flag is also set inline for the ThinLTO case just above, and it is easier to see what is going on if they are near each other. Suggest making the new

[PATCH] D123026: [clang][NFC] Extract the EmitAssemblyHelper::shouldEmitLTOSummary method

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. lgtm with a couple of small changes noted below. Comment at: clang/lib/CodeGen/BackendUtil.cpp:167 + /// Check whether we should emit a module summary. + /// The mo

[PATCH] D128955: [WPD] Use new llvm.public.type.test intrinsic for potentially publicly visible classes

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. lgtm In D128955#3676478 , @aeubanks wrote: > In D128955#3676198 , @tejohnson > wrote: > >> In D128955

[PATCH] D130531: [IR] Use Min behavior for module flag "PIC Level"

tejohnson added a comment. @tmsriram can you take a look? I can review from a code standpoint but you are more familiar with PIC/PIE details. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130531/new/ https://reviews.llvm.org/D130531 _

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson added inline comments. Herald added a project: All. Comment at: clang/include/clang/Basic/CodeGenOptions.def:172 CODEGENOPT(NoWarn, 1, 0) ///< Set when -Wa,--no-warn is enabled. +CODEGENOPT(MisExpect , 1, 0) ///< Set -Wmisexpect is enabled CODEGENO

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson added inline comments. Comment at: clang/include/clang/Basic/CodeGenOptions.h:425 + /// values in order to be included in misexpect diagnostics. + Optional DiagnosticsMisExpectTollerance = 0; + s/Tollerance/Tolerance/ (and elsewhere throughout patch)

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. lgtm Just some minor cleanup before submitting as noted below. Comment at: clang/test/Profile/misexpect-branch.c:3 + +// test likely branch +// RUN: llvm-profdata merg

[PATCH] D123026: [clang][NFC] Extract EmitAssemblyHelper::shouldEmitRegularLTOSummary

tejohnson added a comment. In D123026#3436323 , @psamolysov-intel wrote: > Colleagues, could you help me with landing? @tejohnson has approved the patch > (if I applied the suggestion as it was expected, thank you @tejohnson!) lgtm. I can commit for yo

[PATCH] D123026: [clang][NFC] Extract EmitAssemblyHelper::shouldEmitRegularLTOSummary

This revision was automatically updated to reflect the committed changes. Closed by commit rGb4ac84901e9b: [clang][NFC] Extract EmitAssemblyHelper::shouldEmitRegularLTOSummary (authored by psamolysov-intel, committed by tejohnson). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D115907: [misexpect] Re-implement MisExpect Diagnostics

tejohnson added a comment. lgtm if you are waiting for a re-review. As noted in my previous comment, you have added a little bit of unnecessary checking at the lower end with your new clamp function, since it is an unsigned value and can never go below 0. But it isn't a big deal if you prefer t

[PATCH] D72404: [ThinLTO/FullLTO] Support Os and Oz

tejohnson added a comment. In D72404#3310704 , @aykevl wrote: > @mehdi_amini thanks for explaining! D119342 > moves slightly closer to removing > SizeLevel from the pass pipeline setup. I left a comment on D119342

[PATCH] D128955: [WPD] Use new llvm.public.type.test intrinsic for potentially publicly visible classes

tejohnson added a comment. Thanks for implementing this! In D71913 , I also modified this code, which might need something similar: https://github.com/llvm/llvm-project/blob/59afc4038b1096bc6fea7b322091f6e5e2dc0b38/clang/lib/CodeGen/ItaniumCXXABI.cpp#L707-L713

[PATCH] D128955: [WPD] Use new llvm.public.type.test intrinsic for potentially publicly visible classes

tejohnson added inline comments. Comment at: clang/test/CodeGenCXX/thinlto-distributed-type-metadata.cpp:22 // OPT-NOT: @llvm.type.test -// OPT-NOT: call void @llvm.assume // We should have only one @llvm.assume call, the one that was expanded aeubanks wrote: >

[PATCH] D128955: [WPD] Use new llvm.public.type.test intrinsic for potentially publicly visible classes

tejohnson added a comment. In D128955#3666830 , @aeubanks wrote: > the assert that there are no public.type.tests in LTT fails on > `CodeGenCXX/thinlto-distributed-type-metadata.cpp`. for some reason clang > doesn't go through the LTO API at [1], it jus

[PATCH] D128955: [WPD] Use new llvm.public.type.test intrinsic for potentially publicly visible classes

tejohnson added inline comments. Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:456 + updatePublicTypeTestCalls(TheModule, +/* WholeProgramVisibilityEnabledInLTO */ false); Add a comment to see note at call to updateVCallVisibili

[PATCH] D128142: [MemProf] Memprof profile matching and annotation

tejohnson updated this revision to Diff 446986. tejohnson marked 3 inline comments as done. tejohnson added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128142/new/ https://reviews.llvm.org/D128142 Files: clang/lib/F

[PATCH] D128142: [MemProf] Memprof profile matching and annotation

tejohnson added inline comments. Comment at: clang/test/CodeGen/memprof.cpp:15 +// # Collect memory profile: +// $ clang++ -fuse-ld=lld -Wl,-no-pie -Wl,--no-rosegment -gmlt \ +// -fdebug-info-for-profiling -mno-omit-leaf-frame-pointer \ snehasish wrote: > Ju

[PATCH] D128955: [WPD] Use new llvm.public.type.test intrinsic for potentially publicly visible classes

tejohnson added a subscriber: fhahn. tejohnson added a comment. In D128955#3674787 , @aeubanks wrote: > random question, if the old API is "legacy", are there any plans to remove it? @fhahn started to work on this at some point (see https://bugs.llvm.or

[PATCH] D66488: [LTO] Always mark regular LTO units with EnableSplitLTOUnit=1 under the new pass manager

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. Lgtm. Thanks for fixing this! Not sure how I missed doing this for both PMs. As an aside, please upload future patches with context, it makes reviewing much easier. Repository: rG LL

[PATCH] D71907: [WPD/VFE] Always emit vcall_visibility metadata for -fwhole-program-vtables

tejohnson added a comment. In D71907#1797218 , @evgeny777 wrote: > Looks good at first sight. Do you have linker patch for me to try this out? The linker changes are in D71913 . This one should be a noop by itself (other

[PATCH] D71911: [ThinLTO] Summarize vcall_visibility metadata

tejohnson updated this revision to Diff 235509. tejohnson added a comment. Implement suggested name change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71911/new/ https://reviews.llvm.org/D71911 Files: clang/lib/CodeGen/CGVTables.cpp clang/l

[PATCH] D71911: [ThinLTO] Summarize vcall_visibility metadata

tejohnson updated this revision to Diff 235510. tejohnson added a comment. Attempt to fix patch to not include parent revision changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71911/new/ https://reviews.llvm.org/D71911 Files: clang/test/Cod

[PATCH] D70765: LTOVisibility.rst: fix up syntax in example

tejohnson added a comment. In D70765#1802564 , @dankamongmen wrote: > I hate to bother anyone, but can this go ahead and get merged? :) thanks Typically the author commits the patch, unless they don't have commit access and request the reviewer to comm

[PATCH] D61634: [clang/llvm] Allow efficient implementation of libc's memory functions in C/C++

tejohnson added a comment. In D61634#1515176 , @tejohnson wrote: > In D61634#1512020 , @gchatelet wrote: > > > AFAIU here is a coarse plan of what needs to happen > I've listed below what I believe is the status:

[PATCH] D70765: LTOVisibility.rst: fix up syntax in example

tejohnson added a comment. In D70765#1807609 , @dankamongmen wrote: > > Typically the author commits the patch, unless they don't have commit > > access and request the reviewer to commit for them, but I don't see a > > request for that here. > > Thanks

[PATCH] D70765: LTOVisibility.rst: fix up syntax in example

This revision was automatically updated to reflect the committed changes. Closed by commit rG43f938eddc8a: LTOVisibility.rst: fix up syntax in example (authored by tejohnson). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70765/new/ https://reviews.

<    1   2   3   4   5   6   7   >