Convert MLIR IndentedOstream to header only.

[stellaraccident]

Summary

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).

[jpienaar]

SGTM (i was thinking about moving to LLVM Support but didn't get that far yet, this is improvement).

mlir/include/mlir/Support/IndentedOstream.h

public:
   raw_indented_ostream &indent(int with) {
     os.indent(with);
     atStartOfLine = false;
     currentIndent = with;
     return *this;
   }
 
 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;
 
   /// Tracker for current indentation.
   int currentIndent = 0;
 
   /// The leading whitespace of the string being printed, if reindent is used.
   int leadingWs = 0;
 
   /// The extra prefix to be printed, if reindent is used.
   StringRef currentExtraPrefix;
 
   /// Tracks whether at start of line and so indent is required or not.
   bool atStartOfLine = true;
 
   /// The underlying 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_

mlir/lib/Support/CMakeLists.txt

 set(LLVM_OPTIONAL_SOURCES
   FileUtilities.cpp
-  IndentedOstream.cpp
   InterfaceSupport.cpp
   StorageUniquer.cpp
   Timing.cpp
   ToolUtilities.cpp
   TypeID.cpp
 )
 
 add_mlir_library(MLIRSupport
   FileUtilities.cpp
-  IndentedOstream.cpp
   InterfaceSupport.cpp
   StorageUniquer.cpp
   Timing.cpp
   ToolUtilities.cpp
   TypeID.cpp
 
   ADDITIONAL_HEADER_DIRS
   ${MLIR_MAIN_INCLUDE_DIR}/mlir/Support
 
   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
-  )

mlir/lib/Support/IndentedOstream.cpp

-//===- 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;
-  }
-}

mlir/lib/TableGen/CMakeLists.txt

 # This library is unusual, since mlir-tblgen depends on it, which is
 # built with DISABLE_LLVM_LINK_LLVM_DYLIB, this must also be built
 # with that option.  Otherwise builds with LLVM_BUILD_LLVM_DYLIB and
 # LLVM_LINK_LLVM_DYLIB fail.  (Note that even if this has no llvm
 # component dependencies, LLVM_LINK_LLVM_DYLIB tends to introduce a
 # dependence on libLLVM.so)  However, it must also be linkable against
 # 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
   AttrOrTypeDef.cpp
   Builder.cpp
   Class.cpp
   CodeGenHelpers.cpp
   Constraint.cpp
llvm_add_library(MLIRTableGen STATIC
   Successor.cpp
   Trait.cpp
   Type.cpp
 
   DISABLE_LLVM_LINK_LLVM_DYLIB
 
   ADDITIONAL_HEADER_DIRS
   ${MLIR_MAIN_INCLUDE_DIR}/mlir/TableGen
+  ${MLIR_MAIN_INCLUDE_DIR}/mlir/Support
 )
 
 mlir_check_all_link_libraries(MLIRTableGen)
 
 add_mlir_library_install(MLIRTableGen)

mlir/lib/Tools/mlir-tblgen/CMakeLists.txt

+# 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
 
   ADDITIONAL_HEADER_DIRS
   ${MLIR_MAIN_INCLUDE_DIR}/mlir/Tools/mlir-tblgen
 
   DISABLE_LLVM_LINK_LLVM_DYLIB
 

mlir/tools/mlir-tblgen/CMakeLists.txt

add_tablegen(mlir-tblgen MLIR
   PassGen.cpp
   RewriterGen.cpp
   SPIRVUtilsGen.cpp
   )
 
 set_target_properties(mlir-tblgen PROPERTIES FOLDER "Tablegenning")
 target_link_libraries(mlir-tblgen
   PRIVATE
-  MLIRSupportIndentedOstream
   MLIRTblgenLib)
 
 mlir_check_all_link_libraries(mlir-tblgen)