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

DanielCChen · 2024-05-07T14:01:18Z

PR #89664 introduced a regression that it unregistered llvm-tblgen option -D for macros. The test TestOps.cpp failed due to passing a macros to llvm-tblgen.

It caused our internal build to fail because we append -DLOCAL_NAME into LLVM_TABLEGEN_FLANGS in llvm/lib/cmake/llvm/TableGen.cmake as

list(APPEND LLVM_TABLEGEN_FLAGS "-DLOCAL_NAME")

And in ./llvm/lib/Target/PowerPC/PPC.td, we check it for some downstream code as:

...
#ifdef LOCAL_NAME
...
#endif

Now we got error message from mlir-src-sharder as

mlir-src-sharder -op-shard-index=1 -DLOCAL_NAME llvm-project/mlir/test/lib/Dialect/Test/TestOps.cpp --write-if-changed -o tools/mlir/test/lib/Dialect/Test/TestOps.1.cpp -d tools/mlir/test/lib/Dialect/Test/TestOps.1.cpp.d
mlir-src-sharder: Unknown command line argument '-DLOCAL_NAME'.  Try: 'llvm-project/build/bin/mlir-src-sharder --help'
mlir-src-sharder: Did you mean '-I'?

This PR is to fix the regression.

…tered.

llvmbot · 2024-05-07T14:01:51Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Daniel Chen (DanielCChen)

Changes

PR #89664 introduced a regression that it unregistered llvm-tblgen option -D for macros. The test TestOps.cpp failed due to passing a macros to llvm-tblgen.
It caused our internal build to fail.
This PR is to fix the regression.

Full diff: https://github.com/llvm/llvm-project/pull/91329.diff

1 Files Affected:

(modified) mlir/tools/mlir-src-sharder/mlir-src-sharder.cpp (+8)

diff --git a/mlir/tools/mlir-src-sharder/mlir-src-sharder.cpp b/mlir/tools/mlir-src-sharder/mlir-src-sharder.cpp
index dc1e2939c7d25b..c29455f00c2f07 100644
--- a/mlir/tools/mlir-src-sharder/mlir-src-sharder.cpp
+++ b/mlir/tools/mlir-src-sharder/mlir-src-sharder.cpp
@@ -62,6 +62,14 @@ 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(
+      "D", llvm::cl::desc("Name of the macro to be defined"),
+      llvm::cl::value_desc("macro name"), llvm::cl::Prefix);
+
   llvm::InitLLVM y(argc, argv);
   llvm::cl::ParseCommandLineOptions(argc, argv);

joker-eph · 2024-05-07T16:21:53Z

mlir/tools/mlir-src-sharder/mlir-src-sharder.cpp

+  // present. The following is a workaround to re-register it again.
+  llvm::cl::list<std::string> MacroNames(
+      "D", llvm::cl::desc("Name of the macro to be defined"),
+      llvm::cl::value_desc("macro name"), llvm::cl::Prefix);


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

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

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

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)

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

fhahn · 2024-05-10T09:09:30Z

mlir/tools/mlir-src-sharder/mlir-src-sharder.cpp

+  // 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(


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

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.

The variable should follow MLIR convention.

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

Similar to #91329, `mlir-pdll` is a tool used in tablegen macros that unregisters from common flags, including `-D` macros. Because a macro may be used globally, e.g. configured via `LLVM_TABLEGEN_FLAGS`, we want this tool to just ignore the macro instead of a fatal failure due to the unrecognized flag.

[mlir] Fixing a regression that '-D' option of llvm-tblgen is unregis…

c0b08da

…tered.

DanielCChen requested review from rupprecht, joker-eph and Mogball May 7, 2024 14:01

DanielCChen self-assigned this May 7, 2024

llvmbot added mlir:core MLIR Core Infrastructure mlir labels May 7, 2024

DanielCChen mentioned this pull request May 7, 2024

[mlir][ods] Add documentation on how to use sharded op definitions (NFC) #89664

Merged

DanielCChen requested a review from jeanPerier May 7, 2024 14:05

joker-eph approved these changes May 7, 2024

View reviewed changes

[mlir] Document -D is ignored by mlir-src-sharder.

e432644

DanielCChen merged commit f97f04e into llvm:main May 8, 2024

DanielCChen deleted the daniel_mlir branch May 8, 2024 13:48

fhahn reviewed May 10, 2024

View reviewed changes

rupprecht mentioned this pull request Jan 23, 2025

[mlir][PDLL] Allow (and ignore) -D tablegen macros. #124166

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

DanielCChen commented May 7, 2024 •

edited

Loading

Uh oh!

llvmbot commented May 7, 2024 •

edited

Loading

Uh oh!

joker-eph May 7, 2024

Uh oh!

DanielCChen May 7, 2024

Uh oh!

joker-eph May 7, 2024

Uh oh!

DanielCChen May 7, 2024

Uh oh!

joker-eph May 8, 2024

Uh oh!

fhahn May 10, 2024

Uh oh!

DanielCChen May 10, 2024

Uh oh!

joker-eph May 11, 2024

Uh oh!

DanielCChen May 13, 2024

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

DanielCChen commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DanielCChen commented May 7, 2024 •

edited

Loading

llvmbot commented May 7, 2024 •

edited

Loading