[PATCH] D152741: [WPD] implement -fskip-vtable-filepaths

tejohnson added a comment. In D152741#4419265 , @modimo wrote: > In D152741#4418831 , @tejohnson > wrote: > >> I think I understand the motivation, but not sure I agree this is the right >> approach - can you si

[PATCH] D152741: [WPD] implement -fskip-vtable-filepaths

tejohnson added a comment. In D152741#4419366 , @modimo wrote: > In D152741#4419324 , @tejohnson > wrote: > >> In D152741#4419265 , @modimo wrote: >> >>> In D152741#44188

[PATCH] D146777: [clang] Preliminary fat-lto-object support

tejohnson added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1065 + if (CodeGenOpts.FatLTO) { +// Set EnableSplitLTOUnit, since the config above won't +if (!TheModule->getModuleFlag("EnableSplitLTOUnit")) Can you expand the comment a bi

[PATCH] D146777: [clang] Preliminary fat-lto-object support

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. lgtm Comment at: clang/test/Driver/fatlto.c:1 + Is this empty file meant to contain something? Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D28139: [ThinLTO] No need to rediscover imports in distributed backend

tejohnson created this revision. tejohnson added reviewers: mehdi_amini, pcc. tejohnson added a subscriber: cfe-commits. We can simply import all external values with summaries included in the individual index file created for the distributed backend job, as only those are added to the individual

[PATCH] D28139: [ThinLTO] No need to rediscover imports in distributed backend

This revision was automatically updated to reflect the committed changes. Closed by commit rL290674: [ThinLTO] No need to rediscover imports in distributed backend (authored by tejohnson). Changed prior to commit: https://reviews.llvm.org/D28139?vs=82603&id=82608#toc Repository: rL LLVM htt

[PATCH] D28362: [ThinLTO] Optionally ignore empty index file

tejohnson created this revision. tejohnson added a reviewer: mehdi_amini. tejohnson added a subscriber: cfe-commits. In order to simplify distributed build system integration, where actions may be scheduled before the Thin Link which determines the list of objects selected by the linker. The gold

[PATCH] D28362: [ThinLTO] Optionally ignore empty index file

tejohnson added inline comments. Comment at: lib/CodeGen/BackendUtil.cpp:65 +"Ignore an empty index file and perform non-ThinLTO compilation"), +llvm::cl::init(false)); + mehdi_amini wrote: > Is it common to do this in clang? Doesn't look common, but

[PATCH] D28362: [ThinLTO] Optionally ignore empty index file

tejohnson added inline comments. Comment at: lib/CodeGen/BackendUtil.cpp:65 +"Ignore an empty index file and perform non-ThinLTO compilation"), +llvm::cl::init(false)); + tejohnson wrote: > mehdi_amini wrote: > > Is it common to do this in clang? > Do

[PATCH] D28362: [ThinLTO] Optionally ignore empty index file

tejohnson updated this revision to Diff 83405. tejohnson added a comment. Move option to LLVM. https://reviews.llvm.org/D28362 Files: lib/CodeGen/BackendUtil.cpp test/CodeGen/thinlto_backend.ll Index: test/CodeGen/thinlto_backend.ll

[PATCH] D28362: [ThinLTO] Optionally ignore empty index file

tejohnson added a comment. In https://reviews.llvm.org/D28362#638203, @mehdi_amini wrote: > LGTM Thanks, let me know if the llvm companion patch (https://reviews.llvm.org/D28410) is ok as well. https://reviews.llvm.org/D28362 ___ cfe-commits mai

[PATCH] D28362: [ThinLTO] Optionally ignore empty index file

tejohnson added a comment. In https://reviews.llvm.org/D28362#638260, @mehdi_amini wrote: > > In order to simplify distributed build system integration, where actions > > may be scheduled before the Thin Link which determines the list of > > objects selected by the linker. The gold plugin curre

[PATCH] D28362: [ThinLTO] Optionally ignore empty index file

