View op test case invalid syntax

[bondhugula]

The syntax here appears to be messed up. https://github.com/tensorflow/mlir/blob/b7e6270e6a06fb67b5215e1647774143abaf506c/test/Dialect/Linalg/loops.mlir#L19

From ops.td, the view op takes the offset first followed by sizes (reproduced below), while the test case above passes sizes followed by offset. Not sure why the test case IR even verifies.

// ViewOp with dynamic offset and one dynamic size.
    %2 = view %0[%offset_1024][%size0]
      : memref<2048xi8> to memref<?x4xf32, (d0, d1)[s0] -> (d0 * 4 + d1 + s0)>

[ftynse]

The parser seems to accept a list of offsets https://github.com/tensorflow/mlir/blob/master/lib/Dialect/StandardOps/Ops.cpp#L2332 and the verifier does not catch it.

[bondhugula]

The parser seems to accept a list of offsets https://github.com/tensorflow/mlir/blob/master/lib/Dialect/StandardOps/Ops.cpp#L2332 and the verifier does not catch it.

The number of 'sizes' is also one less!

[ftynse]

This is deeper than I thought, in several ways. I have a pending fix for the ordering problem, and there were incorrect assumptions in other places.

[ftynse]

The number of 'sizes' is also one less!

I don't follow this. The resulting type is memref<?x?xf32, ...>, which has two dynamic sizes and we pass two values as sizes.

[bondhugula]

This is deeper than I thought, in several ways. I have a pending fix for the ordering problem, and there were incorrect assumptions in other places.

Just FYI - my PR #253 fixes the loops.mlir file itself - it had been approved long ago (but not committed), and I discovered this bug when I tried to resolve conflicts there.

[ftynse]

Thanks! There are more tests that need updating to fix this though. Your PR is currently blocked on internal integration due to test breakage, I pinged the relevant person.

[bondhugula]

The number of 'sizes' is also one less!

I don't follow this. The resulting type is memref<?x?xf32, ...>, which has two dynamic sizes and we pass two values as sizes.

%A = view %arg0[%M, %K][%c0] : <-- only %c0 is being passed in as size.

[ftynse]

Ah ok, I see. My interpretation is that the order of []-blocks is swapped. [%M, %K] are sizes [%c0] is the offset.

[nicolasvasilache]

This was the old syntax, it was changed to be consistent with subview but the parser/verifier were not hardened and tests not updated. @Oleksandr Zinenko ftynse@gmail.com has a fix landing, thanks for reporting!

On Tue, Dec 3, 2019, 5:48 AM ftynse notifications@github.com wrote:

Ah ok, I see. My interpretation is that the order of []-blocks is swapped. [%M, %K] are sizes [%c0] is the offset.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tensorflow/mlir/issues/286?email_source=notifications&email_token=ACNNU5A45CJS3Y3PRC5SZ6TQWY2QRA5CNFSM4JUTSZ32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFY6GEY#issuecomment-561111827, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACNNU5GUQUA5LUJ42J7T2FLQWY2QRANCNFSM4JUTSZ3Q .

[ftynse]

Fixed in 721a07c