[mlir] Remove extra whitespace during printout in prop-dict (#145695)

[weirdsmiley]

Using character representation and removal of extra whitespace keeps printout consistent across cases.

[llvmbot]

@llvm/pr-subscribers-mlir-core

Author: Manas (weirdsmiley)

Changes

Using character representation and removal of extra whitespace keeps printout consistent across cases.

---

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

1 Files Affected:

• (modified) mlir/lib/IR/AsmPrinter.cpp (+1-1)

diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp
index c7cc6a02ad208..d9bd6444dda79 100644
--- a/mlir/lib/IR/AsmPrinter.cpp
+++ b/mlir/lib/IR/AsmPrinter.cpp
@@ -2848,7 +2848,7 @@ void AsmPrinter::Impl::printOptionalAttrDict(ArrayRef<NamedAttribute> attrs,
       os << " attributes";
 
     // Otherwise, print them all out in braces.
-    os << " {";
+    os << '{';
     interleaveComma(filteredAttrs,
                     [&](NamedAttribute attr) { printNamedAttribute(attr); });
     os << '}';

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Manas (weirdsmiley)

Changes

Using character representation and removal of extra whitespace keeps printout consistent across cases.

---

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

1 Files Affected:

• (modified) mlir/lib/IR/AsmPrinter.cpp (+1-1)

diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp
index c7cc6a02ad208..d9bd6444dda79 100644
--- a/mlir/lib/IR/AsmPrinter.cpp
+++ b/mlir/lib/IR/AsmPrinter.cpp
@@ -2848,7 +2848,7 @@ void AsmPrinter::Impl::printOptionalAttrDict(ArrayRef<NamedAttribute> attrs,
       os << " attributes";
 
     // Otherwise, print them all out in braces.
-    os << " {";
+    os << '{';
     interleaveComma(filteredAttrs,
                     [&](NamedAttribute attr) { printNamedAttribute(attr); });
     os << '}';

[weirdsmiley]

I notice this fails CI. It wouldn't be a correct fix, because it removes whitespace from all cases which isn't ideal. I will send a v2.

[weirdsmiley]

@River707 a question about the formatting: the issue only talks about keeping things consistent. So do we want to remove the whitespace from single attribute case or put extra whitespace in all cases?

Visually, that would be: we can either make prop-dict look like <{ in all cases (one or more attributes) or to make it < { (add a whitespace) in all cases?

[joker-eph]

Why do we have this extra whitespace in the first place?
Was this actually needed in some cases?

[weirdsmiley]

@joker-eph yes there are cases like this, outside of <> where removing whitespace would lead to something like the following.

expected:
; CHECK: module attributes {

if whitespace removed:
; CHECK: module attributes{

There are more test cases like this (and probably others, total failed cases are 367). I got this from failed test cases (ninja check-mlir) after applying my patch.

I think that adding a whitespace in all cases would be consistent and will have a straightforward fix.

[joker-eph]

I think that adding a whitespace in all cases would be consistent and will have a straightforward fix.

I don't see what is "consistent" about adding an extra whitespace actually: it does not follow any existing convention and seems totally out-of-place to me.

[joker-eph]

This API has a bool withKeyword parameter, why not add a bool addSpacePrefix = true option?

[weirdsmiley]

1. For the "consistency" part, the issue raised here has been that under cases where there is one attribute with elidedProps, then printer puts a whitespace, while if there are more than one attribute then it won't. For example, this test shows both cases which are different during printing.

// CHECK: test.format_optional_prop_dict < {a = ["foo"]}>
//                                        ^ extra whitespace
test.format_optional_prop_dict <{a = ["foo"]}>

// CHECK: test.format_optional_prop_dict < {b = 2 : i32}>
//                                        ^ extra whitespace
test.format_optional_prop_dict <{b = 2 : i32}>

// CHECK: test.format_optional_prop_dict <{a = ["foo"], b = 2 : i32}>
//                                        ^ no whitespace

2. We can add option bool addSpacePrefix = true. But if user sets it to false then do we expect to put no whitespaces in prefix? That would result in above said situations like module attributes{.

[joker-eph]

That would result in above said situations like module attributes{.

It wouldn't because the flag wouldn't be false in this context.

[joker-eph]

// CHECK: test.format_optional_prop_dict <{a = ["foo"], b = 2 : i32}>
// ^ no whitespace

What is the codepath that gets us there? Maybe it is just a matter of calling the right API instead of this one.

[joker-eph]

Here is a fix for this bug: #145962