[mlir] Revert to old fold logic in IR::Dialect::add{Types, Attributes}()

[andrey-golubev]

Fold expressions on Clang are limited to 256 elements. This causes compilation errors in cases when the amount of elements added exceeds this limit. Side-step the issue by restoring the original trick that would use the std::initializer_list. For the record, in our downstream Clang 16 gives:

mlir/include/mlir/IR/Dialect.h:269:23: fatal error: instantiating fold expression with 688 arguments exceeded expression nesting limit of 256
(addType(), ...);

Partially reverts 26d811b.

[llvmbot]

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Andrei Golubev (andrey-golubev)

Changes

Fold expressions on Clang are limited to 256 elements. This causes compilation errors in cases when the amount of elements added exceeds this limit. Side-step the issue by restoring the original trick that would use the std::initializer_list. For instance, in our downstream Clang 16 gives:

mlir/include/mlir/IR/Dialect.h:269:23: fatal error: instantiating fold expression with 688 arguments exceeded expression nesting limit of 256
(addType<Args>(), ...);

Partially reverts 26d811b.

---

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

1 Files Affected:

• (modified) mlir/include/mlir/IR/Dialect.h (+5-1)

diff --git a/mlir/include/mlir/IR/Dialect.h b/mlir/include/mlir/IR/Dialect.h
index 45f29f37dd3b97c..9fe1e1c89b228cb 100644
--- a/mlir/include/mlir/IR/Dialect.h
+++ b/mlir/include/mlir/IR/Dialect.h
@@ -281,7 +281,11 @@ class Dialect {
   /// Register a set of type classes with this dialect.
   template <typename... Args>
   void addTypes() {
-    (addType<Args>(), ...);
+    // This initializer_list argument pack expansion is essentially equal to
+    // using a fold expression with a comma operator. Clang however, refuses
+    // to compile a fold expression with a depth of more than 256 by default.
+    // There seem to be no such limitations for initializer_list.
+    (void)std::initializer_list<int>{0, (addType<Args>(), 0)...};
   }
 
   /// Register a type instance with this dialect.

[zero9178]

LGTM

[andrey-golubev]

@zero9178 I guess you had similar experience with this in addOperations().

CC @joker-eph

btw, given this problem is likely to stay with us for a while, should I also change addAttributes() / addInterfaces()? Chances are, someone would hit it eventually ...

[zero9178]

@zero9178 I guess you had similar experience with this in addOperations().

CC @joker-eph

btw, given this problem is likely to stay with us for a while, should I also change addAttributes() / addInterfaces()? Chances are, someone would hit it eventually ...

Personally I think it'd make sense to change it for addAttributes as well as it is not unlikely to reach similar sizes as types and is also called by TableGen generated code. I think addInterfaces is only ever called by users and is rather unlikely to hit the 256 types limit but I might be misremembering.

[andrey-golubev]

btw, given this problem is likely to stay with us for a while, should I also change addAttributes() / addInterfaces()? Chances are, someone would hit it eventually ...

Personally I think it'd make sense to change it for addAttributes as well as it is not unlikely to reach similar sizes as types and is also called by TableGen generated code. I think addInterfaces is only ever called by users and is rather unlikely to hit the 256 types limit but I might be misremembering.

Quite. My gut feeling tells the same. Guessing if addAttributes is called by TableGen so does the addInterfaces? But given that interfaces, well, abstract away common functionality, the amount of them should be an order smaller.

[andrey-golubev]

Restored old code for addAttributes() as well (force-pushed)

[zero9178]

Still LGTM

[joker-eph]

LG, but what's the tradeoff? Is this code less efficient somehow in any way?

[andrey-golubev]

LG, but what's the tradeoff? Is this code less efficient somehow in any way?

Not that I know of. I guess it got replaced with fold expressions since this is what such cases should really be (and they weren't fold expressions since it was pre-C++17). Unfortunately, turns out there are (strange) limitations for fold expressions (perhaps someone used a fixed-size array in Clang to parse these so now one gets the error mentioned in the commit msg by default).

Thinking about it, the "restored" version might be slower to compile: there's a type lookup (std::initializer_list) plus the AST is bigger (comma operator between expressions, etc.) but I'm not sure whether this matters significantly.

[andrey-golubev]

ping. can one of you press the merge button?