This revision was automatically updated to reflect the committed changes. Closed by commit rL291303: [ThinLTO] Optionally ignore empty index file (authored by tejohnson). Changed prior to commit: https://reviews.llvm.org/D28362?vs=83405&id=83450#toc Repository: rL LLVM https://reviews.llvm.

[PATCH] D28588: Pass -fprofile-sample-use to lto backends.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D28588 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D31050: [ThinLTO] Clang support for emitting minimized bitcode for thin link

tejohnson created this revision. Herald added a subscriber: Prazek. Clang companion patch to LLVM patch https://reviews.llvm.org/D31027, which adds support for emitting minimized bitcode file for use in the thin link step. Add a cc1 option -fthin-link-bitcode= to trigger this behavior. Depends o

[PATCH] D31101: [ThinLTO] Use clang's existing code gen handling for ThinLTO backends

tejohnson created this revision. Herald added subscribers: Prazek, mehdi_amini. We noticed that when invoking the thinBackend via clang (for the distributed build case) that flags like -ffunction-sections and -emit-llvm were not having the intended effect. This could have been fixed by setting up

[PATCH] D31114: Refactor `initTargetOptions` out of `EmitAssemblyHelper::CreateTargetMachine` and use it to initialize TargetOptions for ThinLTO Backends

tejohnson added a comment. I think you won't get the correct handling of -emit-llvm and -emit-llvm-bc since we don't get the handling for Backend_Emit* in EmitAssemblyHelper::EmitAssembly. Comment at: clang/lib/CodeGen/BackendUtil.cpp:595 llvm::Optional RM; RM = llvm::S

[PATCH] D31114: Refactor `initTargetOptions` out of `EmitAssemblyHelper::CreateTargetMachine` and use it to initialize TargetOptions for ThinLTO Backends

tejohnson added a comment. In https://reviews.llvm.org/D31114#704726, @mehdi_amini wrote: > In https://reviews.llvm.org/D31114#704649, @tejohnson wrote: > > > I think you won't get the correct handling of -emit-llvm and -emit-llvm-bc > > since we don't get the handling for Backend_Emit* in > >

[PATCH] D31114: Refactor `initTargetOptions` out of `EmitAssemblyHelper::CreateTargetMachine` and use it to initialize TargetOptions for ThinLTO Backends

tejohnson added a comment. In https://reviews.llvm.org/D31114#704733, @mehdi_amini wrote: > In https://reviews.llvm.org/D31114#704728, @tejohnson wrote: > > > In https://reviews.llvm.org/D31114#704726, @mehdi_amini wrote: > > > > > In https://reviews.llvm.org/D31114#704649, @tejohnson wrote: > >

[PATCH] D31101: [ThinLTO] Use clang's existing code gen handling for ThinLTO backends

tejohnson updated this revision to Diff 92324. tejohnson added a comment. As discussed in the review threads for https://reviews.llvm.org/D31114 and https://reviews.llvm.org/D31100, only fall back to clang's output file emission for -emit-llvm and -emit-llvm-bc. https://reviews.llvm.org/D31101

[PATCH] D31114: Refactor `initTargetOptions` out of `EmitAssemblyHelper::CreateTargetMachine` and use it to initialize TargetOptions for ThinLTO Backends

tejohnson added a comment. In https://reviews.llvm.org/D31114#704749, @mehdi_amini wrote: > In https://reviews.llvm.org/D31114#704748, @tejohnson wrote: > > > In https://reviews.llvm.org/D31114#704733, @mehdi_amini wrote: > > > > > In https://reviews.llvm.org/D31114#704728, @tejohnson wrote: > >

[PATCH] D31101: [ThinLTO] Use clang's existing code gen handling for ThinLTO backends

tejohnson added inline comments. Comment at: lib/CodeGen/BackendUtil.cpp:1007 else AsmHelper.EmitAssembly(Action, std::move(OS)); I just noticed that EmitAssembly does a lot more than just emission - it is also setting up an optimization pipeline in Cr

[PATCH] D31202: Clang change: Do not inline hot callsites for samplepgo in thinlto compile phase.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D31202 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D31050: [ThinLTO] Clang support for emitting minimized bitcode for thin link

