Skip to content

Avoid assertion failure from SetFuncAllowAttrs when struct is empty#231

Merged
tim-hoffman merged 3 commits intomainfrom
th/fix_SetFuncAllowAttrs_assert
Dec 3, 2025
Merged

Avoid assertion failure from SetFuncAllowAttrs when struct is empty#231
tim-hoffman merged 3 commits intomainfrom
th/fix_SetFuncAllowAttrs_assert

Conversation

@tim-hoffman
Copy link
Member

No description provided.

@tim-hoffman tim-hoffman requested a review from a team December 2, 2025 21:58
Copy link
Contributor

@iangneal iangneal left a comment

Choose a reason for hiding this comment

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

I'm not sure that we should allow empty structs, since an empty struct can never be used---a call to @compute or @product is required to create a struct (since those are the only places where struct.new is allowed). Is there a use case for this?

@tim-hoffman
Copy link
Member Author

I'm not sure that we should allow empty structs, since an empty struct can never be used---a call to @compute or @product is required to create a struct (since those are the only places where struct.new is allowed). Is there a use case for this?

That's a fair point. I ran into this in a unit test that uses the C API to create a StructDefOp and then calls the verifier (without generating anything else within the StructDefOp). It had an unhelpful assertion failure within getBody() so I avoided that with this PR.
Here's the weird part. The test uses the C API wrapper for mlir::verify() (in mlir/lib/IR/Verifier.cpp) and we do have the check you mention in StructDefOp::verifyRegions() but I guess the former does not call the latter. In fact, I'm just now noticing that it too calls getBody() which would fail in my test. Let me try a few things and I'll probably push more to this PR.

When calling `StructDefOp::verifyRegions()` with an empty struct, the `getBody()` call would give an assertion failure. Instead, check for that case and give the normal error message.
@tim-hoffman
Copy link
Member Author

@iangneal I fixed the same issue in StructDefOp::verifyRegions() as mentioned earlier. The difference between mlir::verify() not checking regions and verifyRegions() doing so should be expected behavior I believe.

@tim-hoffman tim-hoffman requested a review from iangneal December 3, 2025 06:10
@tim-hoffman tim-hoffman requested a review from iangneal December 3, 2025 16:56
Copy link
Contributor

@iangneal iangneal left a comment

Choose a reason for hiding this comment

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

LGTM.

@tim-hoffman tim-hoffman merged commit ef8fd3f into main Dec 3, 2025
12 checks passed
@tim-hoffman tim-hoffman deleted the th/fix_SetFuncAllowAttrs_assert branch December 3, 2025 17:24
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