Review feedback

Author: dcaballe

mlir/include/mlir/Dialect/Vector/IR/VectorOps.td

@@ -2485,10 +2485,9 @@ def Vector_MaskOp : Vector_Op<"mask", [
     terminator yields all results from the maskable operation to the result of
     this operation. No other values are allowed to be yielded.
 
-    An empty `vector.mask` operation is considered ill-formed but legal to
-    facilitate optimizations across the `vector.mask` operation. It is considered
-    a no-op regardless of its returned values and will be removed by the
-    canonicalizer.
+    An empty `vector.mask` operation is legal to facilitate optimizations across
+    the `vector.mask` operation. However, it is considered a no-op regardless of
+    its returned values and will be removed by the canonicalizer.
 
     The vector mask argument holds a bit for each vector lane and determines
     which vector lanes should execute the maskable operation and which ones

mlir/lib/Dialect/Vector/IR/VectorOps.cpp

@@ -6550,18 +6550,20 @@ void mlir::vector::MaskOp::print(OpAsmPrinter &p) {
 }
 
 void MaskOp::ensureTerminator(Region &region, Builder &builder, Location loc) {
-  // Create default terminator if there are no ops to mask.
+  // 1. For an empty `vector.mask`, create a default terminator.
   if (region.empty() || region.front().empty()) {
     OpTrait::SingleBlockImplicitTerminator<vector::YieldOp>::Impl<
         MaskOp>::ensureTerminator(region, builder, loc);
     return;
   }
 
-  // If  region has an explicit terminator, we don't modify it.
+  // 2. For a non-empty `vector.mask` with an explicit terminator, do nothing.
   Block &block = region.front();
   if (isa<vector::YieldOp>(block.back()))
     return;
 
+  // 3. For a non-empty `vector.mask` without an explicit terminator:
+
   // Create default terminator if the number of masked operations is not
   // one. This case will trigger a verification failure.
   if (block.getOperations().size() != 1) {
@@ -6610,9 +6612,8 @@ LogicalResult MaskOp::verify() {
                        "number of results");
 
   if (!llvm::equal(maskableOp->getResults(), terminator.getOperands()))
-    return emitOpError(
-        "expects all the results from the MaskableOpInterface to "
-        "be returned by the terminator");
+    return emitOpError("expects all the results from the MaskableOpInterface "
+                       "to match all the values returned by the terminator");
 
   if (!llvm::equal(maskableOp->getResultTypes(), getResultTypes()))
     return emitOpError(

mlir/test/Dialect/Vector/invalid.mlir

@@ -1756,12 +1756,12 @@ func.func @vector_mask_empty_passthru_no_return_type(%mask : vector<8xi1>,
 
 // -----
 
-func.func @vector_mask_non_empty_external_return(%t0: tensor<?xf32>, %idx: index,
-                                                 %m0: vector<16xi1>, %ext: vector<16xf32>) -> vector<16xf32> {
+func.func @vector_mask_non_empty_external_return(%t: tensor<?xf32>, %idx: index,
+                                                 %m: vector<16xi1>, %ext: vector<16xf32>) -> vector<16xf32> {
   %ft0 = arith.constant 0.0 : f32
-  // expected-error@+1 {{'vector.mask' op expects all the results from the MaskableOpInterface to be returned by the terminator}}
-  %0 = vector.mask %m0 {
-    %1 =vector.transfer_read %t0[%idx], %ft0 : tensor<?xf32>, vector<16xf32>
+  // expected-error@+1 {{'vector.mask' op expects all the results from the MaskableOpInterface to match all the values returned by the terminator}}
+  %0 = vector.mask %m {
+    %1 =vector.transfer_read %t[%idx], %ft0 : tensor<?xf32>, vector<16xf32>
     vector.yield %ext : vector<16xf32>
   } : vector<16xi1> -> vector<16xf32>
 
@@ -1779,12 +1779,12 @@ func.func @vector_mask_empty_passthru_empty_return_type(%mask : vector<8xi1>,
 
 // -----
 
-func.func @vector_mask_non_empty_mixed_return(%t0: tensor<?xf32>, %idx: index,
-                                              %m0: vector<16xi1>, %ext: vector<16xf32>) -> (vector<16xf32>, vector<16xf32>) {
+func.func @vector_mask_non_empty_mixed_return(%t: tensor<?xf32>, %idx: index,
+                                              %m: vector<16xi1>, %ext: vector<16xf32>) -> (vector<16xf32>, vector<16xf32>) {
   %ft0 = arith.constant 0.0 : f32
   // expected-error@+1 {{'vector.mask' op expects number of results to match maskable operation number of results}}
-  %0:2 = vector.mask %m0 {
-    %1 =vector.transfer_read %t0[%idx], %ft0 : tensor<?xf32>, vector<16xf32>
+  %0:2 = vector.mask %m {
+    %1 =vector.transfer_read %t[%idx], %ft0 : tensor<?xf32>, vector<16xf32>
     vector.yield %1, %ext : vector<16xf32>, vector<16xf32>
   } : vector<16xi1> -> (vector<16xf32>, vector<16xf32>)