Extend TestingGuide (adds notes on formatting)

[banach-space]

No description provided.

[joker-eph]

Thanks!

[joker-eph]

This a good addition: I would still add some documentation in comment to a test: it's not always clear just from a test name what's going on, and the same logic as in code applies (document the why, the context, the "what's no obvious from the code").

[banach-space]

Yes, comments are also super important, thanks for raising this!

I have some ideas on "comments", so let me treat this as encouragement to extend this PR with a dedicated section on documentation :)

[jpienaar]

Thanks! Yes discoverability is important here.

[rengolin]

Awesome addition. All good practices that we've been telling each other for years but always too lazy to encode in a document. :)

[kuhar]

When you say 'existing style', what scope do you have in mind? It could be that the style of a specific directory is inconsistent with the style used in majority of other dialects etc. IE, do we want to make things globally or locally consistent, or something else?

[banach-space]

I’m deliberately avoiding specifying this :)

Making things globally consistent would be great for... consistency. However, that assumes one size fits all, which isn't always realistic. Given the current state of things, I also feel this would be an impractical goal.
The same applies to per-file vs per-directory consistency. For example, "mlir/test/Dialect/Vector" has around 50 files - enforcing 100% consistency across them would require significant effort and churn.

My view is that contributors and reviewers should make the right call on a case-by-case basis while being mindful of:

• Reinventing the wheel in terms of formatting.
• New tests ignoring existing conventions (whether at the file or directory level).

Ultimately, I see this as a gradual process. Let’s start with these guidelines to help "correct the course." Over time, we can refine them - possibly transitioning from guideline to requirement if it makes sense.

WDYT?

[kuhar]

Suggested change

	[vector.maskedload](https://mlir.llvm.org/docs/Dialects/Vector/#vectormaskedload-vectormaskedloadop)
	[`vector.maskedload`](https://mlir.llvm.org/docs/Dialects/Vector/#vectormaskedload-vectormaskedloadop)

[banach-space]

I am slightly worried that this won't render well, but lets try!

[kuhar]

Do we need all 3 examples to drive your point or could we limit this to just two?

[banach-space]

"Generalization Takes Three Examples" :)

It allows me to go into a bit of nuance in this paragraph (i.e. to identify the commonalities):

  ##### Step 2: Improve Test Naming

  Instead of using "regression" (which doesn't add unique information), rename
  tests based on key attributes:

  * All examples test the `vector.maskedload` to `vector.load` lowering.
  * The first test uses a _dynamically_ shaped `memref`, while the others use
    _static_ shapes.
  * The mask in the first two examples is "all true" (`vector.constant_mas
    [16]`), while it is "all false" (`vector.constant_mask [0]`) in the thir
    example.
  * The first and the third tests use `i32` elements, whereas the second uses
    `i8`.

That's not something I feel super strongly about. Usually its tricky to see the "optimal" solution on first iteration and I guess that's the case here.

[adam-smnk]

The key attributes should reflect what's important for the tested transform, in this case having different data types is good for completeness but they don't trigger any special/separate logic.
It would be sufficient to highlight one case that uses different data type with, for example, a suffix and omit data type for others.

I know that's not the intention behind this example but it can be misused to push for "enterprisification" (classic java gem: https://projects.haykranen.nl/java/) of naming with combinatorial explosion of all the little details.

[banach-space]

These are good points and I have come across this myself (without a good solution).

One specific example are Vector ops that can represent "scalars" as:

• vector<1xf32> vs vector<f32> vs f32.

To differentiate tests, we started adding things like @name_from_vec0d_to_vec1d, @name_from_vec1d_to_vec0d ... It spirals very quickly.

I don't have a solution for this, but will definitely incorporate your suggestion to ... not forget about the common sense 😅

[banach-space]

Added a short paragraph in this commit. Let me know what you think - do we need more?

[adam-smnk]

nit: Should the existing bad style (test_1, test_2, ...) prevent naming new test cases more reasonably?

[banach-space]

Great point, thanks! Let me add a dedicated paragraph on that.

[banach-space]

Thank you for the reviews 🙏🏻 I've addressed most of your comments + added a section on "commenting"/"documenting".

Let me know what you think and I will happily iterate based on the feedback.

[dcaballe]

Thanks a lot! We definitely needed some specific guidelines here. Thanks for putting in the time!

I think it might be helpful to clarify the level of enforcement we expect for these guidelines and the degree of flexibility we should allow. I fully agree with the motivation points highlighted in the PR but I'm also worried that we make things painfully rigid. If we set the bar too high, there’s a risk that testing becomes tedious enough to potentially impact actual coverage. I’m also worried that some of the renaming/formatting aspects of the guidelines take the focus away in the code reviews from other aspects that should have more attention.

To alleviate some of this, esp. if we are going after a high level of enforcement, I think we should put some effort into automating some of the renaming/formatting aspects. For example, we could extend generate-test-checks.py (or implement a new tool) for MLIR so that it generates more meaningful CHECK variable names by default, derived from the dialect or op name, for example. That could greatly enhance consistency by default while minimizing manual effort.

I also feel consistency is open to interpretation. For example, if an existing test uses V_INPUT_0 as a CHECK variable name and a new test is added with a INPUT0 variable name, should we enforce a change? That name seems valid to me and we don’t enforce such a level of naming consistency on the C++ code. I think we may want to allow the same level of flexibility on the testing side.

Anyways, some food for thoughts and probably unpopular opinions I wanted to share 😊. Great writeup and ideas overall. Thanks!

[banach-space]

Thank you for the generous feedback, Diego! These are some great points.

Before addressing specific comments, I see the introduction of new testing standards as a three-step process:

1. Codify expectations (this PR).
2. Endorse & embrace—let the community adapt and refine the guidelines.
3. Enforce—once broadly adopted, introduce tooling to assist with enforcement.

This is merely Step 1. Right now, we have neither guidelines nor a clear place to document best practices.

I think it might be helpful to clarify the level of enforcement we expect for these guidelines and the degree of flexibility we should allow.

This might be tricky, and I'd rather leave it under-specified. Many LLVM guidelines are also loosely defined, and I think that’s fine.

I’m also worried that some of the renaming/formatting aspects of the guidelines take the focus away in the code reviews from other aspects that should have more attention.

Agreed - there needs to be a healthy balance. However, IMHO, not enough effort is put into future-proofing and maintaining our tests.

My hope is that these guidelines will streamline discussions and reduce time spent debating formats in reviews. Ultimately, this should free up time to focus on the actual code.

If we are going after a high level of enforcement, I think we should put some effort into automating some of the renaming/formatting aspects.

I don’t think we should automate this just yet.

For example, we could extend generate-test-checks.py (or implement a new tool) for MLIR so that it generates more meaningful CHECK variable names by default, derived from the dialect or op name.

I fully agree that better tooling would help, and this is something we should aim for in the future. However, there’s little point in improving tooling if we haven’t yet agreed on what we want to standardize.

• First, we need to (1) codify best practices.
• Once the community has (2) embraced these guidelines, we can explore ways to automate (3) enforcement.

I’ve considered extending generate-test-checks.py, but I believe we will always need human input to provide meaningful naming based on semantics.

Below is an automatically generated test check using today's version:

// Automatically generated - today’s version
// CHECK-LABEL:   func.func @bitcast_2d(
// CHECK-SAME:                          %[[VAL_0:[0-9]+|[a-zA-Z$._-][a-zA-Z0-9$._-]*]]: vector<2x4xi32>) -> vector<2x16xi8> {
// CHECK:           %[[VAL_1:.*]] = vector.bitcast %[[VAL_0]] : vector<2x4xi32> to vector<2x2xi64>
// CHECK:           %[[VAL_2:.*]] = vector.bitcast %[[VAL_1]] : vector<2x2xi64> to vector<2x16xi8>
// CHECK:           return %[[VAL_2]] : vector<2x16xi8>
func.func @bitcast_2d(%arg0: vector<2x4xi32>) -> vector<2x16xi8> {
  %0 = vector.bitcast %arg0 : vector<2x4xi32> to vector<2x2xi64>
  %1 = vector.bitcast %0 : vector<2x2xi64> to vector<2x16xi8>
  return %1 : vector<2x16xi8>
}

For a mild improvement, we could extend generate-test-checks.py to rename VAL_0 and VAL_1 to BITCAST_1 and BITCAST_2:

// Automatically generated - possible near-future version (mild improvement)
// CHECK-LABEL:   func.func @bitcast_2d(
// CHECK-SAME:                          %[[ARG_0]]: vector<2x4xi32>) -> vector<2x16xi8> {
// CHECK:           %[[BITCAST_1:.*]] = vector.bitcast %[[ARG_0]] : vector<2x4xi32> to vector<2x2xi64>
// CHECK:           %[[BITCAST_2.:*]] = vector.bitcast %[[BITCAST_1]] : vector<2x2xi64> to vector<2x16xi8>
// CHECK:           return %[[BITCAST_2]] : vector<2x16xi8>
func.func @bitcast_2d(%arg0: vector<2x4xi32>) -> vector<2x16xi8> {
  %0 = vector.bitcast %arg0 : vector<2x4xi32> to vector<2x2xi64>
  %1 = vector.bitcast %0 : vector<2x2xi64> to vector<2x16xi8>
  return %1 : vector<2x16xi8>
}

However, ideally, we’d want something context-aware, such as:

// Human-curated, context-aware naming
// CHECK-LABEL:   func.func @bitcast_2d(
// CHECK-SAME:                          %[[ARG_0]]: vector<2x4xi32>) -> vector<2x16xi8> {
// CHECK:           %[[UPCAST:.*]] = vector.bitcast %[[ARG_0]] : vector<2x4xi32> to vector<2x2xi64>
// CHECK:           %[[DOWNCAST.:*]] = vector.bitcast %[[UPCAST]] : vector<2x2xi64> to vector<2x16xi8>
// CHECK:           return %[[DOWNCAST]] : vector<2x16xi8>
func.func @bitcast_2d(%arg0: vector<2x4xi32>) -> vector<2x16xi8> {
  %0 = vector.bitcast %arg0 : vector<2x4xi32> to vector<2x2xi64>
  %1 = vector.bitcast %0 : vector<2x2xi64> to vector<2x16xi8>
  return %1 : vector<2x16xi8>
}

To achieve this level of naming automation, we’d likely need LSP-based tooling, which is beyond my current capacity to explore.

I also feel consistency is open to interpretation.

Yes, I deliberately left "consistency" under-specified. I feel it's something where we should trust our judgment. See also this comment from Jakub (my reply).

Ultimately, we could reduce it to a simple question:

• "Are you re-using an existing style?"
  • If yes, then "You are maintaining a level of consistency, thank you!"
  • If not, then "Please provide rationale." (e.g. "There's no pre-exisitng style to follow.")
• Most importantly, "Please be mindful of consistency when contributing tests 🙏🏻 "

If an existing test uses V_INPUT_0 as a CHECK variable name and a new test is added with INPUT0, should we enforce a change?

In this case, assuming V_INPUT_0 appears in every test in a given file, I would ask the contributor:


• Why not keep things simple and consistent? Following the existing style makes it easier to compare test cases and identify edge cases. If you choose to diverge, please provide a rationale.

Ultimately, we should also trust our judgment - both as reviewers and contributors.

---

Final point

The actual guidelines/requirements boil down to:

• Be consistent and, where possible, follow the existing style.
• Document your tests using comments, meaningful function names, and descriptive MLIR/LIT variable names.

The examples I included in this PR are just that - examples. I don’t think the general principles are sufficient without concrete illustrations, but again, these are just suggestions to inspire contributors rather than rigid rules.

Thanks for considering this!