Skip to content

[mlir] Fixing a regression that '-D' option of llvm-tblgen is unregistered. #91329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 8, 2024
Merged

[mlir] Fixing a regression that '-D' option of llvm-tblgen is unregistered. #91329

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c0b08da
[mlir] Fixing a regression that '-D' option of llvm-tblgen is unregis…
DanielCChen May 7, 2024
e432644
[mlir] Document -D is ignored by mlir-src-sharder.
DanielCChen May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions mlir/tools/mlir-src-sharder/mlir-src-sharder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,16 @@ int main(int argc, char **argv) {
"write-if-changed",
llvm::cl::desc("Only write to the output file if it changed"));

// `ResetCommandLineParser` at the above unregistered the "D" option
// of `llvm-tblgen`, which caused `TestOps.cpp` to fail due to
// "Unknnown command line argument '-D...`" when a macros name is
// present. The following is a workaround to re-register it again.
llvm::cl::list<std::string> MacroNames(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason the variable name starts with uppercase while he others above start with lowercase?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. This code is coming from llvm/lib/TableGen/Main.cpp where llvm-tblgen sets its cl options. It seems LLVM always use uppercase for cl option variables, but MLIR uses lowercase. I am not sure if we should keep it consistent with LLVM or keep using lowercase in MLIR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable should follow MLIR convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I will post another patch to change to lowercase macroNames.

"D",
llvm::cl::desc(
"Name of the macro to be defined -- ignored by mlir-src-sharder"),
llvm::cl::value_desc("macro name"), llvm::cl::Prefix);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be documented as ignored and present for compatibility I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review. Which document are you thinking of?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the llvm::cl::desc and llvm::cl::value_desc on the two lines above. This is what will be displayed when running --help

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Thanks. BTW, when I run mlir-src-sharder --help. It crashed with

> ./bin/mlir-src-sharder --help
USAGE: mlir-src-sharder [options] <input file>

OPTIONS:
mlir-src-sharder: llvm-project/llvm/lib/Support/CommandLine.cpp:2461: virtual void (anonymous namespace)::CategorizedHelpPrinter::printOptions((anonymous namespace)::HelpPrinter::StrOptionPairVector &, size_t): Assertion `llvm::is_contained(SortedCategories, Cat) && "Option has an unregistered category"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
Stack dump:
0.      Program arguments: ./bin/mlir-src-sharder --help
 #0 0x00007fff8c51e4fc llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/scratch/cdchen/FLANG/build/bin/../lib/libLLVMSupport.so.19.0git+0x25e4fc)
 #1 0x00007fff8c51ecf4 PrintStackTraceSignalHandler(void*) Signals.cpp:0:0
 #2 0x00007fff8c51b408 llvm::sys::RunSignalHandlers() (/scratch/cdchen/FLANG/build/bin/../lib/libLLVMSupport.so.19.0git+0x25b408)
 #3 0x00007fff8c51efdc SignalHandler(int) Signals.cpp:0:0
 #4 0x00007fff8c7304d8 (linux-vdso64.so.1+0x4d8)
 #5 0x00007fff8bd3a448 raise (/lib64/libc.so.6+0x4a448)
 #6 0x00007fff8bd14a54 abort (/lib64/libc.so.6+0x24a54)
 #7 0x00007fff8bd2dc30 __assert_fail_base (/lib64/libc.so.6+0x3dc30)
 #8 0x00007fff8bd2dcd4 __assert_fail (/lib64/libc.so.6+0x3dcd4)
 #9 0x00007fff8c417034 (anonymous namespace)::CategorizedHelpPrinter::printOptions(llvm::SmallVector<std::pair<char const*, llvm::cl::Option*>, 128u>&, unsigned long) CommandLine.cpp:0:0
#10 0x00007fff8c410108 (anonymous namespace)::HelpPrinter::printHelp() CommandLine.cpp:0:0
#11 0x00007fff8c418230 llvm::cl::opt<(anonymous namespace)::HelpPrinterWrapper, true, llvm::cl::parser<bool>>::handleOccurrence(unsigned int, llvm::StringRef, llvm::StringRef) CommandLine.cpp:0:0
#12 0x00007fff8c40a3a0 llvm::cl::Option::addOccurrence(unsigned int, llvm::StringRef, llvm::StringRef, bool) (../build/bin/../lib/libLLVMSupport.so.19.0git+0x14a3a0)
#13 0x00007fff8c414dbc CommaSeparateAndAddOccurrence(llvm::cl::Option*, unsigned int, llvm::StringRef, llvm::StringRef, bool) CommandLine.cpp:0:0
#14 0x00007fff8c402a28 ProvideOption(llvm::cl::Option*, llvm::StringRef, llvm::StringRef, int, char const* const*, int&) CommandLine.cpp:0:0
#15 0x00007fff8c407dbc llvm::cl::ParseCommandLineOptions(int, char const* const*, llvm::StringRef, llvm::raw_ostream*, char const*, bool) (..build/bin/../lib/libLLVMSupport.so.19.0git+0x147dbc)
#16 0x0000000100c928cc main (./bin/mlir-src-sharder+0x128cc)
#17 0x00007fff8bd1a96c generic_start_main.isra.0 (/lib64/libc.so.6+0x2a96c)
#18 0x00007fff8bd1ab04 __libc_start_main (/lib64/libc.so.6+0x2ab04)
Abort(coredump)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely something expected! @Mogball do you know about this?


llvm::InitLLVM y(argc, argv);
llvm::cl::ParseCommandLineOptions(argc, argv);

Expand Down