<div dir="ltr"><div dir="ltr">On Fri, Jul 2, 2021 at 3:26 PM Fāng-ruì Sòng <<a href="mailto:maskray@google.com">maskray@google.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Fri, Jul 2, 2021 at 3:21 PM Mehdi AMINI <<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 2, 2021 at 2:09 PM Fāng-ruì Sòng <<a href="mailto:maskray@google.com" target="_blank">maskray@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Fri, Jul 2, 2021 at 2:03 PM Mehdi AMINI <<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 2, 2021 at 1:27 PM Fāng-ruì Sòng <<a href="mailto:maskray@google.com" target="_blank">maskray@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">On Fri, Jul 2, 2021 at 1:10 PM Mehdi AMINI <<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 2, 2021 at 12:49 PM Alexandre Ganea <<a href="mailto:alexandre.ganea@ubisoft.com" target="_blank">alexandre.ganea@ubisoft.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div lang="EN-CA">
<div>
<div>
<div>
<blockquote style="border-style:none none none solid;border-left-width:1pt;border-left-color:rgb(204,204,204);padding:0cm 0cm 0cm 6pt;margin-left:4.8pt;margin-right:0cm">
<div>
<div>
<div>
<p class="MsoNormal">The API benefits sound nice, though presumably some could be retrofitted to cl::opt if that was the only goal. Side benefits in addition to removing global ctors are nice to have.<br>
<u></u><u></u></p>
</div>
<blockquote style="border-style:none none none solid;border-left-width:1pt;border-left-color:rgb(204,204,204);padding:0cm 0cm 0cm 6pt;margin-left:4.8pt;margin-right:0cm">
<div>
<div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">The drawback is some initial boilerplate (e.g. llvm-tblgen -gen-opt-parser-defs in CMakeLists.txt, class NmOptTable in code).<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">The handling of comma separated options -arch=x86_64,arm64 doesn't have direct OptTable support. llvm::SplitString is needed (just search for SplitString in <a href="https://reviews.llvm.org/D105330" target="_blank">https://reviews.llvm.org/D105330</a>)<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">But this doesn't tend to increase complexity because the cl::list<std::string> will need per-value verification anyway.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt"> <u></u><u></u></p>
</div>
<blockquote style="border-style:none none none solid;border-left-width:1pt;border-left-color:rgb(204,204,204);padding:0cm 0cm 0cm 6pt;margin-left:4.8pt;margin-right:0cm">
<div>
<div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">One potential one (though I don't recall it being discussed recently) would be that maybe this addresses the issue of global ctors in cl::opt? Does OptTable avoid/not use global constructors? That would be nice
- it's an ongoing issue that LLVM library users pay for command line argument support they have no need for in the form of global ctor execution time.<u></u><u></u></p>
</div>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal" style="margin-left:11.55pt"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">OptTable is used as a local variable. So yes, it avoids global constructors,
<u></u><u></u></p>
</div>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal" style="margin-left:11.55pt"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">Nice :)<u></u><u></u></p>
</div>
</div>
</div>
</blockquote>
<div>
<p class="MsoNormal" style="margin-left:11.55pt"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">Note that MLIR is using cl::opt without global ctor (we build with `-Werror=global-constructors`).<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">The pattern we use to write a tool with cl::opt and avoid global ctor (and can be used to avoid collision) looks like: <a href="https://github.com/llvm/llvm-project/blob/main/mlir/lib/IR/MLIRContext.cpp#L57-L83" target="_blank">https://github.com/llvm/llvm-project/blob/main/mlir/lib/IR/MLIRContext.cpp#L57-L83</a><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">The tool that wants to expose the MLIRContext options to the command line calls registerMLIRContextCLOptions() before parsing the command line.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">Wouldn't this translate directly to LLVM tools as well with some minor refactoring?<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">The same applies to all of the infrastructure in MLIR, passes are registered explicitly, etc. This decouples the "is this code linked in" from "options are loaded" annoying part of the global constructors.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">-- <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal" style="margin-left:11.55pt">Mehdi<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal"><b><i>[Alexandre Ganea] </i></b>I think one other issue with cl::opt is that it aggregates the “command-line argument definition” and the “runtime parameter”
<i>de facto</i> in a single object (unless cl::location is manually specified to every cl::opt). What MLIR does solves the issue mentioned by David, the fact that every tool pulls/initializes every cl::opt out there. However OptTable solves both problems, and
makes the entry point thread-safe.<u></u><u></u></p>
<p class="MsoNormal"><u></u> </p></div></div></div></div></div></blockquote><div><br></div><div>I agree that removing the global state would be great!</div><div>Right now what I see proposed with OptTable (like <a href="https://reviews.llvm.org/D104889" target="_blank">https://reviews.llvm.org/D104889</a>) seems to just address the tools-specific options, and the value isn't clear to me for these cases, since these options aren't exposed through library entry points.</div><div>I don't quite get right now how OptTable would compose at the LLVM scale? Are there examples of libraries exposing pluggable hooks for a tool to aggregate multiple libraries' options and expose them on the command line?</div><div><br></div></div></div></blockquote><div> </div><div>The first message listed:</div><div><br></div><div>* -t=d is removed (equal sign after a short option). Use -t d instead.<br>* --demangle=0 (=0 to disable a boolean option) is removed. Omit the option or use --no-demangle instead.<br>* To support boolean options (e.g. --demangle --no-demangle), we don't need to compare their positions (if (NoDemangle.getPosition() > Demangle.getPosition()) , see llvm-nm.cpp)<br>* grouped short options can be specified with one line `setGroupedShortOptions`, instead of adding cl::Grouping to every short options.<br>* We don't need to add cl::cat to every option and call `HideUnrelatedOptions` to hide unrelated options from --help. The issue would happen with cl::opt tools if linker garbage collection is disabled or libLLVM-13git.so is used. (See <a href="https://reviews.llvm.org/D104363" target="_blank">https://reviews.llvm.org/D104363</a>)<br></div><div><br></div><div>To me <b>these points are all usability issues of cl::opt</b>. I care about not exposing unnecessary interfaces so cl::opt accepting the weird -t=d looks a downside to me.</div><div><br></div><div>--demangle=0 is weird and some llvm/llvm/test tests do use cl::opt options this way, so we cannot just remove this usage. As a workaround, we could add a cl::foobar_toggle to a cl::opt to disallow =0.</div><div>We would end with more customization for one option, cl::cat (for hiding unrelated options), cl::foobar_toggle (for disallowing =0), and potentially others for other ad-hoc tasks.</div><div><br></div><div>I can highlight another thing about the global state of cl::opt => <b>library cl::opt and binary utility cl::opt share the same namespace</b>.</div><div>So cl::opt options (usually for debugging or testing) in library code can end up in a tool's list of command line options.</div><div>This is usually undesired (e.g. llvm-objdump --x86-asm-syntax in <a href="https://reviews.llvm.org/D100433" target="_blank">https://reviews.llvm.org/D100433</a>).</div><div>People may not notice this if they always use -DLLVM_LINK_LLVM_DYLIB=off and don't use linker garbage collection.</div></div></div></blockquote><div><br></div><div>You're not answering my question here, are you? Are you answering to what I mentioned 3 emails before in an answer to David when I wrote "Indeed: it isn't clear to me that these are outright "benefits"."?</div></div></div></blockquote><div><br></div><div>OptTable doesn't have the listed usability issues. Not having the issues is a large benefit to me.</div></div></div></blockquote><div><br></div><div>Maybe, these are subjective though.</div><div>(And the confusion above came from the fact that you just answered the wrong email)<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>Because I still don't see clearly how to build something like `opt` with all the pass and the options with OptTable, how does it all compose?</div></div></div></blockquote><div><br></div><div>The proposed changes are specific to binary utilities: llvm-{ar,cxxfilt,nm,objcopy,objdump,readobj,size,strings,symbolizer}.</div><div>'opt', 'llc', etc are not in the scope.</div><div>(I guess I should have named the utilities more specifically to not cause confusion.)</div><div><br></div><div>For llvm-{ar,cxxfilt,nm,objcopy,objdump,readobj,size,strings,symbolizer}, we don't want a random cl::opt in lib/Transforms, lib/MC, orlib/LTO to be accidentally specifiable on the command line.</div><div>(In the rare debugging case where such functionality is needed, it needs a -mllvm prefix like what ld.lld does.)</div></div></div></blockquote><div><br></div><div>Have you looked at what I mentioned with MLIR on how to use cl::opt without global constructor? This has exactly the behavior you're looking for.</div><div><br></div></div></div></blockquote><div> </div><div>I have read <a href="https://github.com/llvm/llvm-project/blob/main/mlir/lib/IR/MLIRContext.cpp#L57-L83" target="_blank">https://github.com/llvm/llvm-project/blob/main/mlir/lib/IR/MLIRContext.cpp#L57-L83</a> </div><div>Unless it does something more specially, I don't think it can avoid the issue I mentioned in a previous message:<br></div><div><br></div><div>> I can highlight another thing about the global state of cl::opt => <b>library cl::opt and binary utility cl::opt share the same namespace</b>.<br></div><div><br></div><div>This can be worked around with linker garbage collection by discarding unreferenced cl::opt.</div></div></div>
</blockquote></div><br><div>I re-read these messages. I think you probably meant something more generic - how to design decentralized command line option registry where every library can register some options.</div><div>The binary utilities command line parsing issue I intended to address is fairly different:<br></div><div>We want a single registry of all options and don't want to inherit options from llvm/lib/A just because the tool happens to depend on llvm/lib/A directly or indirectly.</div><div>E.g. llvm-nm needs to depend on bitcode reader because it needs to handle LLVM bitcode files, however, I don't want a random cl::opt in bitcode reader to appear in llvm-nm's command line option list.</div><div><br></div><div>So I just built mlir-opt and inspected its --help output. It has exactly the problem I called out in my first message:</div><div><br></div><div><span style="color:rgb(0,0,0);white-space:pre-wrap">* We don't need to add cl::cat to every option and call</span><br></div><div><pre style="white-space:pre-wrap;color:rgb(0,0,0)">`HideUnrelatedOptions` to hide unrelated options from --help. The issue
would happen with cl::opt tools if linker garbage collection is disabled or
libLLVM-13git.so is used. (See <a href="https://reviews.llvm.org/D104363">https://reviews.llvm.org/D104363</a>)</pre><pre style="white-space:pre-wrap;color:rgb(0,0,0)"><span style="font-family:arial,sans-serif">There are many unrelated options like --amdgpu-bypass-slow-div which is from </span>llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp<br></pre></div></div>