[mlir][vector][nfc] Add clarification on "dim-1" bcast

[banach-space]

Adds a small note to VectorOps.td on what "dim-1" broadcast is. Also
updates comments to consistently use quotes, i.e.

• "dim-1" broadcasting instead of dim-1 broadcasting.

This way it is clear that we are referring to "stretching" one of the
trailing dims rather than e.g. broadcasting a dim at idx 1.

[llvmbot]

@llvm/pr-subscribers-mlir-vector

@llvm/pr-subscribers-mlir

Author: Andrzej Warzyński (banach-space)

Changes

Adds a small note to VectorOps.td on what "dim-1" broadcast is. Also
updates comments to consistently use quotes, i.e.

• "dim-1" broadcasting instead of dim-1 broadcasting.

This way it is clear that we are referring to "stretching" one of the
trailing dims rather than e.g. broadcasting a dim at idx 1.

---

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

2 Files Affected:

• (modified) mlir/include/mlir/Dialect/Vector/IR/VectorOps.td (+4-3)
• (modified) mlir/lib/Dialect/Vector/IR/VectorOps.cpp (+4-4)

diff --git a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
index 835c006356342e..c821e7b1527b4d 100644
--- a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
+++ b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
@@ -373,8 +373,9 @@ def Vector_BroadcastOp :
 
     The source operand is duplicated over all the missing leading dimensions
     and stretched over the trailing dimensions where the source has a non-equal
-    dimension of 1. These rules imply that any scalar broadcast (k=0) to any
-    shaped vector with the same element type is always legal.
+    dimension of 1 (stretching a trailing dimension is also referred to as
+    "dim-1" broadcasting). These rules imply that any scalar broadcast (k=0) to
+    any shaped vector with the same element type is always legal.
 
     Example:
 
@@ -396,7 +397,7 @@ def Vector_BroadcastOp :
 
     /// Broadcast `value` to a vector of `dstShape`, knowing that exactly the
     /// `broadcastedDims` dimensions in the dstShape are broadcasted.
-    /// This requires (and asserts) that the broadcast is free of dim-1
+    /// This requires (and asserts) that the broadcast is free of "dim-1"
     /// broadcasting.
     /// Since vector.broadcast only allows expanding leading dimensions, an extra
     /// vector.transpose may be inserted to make the broadcast possible.
diff --git a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
index 93f89eda2da5a6..01c7a0112f06f7 100644
--- a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
+++ b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
@@ -2355,7 +2355,7 @@ computeBroadcastedUnitDims(ArrayRef<int64_t> srcShape,
   for (auto [s1, s2] :
        llvm::zip_equal(srcShape, dstShape.drop_front(rankDiff))) {
     if (s1 != s2) {
-      assert(s1 == 1 && "expected dim-1 broadcasting");
+      assert(s1 == 1 && "expected \"dim-1\" broadcasting");
       res.insert(dstDim);
     }
     ++dstDim;
@@ -2374,7 +2374,7 @@ llvm::SetVector<int64_t> BroadcastOp::computeBroadcastedUnitDims() {
 
 /// Broadcast `value` to a vector of `dstShape`, knowing that exactly the
 /// `broadcastedDims` dimensions in the dstShape are broadcasted.
-/// This requires (and asserts) that the broadcast is free of dim-1
+/// This requires (and asserts) that the broadcast is free of "dim-1"
 /// broadcasting.
 /// Since vector.broadcast only allows expanding leading dimensions, an extra
 /// vector.transpose may be inserted to make the broadcast possible.
@@ -2460,10 +2460,10 @@ Value BroadcastOp::createOrFoldBroadcastOp(
   // 3.c. Append the srcShape.
   llvm::append_range(broadcastShape, srcVectorType.getShape());
 
-  // Ensure there are no dim-1 broadcasts.
+  // Ensure there are no "dim-1" broadcasts.
   assert(::computeBroadcastedUnitDims(srcVectorType.getShape(), broadcastShape)
              .empty() &&
-         "unexpected dim-1 broadcast");
+         "unexpected \"dim-1\" broadcast");
 
   VectorType broadcastType = VectorType::get(broadcastShape, elementType);
   assert(vector::isBroadcastableTo(value.getType(), broadcastType) ==

[Groverkss]

LGTM

Although, when I read the documentation, it makes me wonder, do we actually use this? The scalar broadcasting makes sense regardless of if we allow this "dim-1" broadcasting or not, because scalars are essentially 0-D vectors. Nothing blocking on this PR though, we can discuss that elsewhere.

[nicolasvasilache]

I see dim-1 and I immediately think "implicit" broadcasting..
Just want to be sure we have not introduced any form of "implicit" broadcasting?
This PR is innocuous but can we confirm this is still banned in Linalg and Vector?

[banach-space]

I see dim-1 and I immediately think "implicit" broadcasting.

Ha, what's "implicit" broadcasting ? :)

I sense that there's many different things "hiding" behind the term "broadcasting" and, without strict definitions, we tend to interpret them differently. Here's e.g. isBroadcasted in Linalg that I find a bit .. misleading.

Anyway, this is why I wanted to clarify "dim-1" broadcasting and hence this PR.

Just want to be sure we have not introduced any form of "implicit" broadcasting?

I am not aware of any, but you will obviously be aware of recent changes to Linalg and the fact that we are allowing "broadcasts" in affine maps, see e.g. linalg.contract.

As for other cases, I'd normally look for tests in "invalid.mlir", but not sure what to look for 😅 (i.e., what would qualify as "implicit"?)