search

tensorflow / mlir

"Multi-Level Intermediate Representation" Compiler Infrastructure
1.74k stars 260 forks source link

Fix verification of zero-dim memref in affine.load/affine.store/std.load/std.store #58

Closed dcaballe closed 5 years ago

dcaballe commented 5 years ago

Verification complained when using zero-dimensional memrefs in affine.load, affine.store, std.load and std.store. Assuming that tests provided are expected use cases for affine and std loads and stores, this PR extends verification so that those memrefs can be used.

dcaballe commented 5 years ago

Thanks for the quick feedback, Alex! I'll address your comments after clarifying this point:

Do you have a specific use case where you want to use one subscript for a zero-dimensional memref? If not, I would think that using zero subscripts is more consistent, and also makes the verification code simpler (and can be reconsidered in future)

In fact, the use case we have is for one subscript. We are modeling single-scalar tensors as zero-dimensional tensors and lower them to zero-dimensional memrefs in affine. We want to create affine loads/stores for them but we need to provide a map for that, IIUC. We provide {} -> 0 map, as you can see here: https://github.com/tensorflow/mlir/pull/58/files#diff-3744a45b757312019c0aa9667cd244eeR1600, so affine loads/stores end up with one subscript. Maybe we could provide a {} -> {} map? Could we add an interface to Builder that does that?

ftynse commented 5 years ago

Maybe we could provide a {} -> {} map? Could we add an interface to Builder that does that?

I think this would be great to have. There may be places in the code that assume affine maps have non-zero outputs though.

dcaballe commented 5 years ago

Maybe we could provide a {} -> {} map? Could we add an interface to Builder that does that?

I think this would be great to have. There may be places in the code that assume affine maps have non-zero outputs though.

Yeah, I did hit some asserts when I tried. Let me see how difficult would be to make that work and we can make a decision. Thanks!

ftynse commented 5 years ago

LMK if there are too many changes involved. I'm fine with using constant zero subscript for zero-dimensional memrefs as a temporary solution. There are other places in the code that do that.

dcaballe commented 5 years ago

It was easier than expected. Last commit should address all the comments.

dcaballe commented 5 years ago

I think I addressed all the comments. Please, let me know if something else is missing. Thanks!

dcaballe commented 5 years ago

Good catch. Thanks!

River707 commented 5 years ago

Make sure to squash your commits.

dcaballe commented 5 years ago

For those doing the merge, there is an option to merge the changes doing a squash. That's the approach that I've seen in other projects. Are you asking me to do a explicit squash?

joker-eph commented 5 years ago

Our infra will squash anyway. The only advantage of the single commit is that we don’t have to reword the message ourselves on integration (otherwise it includes all the commits in the squashed commit message).

dcaballe commented 5 years ago

The problem with me doing the squash and a push force is that, IIRC, this messed up the review comments, but I don't remember the details. Yeah, just let me know. I can do a squash including my commits and the merge commits.

River707 commented 5 years ago

(Sorry, I was OOO for a while a missed the auto-squash update. Please disregard my comment then.)