This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Tosa] Pass encoding through `tosa-to-linalg`
ClosedPublic

Authored by rikhuijzer on Jun 5 2023, 8:25 AM.

Details

Summary

As pointed out by @Sinclair-Dee in
https://github.com/llvm/llvm-project/issues/62304, the tosa-to-linalg
conversion ignored the encoding attribute.

Also, this patch avoids an assertion error crash on unranked tensors.
Instead, the conversion now throws a "failed to legalize" error.

Fixes #62304 and fixes #63165.

Diff Detail

Event Timeline

rikhuijzer created this revision.Jun 5 2023, 8:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2023, 8:25 AM
rikhuijzer requested review of this revision.Jun 5 2023, 8:25 AM
rikhuijzer edited the summary of this revision. (Show Details)Jun 5 2023, 8:26 AM
mehdi_amini added inline comments.Jun 5 2023, 11:59 AM
mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp
565–566

If you dyn_cast that means we expect that this conversion can fail, so it should be checked. Otherwise if we know by construction that this can't fail we should cast.

rikhuijzer updated this revision to Diff 528744.Jun 6 2023, 1:57 AM

Add assertion after dyn_cast

In response to the comment by Mehdi, I added an assertion since I'm
unable to verify that the cast will always succeed. It looks to me
like the only situation in which the cast would fail is in case of an
UnrankedTensorType and I haven't been able to produce a test which
would trigger that. However I'm definitely no MLIR expert, so a kind
request to the reviewers to double check this.

mehdi_amini added inline comments.Jun 6 2023, 2:02 AM
mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp
566

cast embeds an assertion already: the use of dyn_cast is intended for gracefully handling the result.

rkayaith added inline comments.
mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp
565–566

i don't think you need a cast here anyways

Make assumptions more explicit

Jacques Pienaar pointed out on Discord that the code being ranked was already
assumed at an earlier point in the code. This patch makes that and some other
assumptions more explicit.

rikhuijzer edited the summary of this revision. (Show Details)Jun 9 2023, 11:25 AM
rikhuijzer added a comment.EditedJun 9 2023, 11:44 AM

To be clear to the reviewer, note that the line

unsigned rank = resultTy.getRank();

would crash on an unranked tensor anyway, so the introduction of the explicit failure on !rankedType does not change program behavior (before this patch, the program crashed on unranked tensors; after this patch, the program throws an error as is tested in the added test).

Avoid duplicate cast

rikhuijzer edited the summary of this revision. (Show Details)Jun 13 2023, 3:16 AM
mehdi_amini accepted this revision.Jun 15 2023, 8:50 AM
This revision is now accepted and ready to land.Jun 15 2023, 8:50 AM
This revision was automatically updated to reflect the committed changes.