remove CB operands from ttir.generic #2376

vroubtsovTT · 2025-03-05T19:44:38Z

Ticket

Closes #1969

Problem description

We are moving lowering of operands to CB slots to a lower section of the d2m pipeline and hence decided to remove CB operand-related arguments from ttir.generic op.

What's changed

There are two main changes, the first is a generic tablegen cleanup action and is somewhat independent of the second:

a new getDpsOutputs() helper method in ttmlir/Dialect/TTIR/IR/Utils.h will auto-detect whether a concrete OP has single or variadic outputs; using it in extraClassDeclaration of selected base tablegen classes allows multiple such trivial/repeated declarations to be removed from a number of TTIROps defs.
ttir.generic has lost CB operands/mapping attributes; the enqueue program rewriter in TTIRToTTMetal.cpp pass that depends on that logic has been removed for now

Checklist

multiple existing tests had their operand_cb_mapping attributes removed and operandSegmentSizes adjusted accordingly

nsmithtt

Looks great! Style nit regarding headers

nsmithtt · 2025-03-05T20:42:46Z

include/ttmlir/Dialect/TTIR/IR/TTIROps.h

+#include <mlir/Interfaces/ControlFlowInterfaces.h>
+#include <mlir/Interfaces/DestinationStyleOpInterface.h>
+#include <mlir/Interfaces/InferTypeOpInterface.h>
+#include <mlir/Interfaces/SideEffectInterfaces.h>


Using the chevrons here goes against our style guide (inherited from llvm).

See https://docs.tenstorrent.com/tt-mlir/coding-guidelines.html#includes

I've always used <> for all includes that are external to the project's source tree and thus need an include path provided to the compiler. Also have done this in several prev PRs...

Changed in the last commit.

nsmithtt · 2025-03-05T20:44:20Z

include/ttmlir/Dialect/TTIR/IR/Utils.h

+// clang-format on
+template <typename Op>
+mlir::MutableOperandRange getDpsOutputs(Op *op) {
+  return impl::getDpsOutputs<Op>::evaluate(op);


the usual type deduction technique

nsmithtt · 2025-03-05T20:44:53Z

include/ttmlir/Utils.h

+#include <mlir/CAPI/IR.h>
+#include <mlir/Dialect/Tensor/IR/Tensor.h>
+#include <mlir/IR/AffineMap.h>
+#include <mlir/IR/BuiltinAttributes.h>


Fixed in the latest commit

nsmithtt · 2025-03-05T20:47:18Z

lib/Conversion/TTIRToTTMetal/TTIRToTTMetal.cpp

-  }
-};
-} // namespace
-


1300 lines removed, you must have fixed at least 10 bugs in one shot.

the best c++ code is code that doesn't exist

azecevicTT

Nice!

azecevicTT · 2025-03-05T21:13:09Z

include/ttmlir/Dialect/TTIR/IR/Utils.h

+#ifndef TTMLIR_DIALECT_TTIR_IR_UTILS_H
+#define TTMLIR_DIALECT_TTIR_IR_UTILS_H
+
+#include <mlir/IR/ValueRange.h>


Nit: <> here as well.

fixed here as well

azecevicTT · 2025-03-05T21:15:44Z

include/ttmlir/Dialect/TTIR/IR/Utils.h

+
+template <typename Op>
+inline constexpr bool has_variadic_outputs<
+    Op, std::void_t<decltype(std::declval<Op>().getOutputsMutable())>> = true;


Nit: I find convention from STL with _v and _t handy when working with templates, though this is localized so it's okay either way.

I agree and I started with _v but then it seemed to me that llvm sources don't use such suffixes, so I changed. I've checked again and maybe their usage is mixed. I've gone back to _v which is my personal preference as well.

azecevicTT · 2025-03-05T21:17:24Z

include/ttmlir/Dialect/TTIR/IR/Utils.h

+  static mlir::MutableOperandRange evaluate(Op *op) {
+    return op->getOutputsMutable();
+  }
+};


What's your opinion on this vs if constexpr? I find the latter more handy for cases like this one.

if constexpr would work here as well, as well a c++20 concept (when we can use c++20).

I think some intermediate version I had needed template specialization to instantiate single/variadic branches separately. But that's not needed in the final version so I've changed to if constexpr as more compact.

### Ticket Closes #1969 ### Problem description We are moving lowering of operands to CB slots to a lower section of the d2m pipeline and hence decided to remove CB operand-related arguments from ttir.generic op. ### What's changed There are two main changes, the first is a generic tablegen cleanup action and is somewhat independent of the second: 1. a new `getDpsOutputs()` helper method in `ttmlir/Dialect/TTIR/IR/Utils.h` will auto-detect whether a concrete OP has single or variadic outputs; using it in `extraClassDeclaration` of selected base tablegen `class`es allows multiple such trivial/repeated declarations to be removed from a number of TTIROps `def`s. 2. ttir.generic has lost CB operands/mapping attributes; the `enqueue program` rewriter in `TTIRToTTMetal.cpp` pass that depends on that logic has been removed for now ### Checklist - [x] multiple existing tests had their operand_cb_mapping attributes removed and `operandSegmentSizes` adjusted accordingly

vroubtsovTT added 3 commits March 5, 2025 16:04

simplify DPS inheritance using utils::getDpsOutputs()

36a886a

rm CB operands from ttir.generic

ffdd627

move getDpsOutputs() helper into tt::ttir and a dialect-specific Utils.h

46612ac

vroubtsovTT requested review from sdjordjevicTT, svuckovicTT, mtopalovicTT, mrakitaTT, jserbedzijaTT, azecevicTT and nsmithtt as code owners March 5, 2025 19:44

add newline at EOF

a169fc7

nsmithtt reviewed Mar 5, 2025

View reviewed changes

azecevicTT approved these changes Mar 5, 2025

View reviewed changes

vroubtsovTT added 2 commits March 5, 2025 23:53

edits to address PR feedback

f14ba54

final simplification

513445e

nsmithtt approved these changes Mar 6, 2025

View reviewed changes

vroubtsovTT merged commit 5e99f0f into main Mar 6, 2025
33 checks passed

vroubtsovTT deleted the vroubtsov/update_ttir.generic_for_d2m_v1 branch March 6, 2025 02:51

vroubtsovTT changed the title ~~remote CB operands from ttir.generic~~ remove CB operands from ttir.generic Mar 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

remove CB operands from ttir.generic #2376

remove CB operands from ttir.generic #2376

vroubtsovTT commented Mar 5, 2025 •

edited

Loading

nsmithtt left a comment

nsmithtt Mar 5, 2025

vroubtsovTT Mar 5, 2025 •

edited

Loading

nsmithtt Mar 5, 2025

vroubtsovTT Mar 5, 2025

nsmithtt Mar 5, 2025

vroubtsovTT Mar 5, 2025

nsmithtt Mar 5, 2025

vroubtsovTT Mar 5, 2025

azecevicTT left a comment

azecevicTT Mar 5, 2025

vroubtsovTT Mar 5, 2025

azecevicTT Mar 5, 2025

vroubtsovTT Mar 5, 2025

azecevicTT Mar 5, 2025

vroubtsovTT Mar 6, 2025

remove CB operands from ttir.generic #2376

remove CB operands from ttir.generic #2376

Conversation

vroubtsovTT commented Mar 5, 2025 • edited Loading

Ticket

Problem description

What's changed

Checklist

nsmithtt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vroubtsovTT Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azecevicTT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vroubtsovTT commented Mar 5, 2025 •

edited

Loading

vroubtsovTT Mar 5, 2025 •

edited

Loading