tejohnson updated this revision to Diff 92647. tejohnson added a comment. Update as per review comments on llvm side patch https://reviews.llvm.org/D31027. https://reviews.llvm.org/D31050 Files: include/clang/Driver/CC1Options.td include/clang/Frontend/CodeGenOptions.h lib/CodeGen/Backen

[PATCH] D31050: [ThinLTO] Clang support for emitting minimized bitcode for thin link

This revision was automatically updated to reflect the committed changes. Closed by commit rL298639: [ThinLTO] Clang support for emitting minimized bitcode for thin link (authored by tejohnson). Changed prior to commit: https://reviews.llvm.org/D31050?vs=92647&id=92849#toc Repository: rL LLV

[PATCH] D31219: Update the SamplePGO test to verify that unroll/icp is not invoked in thinlto compile phase.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM but please add comments about what we're testing. https://reviews.llvm.org/D31219 ___ cfe-commits mailing list cfe-commits@lists.llvm.

[PATCH] D31114: Refactor `initTargetOptions` out of `EmitAssemblyHelper::CreateTargetMachine` and use it to initialize TargetOptions for ThinLTO Backends

tejohnson added a comment. Ping - I think this just needs a test in addition to undoing the AsmHelper definition move. I have some follow on changes I want to send that set up the rest of the lto::Config fields (e.g. the RelocModel etc). https://reviews.llvm.org/D31114 _

[PATCH] D31101: [ThinLTO] Use clang's existing code gen handling for ThinLTO backends

tejohnson added inline comments. Comment at: lib/CodeGen/BackendUtil.cpp:1007 else AsmHelper.EmitAssembly(Action, std::move(OS)); tejohnson wrote: > I just noticed that EmitAssembly does a lot more than just emission - it is > also setting up an optimi

[PATCH] D31101: [ThinLTO] Use clang's existing code gen handling for ThinLTO backends

tejohnson updated this revision to Diff 93535. tejohnson added a comment. Use LTO to emit LLVM IR https://reviews.llvm.org/D31101 Files: lib/CodeGen/BackendUtil.cpp test/CodeGen/thinlto-emit-llvm.c Index: test/CodeGen/thinlto-emit-llvm.c ===

[PATCH] D31508: [ThinLTO] Set up lto::Config properly for codegen in ThinLTO backends

tejohnson created this revision. Herald added a subscriber: Prazek. This involved refactoring out pieces of EmitAssemblyHelper::CreateTargetMachine for use in runThinLTOBackend. Subsumes https://reviews.llvm.org/D31114. https://reviews.llvm.org/D31508 Files: lib/CodeGen/BackendUtil.cpp tes

[PATCH] D31114: Refactor `initTargetOptions` out of `EmitAssemblyHelper::CreateTargetMachine` and use it to initialize TargetOptions for ThinLTO Backends

tejohnson added a comment. As discussed with Mehdi offline, I am taking this one over. I just mailed https://reviews.llvm.org/D31508 which supersedes this one. https://reviews.llvm.org/D31114 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D31508: [ThinLTO] Set up lto::Config properly for codegen in ThinLTO backends

This revision was automatically updated to reflect the committed changes. Closed by commit rL299152: [ThinLTO] Set up lto::Config properly for codegen in ThinLTO backends (authored by tejohnson). Changed prior to commit: https://reviews.llvm.org/D31508?vs=93536&id=93577#toc Repository: rL LL

[PATCH] D31534: [ThinLTO] Handle -emit-llvm* in ThinLTO backends

tejohnson created this revision. Herald added a subscriber: Prazek. Use PreCodeGenModuleHook to invoke the correct writer when emitting LLVM IR, returning false to skip codegen from within thinBackend. https://reviews.llvm.org/D31534 Files: lib/CodeGen/BackendUtil.cpp test/CodeGen/thinlto-e

[PATCH] D31101: [ThinLTO] Use clang's existing code gen handling for ThinLTO backends

tejohnson abandoned this revision. tejohnson added a comment. Subsumed by https://reviews.llvm.org/D31534 https://reviews.llvm.org/D31101 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-comm

