This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Added strides check to rank reducing subview verification

Authored by limo1996 on Oct 6 2020, 2:15 AM.



Added missing strides check to verification method of rank reducing subview
which enforces strides specification for the resulting type.

Diff Detail

Event Timeline

limo1996 created this revision.Oct 6 2020, 2:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2020, 2:15 AM
limo1996 requested review of this revision.Oct 6 2020, 2:15 AM
nicolasvasilache requested changes to this revision.Oct 6 2020, 2:34 AM

Thanks for fixing this @limo1996 , let's please improve the error messages and add invalid tests for each covered case.


This function has too many failure cases that are captured by "return false".
Let's please make it return an enum with different failure cases and add better error messages + relevant invalid test for each message.


This does not fail because of the stride but because of something else.
We should have more coverage re mixed static and dynamic strides verification errors.

This revision now requires changes to proceed.Oct 6 2020, 2:34 AM
limo1996 updated this revision to Diff 296698.Oct 7 2020, 8:46 AM

Comments of nicolas addressed

limo1996 marked an inline comment as done.Oct 7 2020, 8:46 AM
nicolasvasilache accepted this revision.Oct 7 2020, 8:52 AM

Looks nice, thanks @limo1996


s/OK/Success for consistency with rest of the codebase?


I'd just rewrite as: expected result type ... (mismatching result sizes) here and below.
This would be more consistent with other error messages for other ops which essentially print as:

/tmp/tmp.mlir:29:8: error: expected SSA operand
^bb42 (i32): // expected-error {{expected SSA operand}}
This revision is now accepted and ready to land.Oct 7 2020, 8:52 AM
limo1996 updated this revision to Diff 296880.Oct 8 2020, 12:39 AM
limo1996 marked 2 inline comments as done.

OK->success, error messages changed a bit

nicolasvasilache accepted this revision.Oct 8 2020, 12:59 AM

Thanks! Let's land this.

This revision was landed with ongoing or failed builds.Oct 8 2020, 1:39 AM
This revision was automatically updated to reflect the committed changes.