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

[BACKEND] Fix asserts in 2d scan and add assert mode to layout tests #5075

Merged
merged 2 commits into from
Nov 5, 2024

Conversation

peterbell10
Copy link
Contributor

This is a follow up to #5033 but for scan ops, and also improving the testing as it was clearly insufficient before.

@peterbell10 peterbell10 requested a review from ptillet as a code owner November 5, 2024 17:51
Comment on lines +2551 to +2561
overflow_check = """
%17 = arith.extsi %arg2 : i32 to i64
%18 = arith.extsi %arg3 : i32 to i64
%19 = arith.addi %17, %18 : i64
%i32.min = arith.constant -2147483648: i64
%i32.max = arith.constant 2147483647: i64
%20 = arith.cmpi slt, %19, %i32.max : i64
%21 = arith.cmpi sge, %19, %i32.min : i64
%22 = arith.andi %20, %21 : i1
tt.assert %22, "overflow detected" : i1
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

isn't this likely to break often whenever we do small changes? Should this be a lit test instead?

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 want to check that the code produces the expected result, so I don't think a lit test helps here. Also not sure what changes you had in mind, this would only break if there are significant changes to the ir format which seems unlikely.

Copy link
Collaborator

Choose a reason for hiding this comment

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

if a pass slightly change the IR the value name may be different for instance? In general that's why in LIT tests we try not to hardcode value names.

I want to check that the code produces the expected result, so I don't think a lit test helps here.

ah right

Anyway not a blocker but we may have to find something better if it does break when changing unrelated part of the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if a pass slightly change the IR the value name may be different for instance?

We're providing the gpu ir directly here so there are no passes applied to this IR. The only way the argument names might change is if you manually edit the test itself.

Copy link
Collaborator

Choose a reason for hiding this comment

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

duh sorry I read too fast I though we were checking for this IR. scratch what I said

@peterbell10 peterbell10 enabled auto-merge (squash) November 5, 2024 19:37
@peterbell10 peterbell10 merged commit 11d85f7 into main Nov 5, 2024
6 of 7 checks passed
@peterbell10 peterbell10 deleted the pb/fix-sanitize-scan branch November 5, 2024 19:40
bertmaher pushed a commit that referenced this pull request Nov 5, 2024
…5075)

This is a follow up to #5033 but for scan ops, and also improving the
testing as it was clearly insufficient before.
antiagainst added a commit to antiagainst/triton that referenced this pull request Nov 5, 2024
@antiagainst
Copy link
Collaborator

A heads up that this change causes Python tests on AMD CI to go from ~3min to ~30min. (And it also failed on MI210 before landing but it was not blocking there.)

I think it's likely due to that we use tt.assert which aborts the whole program on AMD. We may have issues in the AMD backend that causes tests to go to a retry-reabort loop? Such took a long time. While figuring out the exact reasons I have #5078 to disable these tests for now.

Luosuu pushed a commit to Luosuu/triton that referenced this pull request Nov 13, 2024
…riton-lang#5075)

This is a follow up to triton-lang#5033 but for scan ops, and also improving the
testing as it was clearly insufficient before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants