This is an archive of the discontinued LLVM Phabricator instance.

[OptTable] Store llvm::cl::Option into a DenseMap instead of a StringMap
Needs ReviewPublic

Authored by serge-sans-paille on Jan 19 2023, 10:52 PM.

Details

Reviewers
nikic
Summary

StringMap are useful when we need to duplicate the key's memory. That's
not the case for llvm:🆑:Option because the keys are StringLiteral.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 10:52 PM
serge-sans-paille requested review of this revision.Jan 19 2023, 10:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2023, 10:52 PM
nikic added a comment.Jan 20 2023, 1:38 AM

Makes sense to me.

From pre-merge checks:

FAILED: tools/clang/tools/clang-refactor/CMakeFiles/clang-refactor.dir/ClangRefactor.cpp.o
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/clang++ -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/clang/tools/clang-refactor -I/var/lib/buildkite-agent/builds/llvm-project/clang/tools/clang-refactor -I/var/lib/buildkite-agent/builds/llvm-project/clang/include -Itools/clang/include -Iinclude -I/var/lib/buildkite-agent/builds/llvm-project/llvm/include -gmlt -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -Wno-nested-anon-types -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/clang/tools/clang-refactor/CMakeFiles/clang-refactor.dir/ClangRefactor.cpp.o -MF tools/clang/tools/clang-refactor/CMakeFiles/clang-refactor.dir/ClangRefactor.cpp.o.d -o tools/clang/tools/clang-refactor/CMakeFiles/clang-refactor.dir/ClangRefactor.cpp.o -c /var/lib/buildkite-agent/builds/llvm-project/clang/tools/clang-refactor/ClangRefactor.cpp
In file included from /var/lib/buildkite-agent/builds/llvm-project/clang/tools/clang-refactor/ClangRefactor.cpp:16:
In file included from /var/lib/buildkite-agent/builds/llvm-project/clang/include/clang/Frontend/CommandLineSourceLoc.h:18:
/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/Support/CommandLine.h:1254:66: error: no member named 'apply' in 'llvm::StringRef'
  template <class Opt> static void opt(const Mod &M, Opt &O) { M.apply(O); }
                                                               ~ ^
/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/Support/CommandLine.h:1302:20: note: in instantiation of function template specialization 'llvm::cl::applicator<llvm::StringRef>::opt<llvm::cl::opt<std::string>>' requested here
  applicator<Mod>::opt(M, *O);
                   ^
/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/Support/CommandLine.h:1494:5: note: in instantiation of function template specialization 'llvm::cl::apply<llvm::cl::opt<std::string>, llvm::StringRef, llvm::cl::desc, llvm::cl::NumOccurrencesFlag, llvm::cl::cat, llvm::cl::sub>' requested here
    apply(this, Ms...);
    ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/unique_ptr.h:962:34: note: in instantiation of function template specialization 'llvm::cl::opt<std::basic_string<char>, false>::opt<llvm::StringRef, llvm::cl::desc, llvm::cl::NumOccurrencesFlag, llvm::cl::cat, llvm::cl::sub>' requested here
    { return unique_ptr<_Tp>(new _Tp(std::forward<_Args>(__args)...)); }
                                 ^
/var/lib/buildkite-agent/builds/llvm-project/clang/tools/clang-refactor/ClangRefactor.cpp:242:17: note: in instantiation of function template specialization 'std::make_unique<llvm::cl::opt<std::string>, llvm::StringRef, llvm::cl::desc, llvm::cl::NumOccurrencesFlag, llvm::cl::cat, llvm::cl::sub>' requested here
    return std::make_unique<cl::opt<T>>(
                ^
/var/lib/buildkite-agent/builds/llvm-project/clang/tools/clang-refactor/ClangRefactor.cpp:231:36: note: in instantiation of function template specialization '(anonymous namespace)::CommandLineRefactoringOptionCreator::create<std::basic_string<char>>' requested here
      Options.addStringOption(Opt, create<std::string>(Opt));
                                   ^
1 error generated.
llvm/lib/Support/CommandLine.cpp
593

Can probably use range based loop here and avoid naming the type altogether.

barannikov88 added inline comments.
llvm/lib/CodeGen/TargetPassConfig.cpp
213–216

From the comment near StringLiteral:

StringLiteral should *only* be used in a constexpr context, as such:
constexpr StringLiteral S("test");

Fix build + make StringLiteral initialization constexpr.

Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 6:14 AM
nikic added a comment.Jan 20 2023, 7:56 AM
FAILED: tools/mlir/test/lib/Dialect/Linalg/CMakeFiles/MLIRLinalgTestPasses.dir/TestLinalgHoisting.cpp.o
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/clang++ -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -DMLIR_CUDA_CONVERSIONS_ENABLED=1 -DMLIR_INCLUDE_TESTS -DMLIR_ROCM_CONVERSIONS_ENABLED=1 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools/mlir/test/lib/Dialect/Linalg -I/var/lib/buildkite-agent/builds/llvm-project/mlir/test/lib/Dialect/Linalg -Iinclude -I/var/lib/buildkite-agent/builds/llvm-project/llvm/include -I/var/lib/buildkite-agent/builds/llvm-project/mlir/include -Itools/mlir/include -gmlt -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -pedantic -Wno-long-long -Wc++98-compat-extra-semi -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wstring-conversion -Wmisleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Werror=mismatched-tags -O3 -DNDEBUG  -fno-exceptions -fno-rtti -UNDEBUG -std=c++17 -MD -MT tools/mlir/test/lib/Dialect/Linalg/CMakeFiles/MLIRLinalgTestPasses.dir/TestLinalgHoisting.cpp.o -MF tools/mlir/test/lib/Dialect/Linalg/CMakeFiles/MLIRLinalgTestPasses.dir/TestLinalgHoisting.cpp.o.d -o tools/mlir/test/lib/Dialect/Linalg/CMakeFiles/MLIRLinalgTestPasses.dir/TestLinalgHoisting.cpp.o -c /var/lib/buildkite-agent/builds/llvm-project/mlir/test/lib/Dialect/Linalg/TestLinalgHoisting.cpp
In file included from /var/lib/buildkite-agent/builds/llvm-project/mlir/test/lib/Dialect/Linalg/TestLinalgHoisting.cpp:17:
In file included from /var/lib/buildkite-agent/builds/llvm-project/mlir/include/mlir/Pass/Pass.h:13:
In file included from /var/lib/buildkite-agent/builds/llvm-project/mlir/include/mlir/Pass/PassRegistry.h:17:
In file included from /var/lib/buildkite-agent/builds/llvm-project/mlir/include/mlir/Pass/PassOptions.h:21:
/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/Support/CommandLine.h:1254:66: error: no member named 'apply' in 'llvm::StringRef'
  template <class Opt> static void opt(const Mod &M, Opt &O) { M.apply(O); }
                                                               ~ ^
/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/Support/CommandLine.h:1302:20: note: in instantiation of function template specialization 'llvm::cl::applicator<llvm::StringRef>::opt<llvm::cl::opt<bool>>' requested here
  applicator<Mod>::opt(M, *O);
                   ^
/var/lib/buildkite-agent/builds/llvm-project/llvm/include/llvm/Support/CommandLine.h:1494:5: note: in instantiation of function template specialization 'llvm::cl::apply<llvm::cl::opt<bool>, llvm::StringRef, llvm::cl::sub, llvm::cl::desc, llvm::cl::initializer<bool>>' requested here
    apply(this, Ms...);
    ^
/var/lib/buildkite-agent/builds/llvm-project/mlir/include/mlir/Pass/PassOptions.h:164:11: note: in instantiation of function template specialization 'llvm::cl::opt<bool, false>::opt<llvm::StringRef, llvm::cl::sub, llvm::cl::desc, llvm::cl::initializer<bool>>' requested here
        : llvm::cl::opt<DataType, /*ExternalStorage=*/false, OptionParser>(
          ^
/var/lib/buildkite-agent/builds/llvm-project/mlir/include/mlir/Pass/Pass.h:98:11: note: in instantiation of function template specialization 'mlir::detail::PassOptions::Option<bool, llvm::cl::parser<bool>>::Option<llvm::cl::desc, llvm::cl::initializer<bool>>' requested here
        : detail::PassOptions::Option<DataType, OptionParser>(
          ^
/var/lib/buildkite-agent/builds/llvm-project/mlir/test/lib/Dialect/Linalg/TestLinalgHoisting.cpp:39:43: note: in instantiation of function template specialization 'mlir::Pass::Option<bool, llvm::cl::parser<bool>>::Option<llvm::cl::desc, llvm::cl::initializer<bool>>' requested here
  Option<bool> testHoistRedundantTransfers{
                                          ^
1 error generated.

@nikic : I thought I fixed that with e8a163dc03e6913360beb305620104ba129c081c ... is it included in your build?

@nikic : I thought I fixed that with e8a163dc03e6913360beb305620104ba129c081c ... is it included in your build?

Oh yeah, I missed that fix. Sorry for the noise.

@nikic : I thought I fixed that with e8a163dc03e6913360beb305620104ba129c081c ... is it included in your build?

Oh yeah, I missed that fix. Sorry for the noise.

^^! Answered in the wrong thread, so I dupliaded that message to https://reviews.llvm.org/D142026 where it belongs :-)

Do not enfore StringLiteral parameter. This is too invasive on the code base.