<div dir="ltr">We could add a validatePassPipeline API to the PassBuilder maybe... My only hesitation is that I'd like for targets to be able to register passes here, and that might make it hard to validate without a TargetMachine. Fundamentally, I suspect that we will want to do pipeline validation based on the actual target machine that will be used, so it may not make sense to hoist stuff past a point where you have that.</div><br><div class="gmail_quote"><div dir="ltr">On Mon, May 16, 2016 at 1:43 PM Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">I'm not sure if you can instantiate these classes in the driver. In the driver, we don't even know what architecture the target machine is.<div><br></div><div>Can you call parsePassPipeline with a dummy TargetMachine and an empty PassManager to verify a string?<br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, May 16, 2016 at 1:10 PM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On Mon, May 16, 2016 at 10:39 AM, Rui Ueyama <<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>> wrote:<br>
> On Sun, May 15, 2016 at 12:29 PM, Davide Italiano via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> Author: davide<br>
>> Date: Sun May 15 14:29:38 2016<br>
>> New Revision: 269605<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=269605&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=269605&view=rev</a><br>
>> Log:<br>
>> [LTO] Add the ability to specify a subset of passes to run.<br>
>><br>
>> Differential Revision: <a href="http://reviews.llvm.org/D20267" rel="noreferrer" target="_blank">http://reviews.llvm.org/D20267</a><br>
>><br>
>> Added:<br>
>> lld/trunk/test/ELF/lto/ltopasses-custom.ll<br>
>> Modified:<br>
>> lld/trunk/ELF/CMakeLists.txt<br>
>> lld/trunk/ELF/Config.h<br>
>> lld/trunk/ELF/Driver.cpp<br>
>> lld/trunk/ELF/LTO.cpp<br>
>> lld/trunk/ELF/Options.td<br>
>><br>
>> Modified: lld/trunk/ELF/CMakeLists.txt<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/CMakeLists.txt?rev=269605&r1=269604&r2=269605&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/CMakeLists.txt?rev=269605&r1=269604&r2=269605&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- lld/trunk/ELF/CMakeLists.txt (original)<br>
>> +++ lld/trunk/ELF/CMakeLists.txt Sun May 15 14:29:38 2016<br>
>> @@ -31,6 +31,7 @@ add_lld_library(lldELF<br>
>> Linker<br>
>> Object<br>
>> Option<br>
>> + Passes<br>
>> MC<br>
>> Support<br>
>> Target<br>
>><br>
>> Modified: lld/trunk/ELF/Config.h<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Config.h?rev=269605&r1=269604&r2=269605&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Config.h?rev=269605&r1=269604&r2=269605&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- lld/trunk/ELF/Config.h (original)<br>
>> +++ lld/trunk/ELF/Config.h Sun May 15 14:29:38 2016<br>
>> @@ -44,6 +44,7 @@ struct Configuration {<br>
>> llvm::StringRef Emulation;<br>
>> llvm::StringRef Fini;<br>
>> llvm::StringRef Init;<br>
>> + llvm::StringRef LtoNewPmPasses;<br>
>> llvm::StringRef OutputFile;<br>
>> llvm::StringRef SoName;<br>
>> llvm::StringRef Sysroot;<br>
>><br>
>> Modified: lld/trunk/ELF/Driver.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Driver.cpp?rev=269605&r1=269604&r2=269605&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Driver.cpp?rev=269605&r1=269604&r2=269605&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- lld/trunk/ELF/Driver.cpp (original)<br>
>> +++ lld/trunk/ELF/Driver.cpp Sun May 15 14:29:38 2016<br>
>> @@ -337,6 +337,7 @@ void LinkerDriver::readConfigs(opt::Inpu<br>
>> Config->Entry = getString(Args, OPT_entry);<br>
>> Config->Fini = getString(Args, OPT_fini, "_fini");<br>
>> Config->Init = getString(Args, OPT_init, "_init");<br>
>> + Config->LtoNewPmPasses = getString(Args, OPT_lto_newpm_passes);<br>
>> Config->OutputFile = getString(Args, OPT_o);<br>
>> Config->SoName = getString(Args, OPT_soname);<br>
>> Config->Sysroot = getString(Args, OPT_sysroot);<br>
>> @@ -495,6 +496,8 @@ template <class ELFT> void LinkerDriver:<br>
>> Symtab.scanVersionScript();<br>
>><br>
>> Symtab.addCombinedLtoObject();<br>
>> + if (HasError)<br>
>> + return;<br>
>><br>
>> for (auto *Arg : Args.filtered(OPT_wrap))<br>
>> Symtab.wrap(Arg->getValue());<br>
>><br>
>> Modified: lld/trunk/ELF/LTO.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/LTO.cpp?rev=269605&r1=269604&r2=269605&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/LTO.cpp?rev=269605&r1=269604&r2=269605&view=diff</a><br>
>><br>
>> ==============================================================================<br>
>> --- lld/trunk/ELF/LTO.cpp (original)<br>
>> +++ lld/trunk/ELF/LTO.cpp Sun May 15 14:29:38 2016<br>
>> @@ -13,6 +13,9 @@<br>
>> #include "Error.h"<br>
>> #include "InputFiles.h"<br>
>> #include "Symbols.h"<br>
>> +#include "llvm/Analysis/AliasAnalysis.h"<br>
>> +#include "llvm/Analysis/CGSCCPassManager.h"<br>
>> +#include "llvm/Analysis/LoopPassManager.h"<br>
>> #include "llvm/Analysis/TargetLibraryInfo.h"<br>
>> #include "llvm/Analysis/TargetTransformInfo.h"<br>
>> #include "llvm/Bitcode/ReaderWriter.h"<br>
>> @@ -20,13 +23,16 @@<br>
>> #include "llvm/CodeGen/ParallelCG.h"<br>
>> #include "llvm/IR/AutoUpgrade.h"<br>
>> #include "llvm/IR/LegacyPassManager.h"<br>
>> +#include "llvm/IR/PassManager.h"<br>
>> #include "llvm/Linker/IRMover.h"<br>
>> +#include "llvm/Passes/PassBuilder.h"<br>
>> #include "llvm/Support/StringSaver.h"<br>
>> #include "llvm/Support/TargetRegistry.h"<br>
>> #include "llvm/Target/TargetMachine.h"<br>
>> #include "llvm/Transforms/IPO.h"<br>
>> #include "llvm/Transforms/IPO/PassManagerBuilder.h"<br>
>> #include "llvm/Transforms/Utils/ModuleUtils.h"<br>
>> +#include <llvm/IR/Verifier.h><br>
>><br>
>> using namespace llvm;<br>
>> using namespace llvm::object;<br>
>> @@ -56,10 +62,44 @@ static void saveBCFile(Module &M, String<br>
>> WriteBitcodeToFile(&M, OS, /* ShouldPreserveUseListOrder */ true);<br>
>> }<br>
>><br>
>> -// Run LTO passes.<br>
>> -// Note that the gold plugin has a similar piece of code, so<br>
>> -// it is probably better to move this code to a common place.<br>
>> -static void runLTOPasses(Module &M, TargetMachine &TM) {<br>
>> +static void runNewCustomLtoPasses(Module &M, TargetMachine &TM) {<br>
>> + PassBuilder PB(&TM);<br>
>> +<br>
>> + AAManager AA;<br>
>> + LoopAnalysisManager LAM;<br>
>> + FunctionAnalysisManager FAM;<br>
>> + CGSCCAnalysisManager CGAM;<br>
>> + ModuleAnalysisManager MAM;<br>
>> +<br>
>> + // Register the AA manager first so that our version is the one used.<br>
>> + FAM.registerPass([&] { return std::move(AA); });<br>
>> +<br>
>> + // Register all the basic analyses with the managers.<br>
>> + PB.registerModuleAnalyses(MAM);<br>
>> + PB.registerCGSCCAnalyses(CGAM);<br>
>> + PB.registerFunctionAnalyses(FAM);<br>
>> + PB.registerLoopAnalyses(LAM);<br>
>> + PB.crossRegisterProxies(LAM, FAM, CGAM, MAM);<br>
>> +<br>
>> + ModulePassManager MPM;<br>
>> + if (!Config->DisableVerify)<br>
>> + MPM.addPass(VerifierPass());<br>
>> +<br>
>> + // Now, add all the passes we've been requested to.<br>
>> + if (!PB.parsePassPipeline(MPM, Config->LtoNewPmPasses)) {<br>
><br>
><br>
> Is there any way to validate a string earlier than this? It would not only<br>
> make error handling simpler but also make it more user friendly as user will<br>
> get an error message immediately rather than after all symbols are resolved.<br>
><br>
<br>
</div></div>Hmm, I agree it's very annoying detecting mistakes so late in the<br>
linking process. I originally thought about it.<br>
The real issue is that you need to instantiate an object `PassBuilder`<br>
on which you call the parse* functions, and the parse functions add<br>
passes to the pipeline on the fly. We could probably move the<br>
validation to the stage of command line parsing, build the pipeline<br>
there, add PB as part of Config, and use that instead of the string in<br>
runNewCustomLtoPasses.<br>
Would you like it better?<br>
<span><font color="#888888"><br>
--<br>
Davide<br>
<br>
"There are no solved problems; there are only problems that are more<br>
or less solved" -- Henri Poincare<br>
</font></span></blockquote></div><br></div>
</blockquote></div>