[PATCH] D31534: [ThinLTO] Handle -emit-llvm* in ThinLTO backends

tejohnson updated this revision to Diff 93695. tejohnson added a comment. Implement review suggestions https://reviews.llvm.org/D31534 Files: lib/CodeGen/BackendUtil.cpp test/CodeGen/thinlto-emit-llvm.c Index: test/CodeGen/thinlto-emit-llvm.c ==

[PATCH] D31534: [ThinLTO] Handle -emit-llvm* in ThinLTO backends

This revision was automatically updated to reflect the committed changes. Closed by commit rL299274: [ThinLTO] Handle -emit-llvm* in ThinLTO backends (authored by tejohnson). Changed prior to commit: https://reviews.llvm.org/D31534?vs=93695&id=93722#toc Repository: rL LLVM https://reviews.l

[PATCH] D31508: [ThinLTO] Set up lto::Config properly for codegen in ThinLTO backends

tejohnson added a comment. In https://reviews.llvm.org/D31508#716087, @davide wrote: > clang -cc1 crashes if an invalid reloc model is passed (via > `-mreloc-model=`), see https://bugs.llvm.org/show_bug.cgi?id=32490 > I'm not sure if the bug was already there or this refactoring exposed it. >

[PATCH] D28588: Pass -fprofile-sample-use to lto backends.

tejohnson added a comment. In https://reviews.llvm.org/D28588#644467, @danielcdh wrote: > The breaks some buildbots thus I reverted the patch: > > http://lab.llvm.org:8011/builders/clang-x86-windows-msvc2015/builds/1889 > > http://lab.llvm.org:8080/green/job/clang-stage1-cmake-RA-incremental_ch

[PATCH] D28588: Pass -fprofile-sample-use to lto backends.

tejohnson added a comment. In https://reviews.llvm.org/D28588#644472, @tejohnson wrote: > Otherwise I think "; REQUIRES: asserts" might do the trick? Looks like it - I looked at another test that used -debug output and it requires asserts. https://reviews.llvm.org/D28588 _

[PATCH] D28588: Pass -fprofile-sample-use to lto backends.

tejohnson added a comment. In https://reviews.llvm.org/D28588#644489, @danielcdh wrote: > Thanks for the prompt response. > > But looks like several other tests also has "-mllvm -debug-pass=Structure" in > their tests: > > tools/clang/test/CodeGen/pgo-instrumentation.c > test/CodeGen/Generic/ll

[PATCH] D28588: Pass -fprofile-sample-use to lto backends.

tejohnson added a comment. In https://reviews.llvm.org/D28588#644637, @danielcdh wrote: > Looks like this is still breaking these buildbots: > > http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/3216/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Athinlto_backend.ll > > I reverted th

[PATCH] D28746: Mention ThinLTO with PGO in ReleaseNotes 4.0

tejohnson added a comment. In https://reviews.llvm.org/D28746#646793, @Prazek wrote: > Maybe I should change ThinLTO for '-flto=thin' and also mention this > improvement in LLVM release notes? > > BTW '-flto=thin' is not documented in UsersManual It's documented here: http://clang.llvm.org/

