[mlir] Change ABI breaking use of NDEBUG to LLVM_ENABLE_ABI_BREAKING_CHECKS

[zero9178]

Summary

The DataLayout class currently contains the member layoutStack which is hidden behind a preprocessor region dependant on the NDEBUG macro. Code wise this makes a lot of sense, as the layoutStack is used for extra assertions that users will want when compiling a debug build.
It however has the uncomfortable consequence of leading to a different ABI in Debug and Release builds. This I think is a bit annoying for downstream projects and others as they may want to build against a stable Release of MLIR in Release mode, but be able to debug their own project depending on MLIR.

This patch changes the related uses of NDEBUG to LLVM_ENABLE_ABI_BREAKING_CHECKS. As the macro is computed at configure time of LLVM, it may not change based on compiler settings of a downstream projects like NDEBUG would.

---

As an aside I'd like to ask if MLIR does bug fix cherry picks onto the release branches? Because if accepted I'd like this patch to be cherry picked on the release/13.x branch if that is okay.

[lattner]

This makes a lot of sense to me, I agree with your thought process, and this looks obvious. I think @ftynse is the best reviewer though.

[mehdi_amini]

This I think is a bit annoying for downstream projects and others as they may want to build against a stable Release of MLIR in Release mode, but be able to debug their own project depending on MLIR.

If I am following what you’re after, this is explicitly something prevented by LLVM: there are even some symbols that are different between debug and release builds to force a link failure (on my phone right now, can’t give you a pointer)

[lattner]

I'm not sure what you mean. LLVM generally doesn't allow ifdef'ing out chunks of data structures on NDEBUG in headers, specifically to prevent this sort of problem.

[jpienaar]

In D107227#2918602, @lattner wrote:

I'm not sure what you mean. LLVM generally doesn't allow ifdef'ing out chunks of data structures on NDEBUG in headers, specifically to prevent this sort of problem.

I believe Mehdi is referring to
https://lists.llvm.org/pipermail/llvm-dev/2016-November/107148.html ("an [LLVM] assert build isn’t ABI compatible with a release build")

[lattner]

Ok, I don't recall that patch, and it wasn't merged.

Our general policy is that public headers shouldn't change type layouts when assertions are enabled. We do allow that for "expensive checks" though.

[mehdi_amini]

In D107227#2918605, @lattner wrote:

Ok, I don't recall that patch, and it wasn't merged.

It was: https://reviews.llvm.org/D26876 ; but it's irrelevant here actually: I incorrectly brought this aspect in the discussion in this patch. Sorry for the added confusion!
So:

• An LLVM binary ABI depends on the setting of LLVM_ENABLE_ABI_BREAKING_CHECKS, unsurprisingly.
• The *default* settings if not specified for this is that it gets enabled with assertions and disabled otherwise.

Corollary: release and debug build of LLVM are incompatible with each other, unless the users building with assertions explicitly sets -DLLVM_ENABLE_ABI_BREAKING_CHECKS=OFF as well.

The use of a separate flag (LLVM_ENABLE_ABI_BREAKING_CHECKS vs NDEBUG) allows to decouple the client app NDEBUG config from LLVM NDEBUG config, the important part is to build against LLVM header configured with the same LLVM_ENABLE_ABI_BREAKING_CHECKS as the LLVM binary.
The thing that trips people is to build their app against a debug build of LLVM, and then try to swap with a release build of LLVM and things breaks.
(But that's not what's asked here....)

Our general policy is that public headers shouldn't change type layouts when assertions are enabled. We do allow that for "expensive checks" though.

Yes your right, the correct fix in this patch is to guard this in DataLayoutInterfaces.h with LLVM_ENABLE_ABI_BREAKING_CHECKS instead of NDEBUG.
@zero9178 can you update accordingly?

As an aside I'd like to ask if MLIR does bug fix cherry picks onto the release branches? Because if accepted I'd like this patch to be cherry picked on the release/13.x branch if that is okay.

We haven't been very active in trying to back port patches in release branches, but we've honored every request and this fix seems to be an obvious candidate! Thanks

[zero9178]

Address review comments:

Replace the uses of NDEBUG that are dependant on the ABI of DataLayout with the macro LLVM_ENABLE_ABI_BREAKING_CHECKS.

[mehdi_amini]

LG! Thanks.

(Need to change title/description to reflect the change)

mlir/include/mlir/Interfaces/DataLayoutInterfaces.h

public:
 
   /// Returns the preferred of the given type in the current scope.
   unsigned getTypePreferredAlignment(Type t) const;
 
 private:
   /// Combined layout spec at the given scope.
   const DataLayoutSpecInterface originalLayout;
 
-#ifndef NDEBUG
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
   /// List of enclosing layout specs.
   SmallVector<DataLayoutSpecInterface, 2> layoutStack;
 #endif
 
   /// Asserts that the cache is still valid. Expensive in debug mode. No-op in
   /// release mode.
   void checkValid() const;
 

mlir/lib/Interfaces/DataLayoutInterfaces.cpp

assert((!op || !op.getDataLayoutSpec()) &&
            "combine attributes?)");
   }
 }
 
 mlir::DataLayout::DataLayout() : DataLayout(ModuleOp()) {}
 
 mlir::DataLayout::DataLayout(DataLayoutOpInterface op)
     : originalLayout(getCombinedDataLayout(op)), scope(op) {
-#ifndef NDEBUG
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
   checkMissingLayout(originalLayout, op);
   collectParentLayouts(op, layoutStack);
 #endif
 }
 
 mlir::DataLayout::DataLayout(ModuleOp op)
     : originalLayout(getCombinedDataLayout(op)), scope(op) {
-#ifndef NDEBUG
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
   checkMissingLayout(originalLayout, op);
   collectParentLayouts(op, layoutStack);
 #endif
 }
 
 mlir::DataLayout mlir::DataLayout::closest(Operation *op) {
   // Search the closest parent either being a module operation or implementing
   // the data layout interface.
   while (op) {
     if (auto module = dyn_cast<ModuleOp>(op))
       return DataLayout(module);
     if (auto iface = dyn_cast<DataLayoutOpInterface>(op))
       return DataLayout(iface);
     op = op->getParentOp();
   }
   return DataLayout();
 }
 
 void mlir::DataLayout::checkValid() const {
-#ifndef NDEBUG
+#if LLVM_ENABLE_ABI_BREAKING_CHECKS
   SmallVector<DataLayoutSpecInterface> specs;
   collectParentLayouts(scope, specs);
   assert(specs.size() == layoutStack.size() &&
          "data layout object used, but no longer valid due to the change in "
          "number of nested layouts");
   for (auto pair : llvm::zip(specs, layoutStack)) {
     Attribute newLayout = std::get<0>(pair);
     Attribute origLayout = std::get<1>(pair);