-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[mlir][tblgen] Adds support for embedded LIT tests in TableGen records #158017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jkshtj
wants to merge
1
commit into
llvm:main
Choose a base branch
from
jkshtj:jkshtj/tblgen-embedded-lit-tests
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
//===-- Testable.td - Testable type definition file --------*- tablegen -*-===// | ||
// | ||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
// See https://llvm.org/LICENSE.txt for license information. | ||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
// | ||
//===----------------------------------------------------------------------===// | ||
// | ||
// This file contains the definition of the `Testable` type. | ||
// | ||
// Any type whose records can have corresponding LIT tests (eg - Pass) can extend | ||
// `Testable` in order to be able to embed LIT tests within record definitions. | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#ifndef TESTABLE | ||
#define TESTABLE | ||
|
||
// Represents a LIT test record in TableGen | ||
class LitTest<string name, code snippet, list<string> run = [], list<string> check = []> { | ||
// The name of the generated test file | ||
string testFileName = name; | ||
|
||
// The IR snippet/code to be tested | ||
code irSnippet = snippet; | ||
|
||
// The RUN commands for the test (e.g., "mlir-opt %s") | ||
list<string> runLines = run; | ||
|
||
// Expected output patterns (CHECK lines) | ||
list<string> checkLines = check; | ||
} | ||
|
||
// Base class for elements that can have auto-generated LIT tests | ||
class Testable { | ||
// List of LIT tests associated with this element | ||
list<LitTest> tests = []; | ||
} | ||
|
||
#endif // TESTABLE |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
// RUN: mlir-tblgen -gen-lit-tests -I %S/../../include -dialect=test %s | FileCheck %s | ||
|
||
include "mlir/Pass/PassBase.td" | ||
include "mlir/IR/Testable.td" | ||
|
||
def TestPassWithEmbeddedLitTests : Pass<"test-pass-with-embedded-lit-tests"> { | ||
let summary = "pass summary"; | ||
let description = [{ | ||
Pass description | ||
}]; | ||
|
||
let tests = [ | ||
LitTest< | ||
"lit_test_file_1.mlir", | ||
[{ | ||
func.func @test1() { | ||
return 42; | ||
} | ||
}], | ||
[ | ||
"// RUN: mlir-opt %s --verify-roundtrip | FileCheck %s", | ||
], | ||
[ | ||
"// RANDOM-CHECK-LABEL: func.func @test1", | ||
] | ||
>, | ||
LitTest< | ||
"lit_test_file_2.mlir", | ||
[{ | ||
func.func @test2() { | ||
return 42; | ||
} | ||
}], | ||
[ | ||
"// RUN: mlir-opt %s --verify-roundtrip | FileCheck %s", | ||
], | ||
[ | ||
"// RANDOM-CHECK-LABEL: func.func @test2", | ||
] | ||
>, | ||
]; | ||
} | ||
|
||
// CHECK-LABEL: // Generated 2 LIT test files | ||
// CHECK: // Use the following files for LIT testing: | ||
|
||
// CHECK: // File: generated_TestPassWithEmbeddedLitTests_lit_test_file_1.mlir | ||
// CHECK: // --- BEGIN generated_TestPassWithEmbeddedLitTests_lit_test_file_1.mlir --- | ||
// CHECK: // RUN: mlir-opt %s --verify-roundtrip | FileCheck %s | ||
// CHECK: // Generated from TableGen definition: TestPassWithEmbeddedLitTests | ||
// CHECK: func.func @test1() { | ||
// CHECK: return 42; | ||
// CHECK: } | ||
// CHECK: // RANDOM-CHECK-LABEL: func.func @test1 | ||
// CHECK: --- END generated_TestPassWithEmbeddedLitTests_lit_test_file_1.mlir --- | ||
|
||
// CHECK: // File: generated_TestPassWithEmbeddedLitTests_lit_test_file_2.mlir | ||
// CHECK: // --- BEGIN generated_TestPassWithEmbeddedLitTests_lit_test_file_2.mlir --- | ||
// CHECK: // RUN: mlir-opt %s --verify-roundtrip | FileCheck %s | ||
// CHECK: // Generated from TableGen definition: TestPassWithEmbeddedLitTests | ||
// CHECK: func.func @test2() { | ||
// CHECK: return 42; | ||
// CHECK: } | ||
// CHECK: // RANDOM-CHECK-LABEL: func.func @test2 | ||
// CHECK: // --- END generated_TestPassWithEmbeddedLitTests_lit_test_file_2.mlir --- |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than try to reconstruct the list test from separate records, it seems like it would be simpler to have a single text field that contains the entire lit test, and this feature would limit the allowed complexity of the RUN line. (e.g., only one RUN line with one pipe)
Or you could have the entire lit test in one field, except for the RUN line, and autogenerate the run line as
// RUN: mlir-opt %s <pass-flag> | FileCheck %s
, since<pass-flag>
is already part of the tablegen record and also it would be a good practice to ensure that the behavior tested/documented for a pass is restricted to just what that one pass does.If you render the before/after example from the lit test, then, you could just strip out any lines starting with
\s*// CHECK
.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the same, we actually have a prototype downstream.
We automatically generate the round-trip testing from this, and use it to generate an "example" section in the documentation, while stripping the lines starting with # from the documentation, which allows to build more complex example without clobbering the documentation with the surrounding noise.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a separate tablegen field, I would consider some markdown in the description field, that we can do:
This can then be similarly extracted for round-trip test generation by default, while also integrating nicely when generating the markdown documentation (eliding the commented out lines).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestions. In general I agree with the ideas you guys mentioned. I have a few questions to get a better understanding of the desired end product.
Do we always want to include the IR examples in the documentation or do we want to give the user an option to only generate LIT tests from them?
If it's the former then we'd always need the user to also specify the full "after" version of the IR. This "after" version obviously can be used as the CHECK lines in the test. It does increase the onus on the user a bit, but should give high-quality before/after versions of the IR to go along with documentation, as well as precise LIT tests.
If it's the latter, we'd still need the user to specify CHECK lines, but It'd be quite complicated to generate the "after" versions of the IR for documentation.
It seemed to me, from the comments, that there may be a third option of just generating a
--verify-roundtrip
test.If so, I'm not sure what utility such IR can provide either for documentation or for testing.
I personally prefer option 1, and have the user always always specify IR examples in before/after pairs.
Thoughts?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's not the documentation, I'd question why is it in ODS?
I don't quite get what is the "after" version in the context of the documentation?
Right.
I don't follow: the IR here isn't meant to be useful for testing, we already have tests for it. The problem statement as I understand it is that we want examples in the doc that are maintained up-to-date in terms of being "correct". That is: we want to test the documentation to actually have examples reflecting the implementation.
Is there another problem to solve that I'm missing?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see the misalignment in our expectations now.
You're primarily thinking about tests for operations while I started out with working on tests for passes. For operations, I agree that all we need are
--verify-roundtrip
tests. For passes however, I think it makes sense to have the user specify before/after IR snippets that we generate the tests from.And I do like the idea of having the snippets be a part of the
description
field itself, so as to not introduce additional complications for documentation rendering. Let me take another shot at things based on the feedback.Thank you for your comments!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh passes is a whole other world, I'm not sure what would make sense for passes, I would need to see any proposal with concrete example on a few actual passes in the code base to figure out.