This is an archive of the discontinued LLVM Phabricator instance.

[mlir][sparse] Add checking parent op of SortOp

Authored by sotoshigoto on Feb 12 2023, 8:57 PM.



Fix crash with segmentation fault caused by setting a parent operator
that is not func::FuncOp with sparse_tensor SortOp.


Diff Detail

Event Timeline

sotoshigoto created this revision.Feb 12 2023, 8:57 PM
Herald added a project: Restricted Project. · View Herald Transcript
sotoshigoto requested review of this revision.Feb 12 2023, 8:57 PM
aartbik added inline comments.Feb 14 2023, 12:02 PM
1023 ↗(On Diff #496834)

Thanks for filing the bug and looking into this. The code crashes since the rewriting indeed assumes to find a func op during codegen, in particular

auto insertPoint = op->template getParentOfType<func::FuncOp>();

during SparseBufferRewriting. However, it seems we need to fix this at the rewriting level, not in verification of the op (which is really a user facing validation).

Move a fix point from verification of op to rewriting level

sotoshigoto marked an inline comment as done.Feb 14 2023, 5:38 PM

remove unnecessary an include

This comment was removed by wrengr.
wrengr accepted this revision.Feb 14 2023, 5:52 PM

Since matchAndRewriteSortOp was already getting the insertPoint, it seems perfectly reasonable to have the function return failure if it can't do so. I also like that it catches the error exactly where it is introduced: if we changed the function to compute insertPoint differently, then the check is still valid (and if we change the function to not compute insertPoint at all, then it's also clear that we should remove the check).

Thanks :)

This revision is now accepted and ready to land.Feb 14 2023, 5:52 PM
aartbik accepted this revision.Feb 14 2023, 5:56 PM
sotoshigoto added a comment.EditedFeb 16 2023, 4:56 PM

@aartbik @wrengr Could you land this patch? I don't have commit access.

This revision was automatically updated to reflect the committed changes.

@aartbik @wrengr Could you land this patch? I don't have commit access.

Thanks Alex for landing! I missed this request.