[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.

tejohnson added a comment. > TODO: avoid breaking Darwin. How does this break Darwin? Comment at: clang/lib/CodeGen/BackendUtil.cpp:694 +else + PerModulePasses.add( + createBitcodeWriterPass(*OS, CodeGenOpts.EmitLLVMUseLists)); Can we transfo

[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.

tejohnson added inline comments. Comment at: clang/test/CodeGenCXX/type-metadata-thinlto.cpp:2 +// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s +// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s + ---

[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.

tejohnson added inline comments. Comment at: clang/test/CodeGenCXX/type-metadata-thinlto.cpp:2 +// RUN: %clang_cc1 -flto=thin -triple x86_64-unknown-linux -fvisibility hidden -emit-llvm-bc -o %t %s +// RUN: llvm-modextract -o - -n 1 %t | llvm-dis | FileCheck %s + ---

[PATCH] D28843: IRGen: Start using the WriteThinLTOBitcode pass.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D28843 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D29067: IRGen: When loading the main module in the distributed ThinLTO backend, look for the module containing the summary.

tejohnson added inline comments. Comment at: clang/lib/CodeGen/BackendUtil.cpp:867 +Expected clang::FindThinLTOModule(MemoryBufferRef MBRef) { + Expected> BMsOrErr = getBitcodeModuleList(MBRef); Would it be better to have this in llvm (e.g. in BitcodeReader li

[PATCH] D29067: IRGen: When loading the main module in the distributed ThinLTO backend, look for the module containing the summary.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D29067 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D34728: [ThinkLTO] Invoke build(Thin)?LTOPreLinkDefaultPipeline.

tejohnson added inline comments. Comment at: llvm/test/Other/new-pm-thinlto-defaults.ll:157 ; CHECK-PRELINK-O-NEXT: Running pass: GlobalOptPass -; CHECK-PRELINK-O-NEXT: Running pass: NameAnonGlobalPass ; CHECK-POSTLINK-O-NEXT: Running pass: PassManager<{{.*}}Module{{.*}}> -

[PATCH] D34721: [PM] Add support for sample PGO in the new pass manager (clang-side)

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM for when the dependent patches are in. https://reviews.llvm.org/D34721 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://

[PATCH] D34728: [ThinkLTO] Invoke build(Thin)?LTOPreLinkDefaultPipeline.

tejohnson added a comment. In https://reviews.llvm.org/D34728#793131, @timshen wrote: > A question I have is that I don't know how to test this. Ideally we want > -debug-pass-manager from opt, but that flag is not part of the LLVM libraries. How about add a clang test that builds with "-mllvm

[PATCH] D34728: [ThinkLTO] Invoke build(Thin)?LTOPreLinkDefaultPipeline.

tejohnson added a comment. In https://reviews.llvm.org/D34728#794508, @timshen wrote: > Added -fexperimental-new-pass-manager=off/on/debug for printing debug > information. > > Added a Clang test. > > Do tell if you want me to split this patch. I didn't, becuase then I don't > have to write a t

[PATCH] D34790: [NewPM] Add Clang cc1 flag -fdebug-pass-manager for printing debug information.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM. Just one minor nit on the option help string below. Thanks Comment at: clang/include/clang/Driver/CC1Options.td:329 +def fno_debug_pass_manager : Flag<["-"], "fno

[PATCH] D34896: Enable the new PM + SamlePGO + ThinLTO testing.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D34896 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

tejohnson added a comment. How are you invoking this? Typically this path is invoked for distributed builds, when the individual index files for each backend were written during the thin link (e.g. via gold plugin's -thinlto-index-only option). In that case, we only expect a single summary to e

[PATCH] D35153: Use DenseMap instead std::map for GVSummaryMapTy

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D35153 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

tejohnson added a comment. In https://reviews.llvm.org/D35081#805127, @fhahn wrote: > To reproduce the issue you could use > > +; Check that we only add a single summary entry for multiple definitions > +; of a linkonce_odr function > + > +; RUN: opt -module-summary %s -o %t1.bc > +; RU

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

tejohnson added a comment. In https://reviews.llvm.org/D35081#805506, @yunlian wrote: > This error happens when I try to triage a thinLTO failure on ARM. > > Initially I got some error like > Instruction does not dominate all uses! > > %205 = bitcast i1 (%"class.blink::LayoutObject"*)** %194

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

tejohnson added a comment. In https://reviews.llvm.org/D35081#805761, @yunlian wrote: > I've sent a reproduce test case to tejohnson. FYI I tracked down what is going on here and reproduced with a small test case. Essentially, two copies of a linkonce odr function were optimized somewhat diff

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

tejohnson added a comment. In https://reviews.llvm.org/D35081#807497, @mehdi_amini wrote: > In https://reviews.llvm.org/D35081#807172, @fhahn wrote: > > > It's @yunlian, so if the issue you described above covers his failure, than > > that's great. @tejohnson do you have time to work on a fix so

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

tejohnson added a comment. In https://reviews.llvm.org/D35081#808124, @mehdi_amini wrote: > Teresa: can you describe a bit more how we end up importing two summaries for > the same GUID? I would expect that after importing one, we don't even come to > try to import another since we already have

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

tejohnson added a comment. In https://reviews.llvm.org/D35081#808158, @mehdi_amini wrote: > Thanks, very clear :) > > I would expect that if we reprocess a GUID we invalidate the previous import > of the same GUID. Right now my impression is that the issue is that ` > ImportList[ExportModulePat

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

tejohnson added a comment. In https://reviews.llvm.org/D35081#808204, @mehdi_amini wrote: > In https://reviews.llvm.org/D35081#808189, @tejohnson wrote: > > > > Alternatively (and likely preferably from a compile-time point of view to > > > limit the list of import), we could keep a map of GUID-

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

tejohnson added a comment. In https://reviews.llvm.org/D35081#808517, @mehdi_amini wrote: > In https://reviews.llvm.org/D35081#808207, @tejohnson wrote: > > > My first option was if any copy is under the threshold, simply pick the > > prevailing copy (it may be over threshold, but assume we want

[PATCH] D35081: [ThinLTO] Allow multiple summary entries.

tejohnson added a comment. In https://reviews.llvm.org/D35081#808789, @tejohnson wrote: > In https://reviews.llvm.org/D35081#808517, @mehdi_amini wrote: > > > In https://reviews.llvm.org/D35081#808207, @tejohnson wrote: > > > > > My first option was if any copy is under the threshold, simply pick

[PATCH] D42680: [ThinLTO] Ignore -fthinlto-index= for non-ThinLTO files

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM (minor comment fix in test needed) Comment at: clang/test/CodeGen/thinlto_backend.ll:23 +; Ensure we don't fail with index and non-ThinLTO object file, and run

[PATCH] D42680: [ThinLTO] Ignore -fthinlto-index= for non-ThinLTO files

tejohnson added a comment. In https://reviews.llvm.org/D42680#991938, @pcc wrote: > This doesn't seem right to me. In a mixed full/thin LTO link the full LTO > module would be compiled during the indexing phase. We don't want to compile > it again in the backend as it could lead at best to dupl

[PATCH] D42680: [ThinLTO] Ignore object files with no ThinLTO modules if -fthinlto-index= is set

tejohnson added a comment. This looks ok to me, assuming it produces the empty output file as I expect it should. Please wait for pcc to take a look as well. Comment at: clang/lib/CodeGen/CodeGenAction.cpp:959 +if (!Bm) + return {}; Expected> MOrErr = ---

[PATCH] D30792: Use callback for internalizing linked symbols.

tejohnson added a comment. Great, thanks. One question below and a format nit. Comment at: lib/CodeGen/CodeGenAction.cpp:177 +if (LM.Internalize) { + Err = Linker::linkModules( + *getModule(), std::move(LM.Module), LM.LinkFlags, Need

[PATCH] D30792: Use callback for internalizing linked symbols.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM with the format fix I pointed out. Comment at: lib/CodeGen/CodeGenAction.cpp:186 +} else { + Err = Linker::linkModules(*getModule(), std::move(LM.

[PATCH] D30920: Do not pass -Os and -Oz to the Gold plugin

tejohnson added a comment. Interested in pcc's thoughts, as https://bugs.llvm.org/show_bug.cgi?id=32155 mentioned you already discussed with him. Note that some of the passes that check PassManagerBuilder::sizeLevel are added during the ThinLTO back end (e.g. populateModulePassManager which che

[PATCH] D30920: Do not pass -Os and -Oz to the Gold plugin

tejohnson added a comment. In https://reviews.llvm.org/D30920#700133, @pcc wrote: > In https://reviews.llvm.org/D30920#700077, @tejohnson wrote: > > > Until everything is converted to using size attributes, it seems like a > > correct fix for the bug is to accept these options in the gold-plugin

[PATCH] D33134: Remove ignore-empty-index-file option

tejohnson created this revision. Herald added subscribers: eraman, mehdi_amini. Clang changes to remove this option and replace with a parameter always set in the context of a ThinLTO distributed backend. Depends on https://reviews.llvm.org/D33133. https://reviews.llvm.org/D33134 Files: lib/

[PATCH] D33134: Remove ignore-empty-index-file option

This revision was automatically updated to reflect the committed changes. Closed by commit rL302940: Remove ignore-empty-index-file option (authored by tejohnson). Changed prior to commit: https://reviews.llvm.org/D33134?vs=98779&id=98821#toc Repository: rL LLVM https://reviews.llvm.org/D33

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

tejohnson added inline comments. Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:93 - virtual bool isIRLevelProfile() const = 0; - It seems like these helper methods could still be defined using the new bitset, which would reduce some of the code

[PATCH] D115393: [InstrProf][NFC] Refactor Profile kind into a bitset enum.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. lgtm Comment at: llvm/include/llvm/ProfileData/InstrProfWriter.h:48 public: - InstrProfWriter(bool Sparse = false, bool InstrEntryBBEnabled = false); ~InstrProfWri

[PATCH] D115492: [LTO] Ignore unreachable virtual functions in WPD in hybrid LTO

tejohnson added inline comments. Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:241 +// to be unreachable; if it returns false, `F` might still +// be unreachble but not covered by this helper function. +static bool mustBeUnreachableFunction(const Function &F) {

[PATCH] D115492: [LTO] Ignore unreachable virtual functions in WPD in hybrid LTO

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. lgtm after adding a description to test as noted below. Comment at: llvm/test/ThinLTO/X86/devirt_hybrid_after_filtering_unreachable.ll:1 +; Generate split module with s

[PATCH] D115648: [LTO] Ignore unreachable virtual functions in WPD in thin LTO

tejohnson added inline comments. Comment at: llvm/test/ThinLTO/X86/devirt_after_filtering_unreachable.ll:64 +; Check that EnableSplitLTOUnit is off, and check the content of summary information. +; RUN: llvm-dis -o - %t3.o | FileCheck %s --check-prefix=NOENABLESPLITFLAG +; NOENA

[PATCH] D115648: [LTO] Ignore unreachable virtual functions in WPD in thin LTO

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. lgtm with one more test simplification below. Comment at: llvm/test/ThinLTO/X86/devirt_after_filtering_unreachable.ll:11 +; RUN: llvm-dis -o - %t-main.bc.0 | FileCheck

[PATCH] D115648: [LTO] Ignore unreachable virtual functions in WPD in thin LTO

tejohnson added inline comments. Comment at: llvm/test/ThinLTO/X86/devirt_after_filtering_unreachable.ll:11 +; RUN: llvm-dis -o - %t-main.bc.0 | FileCheck %s --check-prefix=ENABLESPLITFLAG +; ENABLESPLITFLAG: gv: (name: "_ZN4BaseD0Ev", {{.*}}, funcFlags: ({{.*}} mustBeUnreachabl

[PATCH] D115648: [LTO] Ignore unreachable virtual functions in WPD in thin LTO

tejohnson added inline comments. Comment at: llvm/test/ThinLTO/X86/devirt_after_filtering_unreachable.ll:16 + +; ENABLESPLITFLAG: gv: (name: "_ZN4BaseD0Ev", {{.*}}, funcFlags: ({{.*}} mustBeUnreachable: 1 + I think it can be shared with the equivalent checks in

[PATCH] D99554: [ThinLTO] During module importing, close one source module before open another one for distributed mode.

tejohnson accepted this revision. tejohnson added a comment. This revision is now accepted and ready to land. LGTM with a couple of comment requests noted below. Comment at: clang/lib/CodeGen/BackendUtil.cpp:1583 ModuleToDefinedGVSummaries[M->getModuleIde

[PATCH] D99683: [HIP] Support ThinLTO

tejohnson added a comment. I haven't looked extensively yet, but why import noinline functions? Also, please add a patch description. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99683/new/ https://reviews.llvm.org/D99683 ___ cfe-commits ma

[PATCH] D99683: [HIP] Support ThinLTO

tejohnson added a comment. In D99683#2669047 , @yaxunl wrote: > In D99683#2664674 , @tejohnson wrote: > >> I haven't looked extensively yet, but why import noinline functions? Also, >> please add a patch descriptio

[PATCH] D99683: [HIP] Support ThinLTO

tejohnson added a comment. In D99683#2671727 , @yaxunl wrote: > In D99683#2669136 , @yaxunl wrote: > >> In D99683#2669080 , @tejohnson >> wrote: >> >>> In D99683#2669047

[PATCH] D99683: [HIP] Support ThinLTO

tejohnson added a comment. In D99683#2672578 , @yaxunl wrote: > In D99683#2672554 , @tejohnson wrote: > >> This raises some higher level questions for me: >> >> First, how will you deal with other corner cases that

[PATCH] D99683: [HIP] Support ThinLTO

tejohnson added a comment. In D99683#2677357 , @yaxunl wrote: > Any other concerns? Thanks. I have some concerns around the fragility of this, for the reasons I mentioned earlier where it may not always be able to import everything needed. > We will doc

[PATCH] D99683: [HIP] Support ThinLTO

tejohnson requested changes to this revision. tejohnson added a comment. This revision now requires changes to proceed. To do what I suggested in the prior comment, you'd probably want to add a new index-wide flag (since we don't read IR in the thin link). See for example how EnableSplitLTOUnit

[PATCH] D100484: add test case for ignoring -flto=auto and -flto=jobserver

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

[PATCH] D99501: ignore -flto= options recognized by GCC

tejohnson added a comment. Just approved the test case patch. Sorry I missed the lack of test on this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99501/new/ https://reviews.llvm.org/D99501 ___ cfe-com

[PATCH] D99554: [ThinLTO] During module importing, close one source module before open another one for distributed mode.

tejohnson added a comment. In D99554#2674229 , @wenlei wrote: > Does this help non-distributed ThinLTO as well? cc: @weiwang No. See the note in Wei's description about this: > Note that this patch only changes the distributed thinlto mode. For in > pro

[PATCH] D94647: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for -fthinlto-index=

tejohnson added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3758 + // Normally -gsplit-dwarf is only useful with -gN. For -gsplit-dwarf in the + // backend phase of a distributed ThinLTO which does object file generation + // and no IR generation, -gN sh

[PATCH] D94647: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for -fthinlto-index=

tejohnson added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3758 + // Normally -gsplit-dwarf is only useful with -gN. For -gsplit-dwarf in the + // backend phase of a distributed ThinLTO which does object file generation + // and no IR generation, -gN sh

[PATCH] D94655: [Driver] -gsplit-dwarf: Produce .dwo regardless of -gN for IR input

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

[PATCH] D111371: [Support][ThinLTO] Move ThinLTO caching to LLVM Support library.

tejohnson added a comment. This lgtm but I agree with @phosek 's suggestion to configure the file prefix and keep it as "Thin" for ThinLTO. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111371/new/ https://reviews.llvm.org/D111371 ___

[PATCH] D111371: [Support][ThinLTO] Move ThinLTO caching to LLVM Support library.

tejohnson added inline comments. Comment at: llvm/include/llvm/Support/Caching.h:68 +/// already exist. +Expected localCache(StringRef CacheDirectoryPath, + AddBufferFn AddBuffer, Nit: document new parameter. ==

[PATCH] D111371: [Support][ThinLTO] Move ThinLTO caching to LLVM Support library.

tejohnson added inline comments. Comment at: llvm/lib/Support/Caching.cpp:36 + SmallString<10> CacheName = CacheNameRef; return [=](unsigned Task, StringRef Key) -> AddStreamFn { noajshu wrote: > noajshu wrote: > > tejohnson wrote: > > > Is this copy necess

[PATCH] D111371: [Support][ThinLTO] Move ThinLTO caching to LLVM Support library.

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

[PATCH] D112177: [clang] Do not duplicate "EnableSplitLTOUnit" module flag

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

<    1   2   3   4   5   6   7   >