[Mlir-commits] [mlir] [mlir][gpu] Clean up repeated spaces in op syntax. NFC. (PR #89249)

[Maksim Levental]

makslevental wrote:

There's a printer ambiguity due to poor dispatch layering in [AttrOrTypeFormatGen.cpp](https://github.com/iree-org/llvm-project/blob/7359a6b7996f92e6659418d3d2e5b57c44d65e37/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp#L786-L788): consider 

```
def IREEGPU_ReorderWorkgroupsStrategyAttr :
    EnumAttr<IREEGPU_Dialect, IREEGPU_ReorderWorkgroupsStrategy, "reorder_work_groups_strategy"> {
  let assemblyFormat = "$value";
  let cppNamespace = "::mlir::iree_compiler::IREE::GPU";
}
```

which currently generates

```cpp
void ReorderWorkgroupsStrategyAttr::print(::mlir::AsmPrinter &odsPrinter) const {
  ::mlir::Builder odsBuilder(getContext());
  // shouldEmitSpace: 1
  // !lastWasPunctuation: 1
  odsPrinter << ' ';
  odsPrinter << stringifyReorderWorkgroupsStrategy(getValue());
}
```

(where I've inlined the values of `DefFormat::shouldEmitSpace` and `!DefFormat::lastWasPunctuation` at time of printer generation, i.e., during build).

This prints the following

```mlir
// IREEGPU_ReorderWorkgroupsStrategyAttr alone
#iree_gpu<reorder_work_groups_strategy Swizzle>

// IREEGPU_ReorderWorkgroupsStrategyAttr as a Parameter to IREEGPU_GPUPipelineOptionsAttr
#iree_gpu.pipeline_options<prefetch_shared_memory = true, no_reduce_shared_memory_bank_conflicts = false, reorder_workgroups_strategy =  Swizzle>
```

Note the extra space in `reorder_workgroups_strategy =  Swizzle`.

The reason this happens is because in the latter case, the printer `GPUPipelineOptionsAttr` tries to do the correct thing: 

```cpp
void GPUPipelineOptionsAttr::print(::mlir::AsmPrinter &odsPrinter) const {
  ::mlir::Builder odsBuilder(getContext());
  odsPrinter << "<";
  {
    ...
    if (!(getReorderWorkgroupsStrategy() == ReorderWorkgroupsStrategyAttr())) {
      if (!_firstPrinted) odsPrinter << ", ";
      _firstPrinted = false;
      odsPrinter << "reorder_workgroups_strategy = ";
      if (!(getReorderWorkgroupsStrategy() == ReorderWorkgroupsStrategyAttr())) {
        // shouldEmitSpace: 0
        // !lastWasPunctuation: 0
        odsPrinter.printStrippedAttrOrType(getReorderWorkgroupsStrategy());
      }
    }
  }
  odsPrinter << ">";
}
```

but `printStrippedAttrOrType` [of course has no connection to](https://github.com/iree-org/llvm-project/blob/7900daaa7ba57b5f9729bbbdb54f4e0599a45cd7/mlir/include/mlir/IR/OpImplementation.h#L176) `DefFormat::shouldEmitSpace` and `!DefFormat::lastWasPunctuation`.

I believe the logical error is in pushing the responsibility of `shouldEmitSpace` and `lastWasPunctuation` on `DefFormat`; an attribute/token/thing should be able to print itself should be agnostic to its context I think? Here is a minimal patch that produces reasonable results in this case but I have not tested beyond:

```diff
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
--- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp	(revision a758bcdbd92efb64a3482eb95d2769d74e33f5bb)
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp	(date 1729175762504)
@@ -932,7 +932,7 @@
   printer.body() << "  return ::llvm::TypeSwitch<::mlir::" << valueType
                  << ", ::llvm::LogicalResult>(def)";
   const char *const printValue = R"(    .Case<{0}>([&](auto t) {{
-      printer << {0}::getMnemonic();{1}
+      printer << {0}::getMnemonic() << ' ';{1}
       return ::mlir::success();
     })
 )";
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp
--- a/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp	(revision a758bcdbd92efb64a3482eb95d2769d74e33f5bb)
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeFormatGen.cpp	(date 1729175930071)
@@ -783,12 +783,6 @@
     os.indent();
   }
 
-  // Insert a space before the next parameter, if necessary.
-  if (shouldEmitSpace || !lastWasPunctuation)
-    os << tgfmt("$_printer << ' ';\n", &ctx);
-  shouldEmitSpace = true;
-  lastWasPunctuation = false;
-
   if (el->shouldBeQualified())
     os << tgfmt(qualifiedParameterPrinter, &ctx) << ";\n";
   else if (auto printer = param.getPrinter())
 ```

It produces the following prints (transcribing the "before"):

```mlir
// before
#iree_gpu<reorder_work_groups_strategy Swizzle>
#iree_gpu.pipeline_options<prefetch_shared_memory = true, no_reduce_shared_memory_bank_conflicts = false, reorder_workgroups_strategy =  Swizzle>
// after
#iree_gpu<reorder_work_groups_strategy Swizzle>
#iree_gpu<pipeline_options <prefetch_shared_memory = true, no_reduce_shared_memory_bank_conflicts = false, reorder_workgroups_strategy = Swizzle>>
```

Note, the reason that the aggregate attribute is printed so differently is because somewhere (I have no found where...) someone is making a decision about whether it should be `#iree_gpu.` or `#iree_gpu<` based on whether a space follows the first following token (I supposed by `peek`ing the `os` buffer........................).

If we are in agreement that this is a problem I can fully explore/investigate/fix.


https://github.com/llvm/llvm-project/pull/89249