Skip to content
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

Suppress testing SemIR in int builtin tests. #4748

Merged
merged 9 commits into from
Dec 31, 2024

Conversation

zygoloid
Copy link
Contributor

Add EXTRA-ARGS: support to file_test, to add arguments without
overriding the default arguments. Use EXTRA-ARGS: --no-dump-sem-ir to
turn off SemIR dumping and thus SemIR testing in the int builtin tests,
which validate correct behavior through diagnostics instead.

This doesn't get us any closer to supporting more targeted SemIR dumping
/ testing, but this seems to be a generally useful feature anyway. Most existing
tests using ARGS have been switched over to using EXTRA-ARGS.

Requested in review of #4716.

Add `EXTRA-ARGS:` support to file_test, to add arguments without
overriding the default arguments. Use `EXTRA-ARGS: --no-dump-sem-ir` to
turn off SemIR dumping and thus SemIR testing in the int builtin tests,
which validate correct behavior through diagnostics instead.

This doesn't get us any closer to supporting more targeted SemIR dumping
/ testing, but hopefully this is a generally useful feature for argument
testing.

Requested in review of carbon-language#4716.
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I think one suggestion inline about handling RuntimeCall testing.

Comment on lines 20 to 22
fn RuntimeCall(a: i32, b: i32) -> i32 {
return And(a, b);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SemIR seams still useful, and the only real test we have for the runtime call case. Would it make sense to split the runtime call tests out to files that continue to dump SemIR and only skip it in the ones where we do compile time validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the only thing this is testing that's specific to each builtin is that a runtime call is valid. Beside that, the handling is not specific to the particular builtin -- we get the same call SemIR regardless of the kind of builtin -- so I think it's probably good enough to test that only once rather than once per builtin.

To that end, I've gone ahead and added SemIR output coverage for runtime calls to toolchain/check/testdata/function/builtin/call.carbon (which previously only covered compile-time calls).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure... Maybe RuntimeCallIsValid or something to make that a bit more obvious why there is nothing verified beyond that?

@zygoloid zygoloid requested a review from chandlerc December 30, 2024 23:25
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG!

Comment on lines 20 to 22
fn RuntimeCall(a: i32, b: i32) -> i32 {
return And(a, b);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure... Maybe RuntimeCallIsValid or something to make that a bit more obvious why there is nothing verified beyond that?

@zygoloid zygoloid enabled auto-merge December 31, 2024 00:31
@zygoloid zygoloid added this pull request to the merge queue Dec 31, 2024
Merged via the queue into carbon-language:trunk with commit d416683 Dec 31, 2024
8 checks passed
@zygoloid zygoloid deleted the toolchain-non-ir-tests branch December 31, 2024 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants