Skip to content

Add back barrier after asserts#5043

Merged
ThomasRaoux merged 1 commit into
triton-lang:mainfrom
ThomasRaoux:assert_fix
Nov 2, 2024
Merged

Add back barrier after asserts#5043
ThomasRaoux merged 1 commit into
triton-lang:mainfrom
ThomasRaoux:assert_fix

Conversation

@ThomasRaoux
Copy link
Copy Markdown
Collaborator

@ThomasRaoux ThomasRaoux commented Nov 1, 2024

support asserts with scalar condition and only emit barrier for assert of tensors.
Thanks to @peterbell10 for the suggestion.

// by an op that may trap if the assert condition is true. Since the
// tensor in those two operations may have different layout we need to
// make sure all the threads are done executing the assert before going to
// the next op.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

btw this highlights an interesting thing I noticed a while back. Sometimes we do the same computation in multiple different layouts when there are assert statements because the assertion path doesn't have a data flow path to the next layout anchor operation.

It's not really the end of the world, but an interesting quirk of the remove layout conversion code.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

right, yes this is definitely a limitation as we haven't tried very hard to make assert efficient.

// those two operations may have different layout we need to make sure all
// the threads are done executing the assert before going to the next op.
barrier();
if (isa<RankedTensorType>(condition.getType())) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh I think this should be

Suggested change
if (isa<RankedTensorType>(condition.getType())) {
if (isa<RankedTensorType>(op.getCondition().getType())) {

@ThomasRaoux
Copy link
Copy Markdown
Collaborator Author

Actually I realize now asserts should always take a tensor:

def TT_AssertOp : TT_Op<"assert", [MemoryEffects<[MemWrite<GlobalMemory>]>]> {
  let summary = "Device-side assert, as in CUDA for correctness checking";
  let description = [{
    `tt.assert` takes a condition tensor and a message string.
    If the condition is false, the message is printed, and the program is aborted.
  }];
  let arguments = (ins TT_Tensor:$condition, StrAttr:$message);
  let assemblyFormat = "$condition `,` $message attr-dict `:` type($condition)";
}

I'm now wondering how it works within a reduction region. Do we have example somewhere?

@peterbell10
Copy link
Copy Markdown
Contributor

Hrm okay, an example can be the kernel in test_side_effectful_reduction. I guess we probably promote scalars to tensor in the frontend. Would it be unreasonable to add support for scalars?

@peterbell10
Copy link
Copy Markdown
Contributor

I guess the region verifier isn't strong enough, but really I don't think any op that has a RankedTensor type should be allowed inside a reduction region.

@ThomasRaoux
Copy link
Copy Markdown
Collaborator Author

yeah I agree, I don't think there is any reason why the condition has to be tensor. I'll try to fix that part

@ThomasRaoux ThomasRaoux changed the title Only insert barrier after assert for tensor conditions Add back barrier after asserts Nov 2, 2024
@ThomasRaoux ThomasRaoux enabled auto-merge (squash) November 2, 2024 02:01
@ThomasRaoux ThomasRaoux merged commit 92a4fad into triton-lang:main Nov 2, 2024
bertmaher pushed a commit that referenced this pull request Nov 5, 2024
support asserts with scalar condition and only emit barrier for assert
of tensors.
Thanks to @peterbell10 for the suggestion.
Luosuu pushed a commit to Luosuu/triton that referenced this pull request Nov 13, 2024
support asserts with scalar condition and only emit barrier for assert
of tensors.
Thanks to @peterbell10 for the suggestion.
guacamoleo pushed a commit to guacamoleo/triton that referenced this pull request Nov 14, 2024
support asserts with scalar condition and only emit barrier for assert
of tensors.
Thanks to @peterbell10 for the suggestion.
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.

2 participants