[Mlir-commits] [mlir] b9a907d - Convert MLIR IndentedOstream to header only.
Stella Laurenzo
llvmlistbot at llvm.org
Tue Jun 20 19:19:10 PDT 2023
Author: Stella Laurenzo
Date: 2023-06-20T19:19:01-07:00
New Revision: b9a907d1336f09b6ff79a69ebd86a3a946ad4073
URL: https://github.com/llvm/llvm-project/commit/b9a907d1336f09b6ff79a69ebd86a3a946ad4073
DIFF: https://github.com/llvm/llvm-project/commit/b9a907d1336f09b6ff79a69ebd86a3a946ad4073.diff
LOG: Convert MLIR IndentedOstream to header only.
This class has been causing me no end of grief for a long time, and the way it is used by mlir-tblgen is technically an ODR violation in certain situations.
Due to the way that the build is layered, it is important that the MLIR tablegen libraries only depend on the LLVM tablegen libraries, not on anything else (like MLIRSupport). It has to be this way because these libraries/binaries are special and must pre-exist the full shared libraries. Therefore, the dependency chain must be clean (and static).
At some point, someone pulled out a separate build target for just IndendedOstream in an attempt to satisfy the constraint. But because it is weird in different ways, this target was never installed properly as part of distributions, etc -- this causes problems for downstreams seeking to build a tblggen binary that doesn't itself have ODR/shared library problems.
I was attempting to fix the distribution stuff but just opted to collapse this into a header-only library and not try to solve this with build layering. I think this is the safest and the least bad thing for such a dep. This also makes for a clean comment that actually explains the constraint (which I was having trouble verbalizing with the weird subset dependency).
Differential Revision: https://reviews.llvm.org/D153393
Added:
Modified:
mlir/include/mlir/Support/IndentedOstream.h
mlir/lib/Support/CMakeLists.txt
mlir/lib/TableGen/CMakeLists.txt
mlir/lib/Tools/mlir-tblgen/CMakeLists.txt
mlir/tools/mlir-tblgen/CMakeLists.txt
Removed:
mlir/lib/Support/IndentedOstream.cpp
################################################################################
diff --git a/mlir/include/mlir/Support/IndentedOstream.h b/mlir/include/mlir/Support/IndentedOstream.h
index 3d19a1e1ede37..5961e46c64c72 100644
--- a/mlir/include/mlir/Support/IndentedOstream.h
+++ b/mlir/include/mlir/Support/IndentedOstream.h
@@ -85,11 +85,11 @@ class raw_indented_ostream : public raw_ostream {
}
private:
- void write_impl(const char *ptr, size_t size) override;
+ void write_impl(const char *ptr, size_t size) final;
/// Return the current position within the stream, not counting the bytes
/// currently in the buffer.
- uint64_t current_pos() const override { return os.tell(); }
+ uint64_t current_pos() const final { return os.tell(); }
/// Constant indent added/removed.
static constexpr int indentSize = 2;
@@ -110,5 +110,70 @@ class raw_indented_ostream : public raw_ostream {
raw_ostream &os;
};
+inline raw_indented_ostream &
+mlir::raw_indented_ostream::printReindented(StringRef str,
+ StringRef extraPrefix) {
+ StringRef output = str;
+ // Skip empty lines.
+ while (!output.empty()) {
+ auto split = output.split('\n');
+ size_t indent = split.first.find_first_not_of(" \t");
+ if (indent != StringRef::npos) {
+ // Set an initial value.
+ leadingWs = indent;
+ break;
+ }
+ output = split.second;
+ }
+ // Determine the maximum indent.
+ StringRef remaining = output;
+ while (!remaining.empty()) {
+ auto split = remaining.split('\n');
+ size_t indent = split.first.find_first_not_of(" \t");
+ if (indent != StringRef::npos)
+ leadingWs = std::min(leadingWs, static_cast<int>(indent));
+ remaining = split.second;
+ }
+ // Print, skipping the empty lines.
+ std::swap(currentExtraPrefix, extraPrefix);
+ *this << output;
+ std::swap(currentExtraPrefix, extraPrefix);
+ leadingWs = 0;
+ return *this;
+}
+
+inline void mlir::raw_indented_ostream::write_impl(const char *ptr,
+ size_t size) {
+ StringRef str(ptr, size);
+ // Print out indented.
+ auto print = [this](StringRef str) {
+ if (atStartOfLine)
+ os.indent(currentIndent) << currentExtraPrefix << str.substr(leadingWs);
+ else
+ os << str.substr(leadingWs);
+ };
+
+ while (!str.empty()) {
+ size_t idx = str.find('\n');
+ if (idx == StringRef::npos) {
+ if (!str.substr(leadingWs).empty()) {
+ print(str);
+ atStartOfLine = false;
+ }
+ break;
+ }
+
+ auto split =
+ std::make_pair(str.slice(0, idx), str.slice(idx + 1, StringRef::npos));
+ // Print empty new line without spaces if line only has spaces and no extra
+ // prefix is requested.
+ if (!split.first.ltrim().empty() || !currentExtraPrefix.empty())
+ print(split.first);
+ os << '\n';
+ atStartOfLine = true;
+ str = split.second;
+ }
+}
+
} // namespace mlir
#endif // MLIR_SUPPORT_INDENTEDOSTREAM_H_
diff --git a/mlir/lib/Support/CMakeLists.txt b/mlir/lib/Support/CMakeLists.txt
index 0dbff287cc86b..5288441ca13ba 100644
--- a/mlir/lib/Support/CMakeLists.txt
+++ b/mlir/lib/Support/CMakeLists.txt
@@ -1,6 +1,5 @@
set(LLVM_OPTIONAL_SOURCES
FileUtilities.cpp
- IndentedOstream.cpp
InterfaceSupport.cpp
StorageUniquer.cpp
Timing.cpp
@@ -10,7 +9,6 @@ set(LLVM_OPTIONAL_SOURCES
add_mlir_library(MLIRSupport
FileUtilities.cpp
- IndentedOstream.cpp
InterfaceSupport.cpp
StorageUniquer.cpp
Timing.cpp
@@ -22,14 +20,3 @@ add_mlir_library(MLIRSupport
LINK_LIBS PUBLIC
${LLVM_PTHREAD_LIB})
-
-# This doesn't use add_mlir_library as it is used in mlir-tblgen and else
-# mlir-tblgen ends up depending on mlir-generic-headers.
-add_llvm_library(MLIRSupportIndentedOstream
- IndentedOstream.cpp
-
- DISABLE_LLVM_LINK_LLVM_DYLIB
-
- LINK_COMPONENTS
- Support
- )
diff --git a/mlir/lib/Support/IndentedOstream.cpp b/mlir/lib/Support/IndentedOstream.cpp
deleted file mode 100644
index 4e2cad11712f8..0000000000000
--- a/mlir/lib/Support/IndentedOstream.cpp
+++ /dev/null
@@ -1,80 +0,0 @@
-//===- IndentedOstream.cpp - raw ostream wrapper to indent ----------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// raw_ostream subclass that keeps track of indentation for textual output
-// where indentation helps readability.
-//
-//===----------------------------------------------------------------------===//
-
-#include "mlir/Support/IndentedOstream.h"
-
-using namespace mlir;
-
-raw_indented_ostream &
-mlir::raw_indented_ostream::printReindented(StringRef str,
- StringRef extraPrefix) {
- StringRef output = str;
- // Skip empty lines.
- while (!output.empty()) {
- auto split = output.split('\n');
- size_t indent = split.first.find_first_not_of(" \t");
- if (indent != StringRef::npos) {
- // Set an initial value.
- leadingWs = indent;
- break;
- }
- output = split.second;
- }
- // Determine the maximum indent.
- StringRef remaining = output;
- while (!remaining.empty()) {
- auto split = remaining.split('\n');
- size_t indent = split.first.find_first_not_of(" \t");
- if (indent != StringRef::npos)
- leadingWs = std::min(leadingWs, static_cast<int>(indent));
- remaining = split.second;
- }
- // Print, skipping the empty lines.
- std::swap(currentExtraPrefix, extraPrefix);
- *this << output;
- std::swap(currentExtraPrefix, extraPrefix);
- leadingWs = 0;
- return *this;
-}
-
-void mlir::raw_indented_ostream::write_impl(const char *ptr, size_t size) {
- StringRef str(ptr, size);
- // Print out indented.
- auto print = [this](StringRef str) {
- if (atStartOfLine)
- os.indent(currentIndent) << currentExtraPrefix << str.substr(leadingWs);
- else
- os << str.substr(leadingWs);
- };
-
- while (!str.empty()) {
- size_t idx = str.find('\n');
- if (idx == StringRef::npos) {
- if (!str.substr(leadingWs).empty()) {
- print(str);
- atStartOfLine = false;
- }
- break;
- }
-
- auto split =
- std::make_pair(str.slice(0, idx), str.slice(idx + 1, StringRef::npos));
- // Print empty new line without spaces if line only has spaces and no extra
- // prefix is requested.
- if (!split.first.ltrim().empty() || !currentExtraPrefix.empty())
- print(split.first);
- os << '\n';
- atStartOfLine = true;
- str = split.second;
- }
-}
diff --git a/mlir/lib/TableGen/CMakeLists.txt b/mlir/lib/TableGen/CMakeLists.txt
index 2736d95cc9742..61e14feb6dc12 100644
--- a/mlir/lib/TableGen/CMakeLists.txt
+++ b/mlir/lib/TableGen/CMakeLists.txt
@@ -7,7 +7,10 @@
# libMLIR.so in some contexts (see unittests/Tablegen, for instance, which
# has a dependence on MLIRIR, which must depend on libLLVM.so). This works
# in this special case because this library is static.
-
+#
+# In order for this arrangement to be sound, it must not depend on libraries
+# that may be transitively included in libMLIR.so or libLLVM.so. Specifically,
+# this means that MLIRSupport (outside of header-only access) cannot be used.
llvm_add_library(MLIRTableGen STATIC
Argument.cpp
Attribute.cpp
@@ -35,6 +38,7 @@ llvm_add_library(MLIRTableGen STATIC
ADDITIONAL_HEADER_DIRS
${MLIR_MAIN_INCLUDE_DIR}/mlir/TableGen
+ ${MLIR_MAIN_INCLUDE_DIR}/mlir/Support
)
mlir_check_all_link_libraries(MLIRTableGen)
diff --git a/mlir/lib/Tools/mlir-tblgen/CMakeLists.txt b/mlir/lib/Tools/mlir-tblgen/CMakeLists.txt
index d1a83cf9fb105..ed0033ff6d7cb 100644
--- a/mlir/lib/Tools/mlir-tblgen/CMakeLists.txt
+++ b/mlir/lib/Tools/mlir-tblgen/CMakeLists.txt
@@ -1,3 +1,5 @@
+# See notes in lib/TableGen/CMakeLists.txt regarding why this library is
+# declared as it is. The same dependency rules apply.
llvm_add_library(MLIRTblgenLib STATIC
MlirTblgenMain.cpp
diff --git a/mlir/tools/mlir-tblgen/CMakeLists.txt b/mlir/tools/mlir-tblgen/CMakeLists.txt
index 0d9ece2f061e7..ce16899fd4dd2 100644
--- a/mlir/tools/mlir-tblgen/CMakeLists.txt
+++ b/mlir/tools/mlir-tblgen/CMakeLists.txt
@@ -35,7 +35,6 @@ add_tablegen(mlir-tblgen MLIR
set_target_properties(mlir-tblgen PROPERTIES FOLDER "Tablegenning")
target_link_libraries(mlir-tblgen
PRIVATE
- MLIRSupportIndentedOstream
MLIRTblgenLib)
mlir_check_all_link_libraries(mlir-tblgen)
More information about the Mlir-commits
mailing list