Page MenuHomePhabricator

[MLIR][Affine] Add custom builders for AffineVectorLoadOp/AffineVectorStoreOp
Needs RevisionPublic

Authored by flaub on Aug 21 2020, 3:31 PM.

Details

Summary

Adding missing custom builders for AffineVectorLoadOp & AffineVectorStoreOp. In practice, it is difficult to correctly construct these ops without these builders (because the AffineMap is not included at construction time).

Also including a fix to define normalizeAffineParallel in the mlir namespace.

Diff Detail

Event Timeline

flaub created this revision.Aug 21 2020, 3:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 21 2020, 3:31 PM
flaub requested review of this revision.Aug 21 2020, 3:31 PM
bondhugula accepted this revision.Aug 22 2020, 7:37 PM
bondhugula added inline comments.
mlir/lib/Dialect/Affine/IR/AffineOps.cpp
2819

unsigned

2905

unsigned

mlir/lib/Dialect/Affine/Transforms/AffineParallelNormalize.cpp
22

This doesn't have any users? If yes, how was this being linked earlier without being in the namespace?

This revision is now accepted and ready to land.Aug 22 2020, 7:37 PM
bondhugula requested changes to this revision.Aug 22 2020, 7:40 PM

Can you please separate out the fix to normalizeAffineParallel? It has really nothing to do with the builder additions.

This revision now requires changes to proceed.Aug 22 2020, 7:40 PM
flaub added inline comments.Aug 22 2020, 7:44 PM
mlir/lib/Dialect/Affine/Transforms/AffineParallelNormalize.cpp
22

It has users outside of this repository.

flaub added inline comments.Aug 22 2020, 7:45 PM
mlir/lib/Dialect/Affine/IR/AffineOps.cpp
2819

I can see that you really don't like auto :)

In general, this code was copy/pasted from existing MLIR code.

flaub added a comment.Aug 22 2020, 7:48 PM

Can you please separate out the fix to normalizeAffineParallel? It has really nothing to do with the builder additions.

Of course, I can. I was hoping to get this in quickly since downstream users are broken without it.

Can you please separate out the fix to normalizeAffineParallel? It has really nothing to do with the builder additions.

Of course, I can. I was hoping to get this in quickly since downstream users are broken without it.

It would have been quicker with it in its own patch. :)

Thanks Frank! Sorry, I was OOO last week. Quick comments for now, I'll come back later.

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
2819

I think in general the rule is to only use auto when it help readability. I personally *try* to only use it for cast/isa/dyn_cast, for other cases where the type is obvious or redundant and for cases where the type is too large (e.g., iterators), but I sometimes abuse it as well :)

mlir/lib/Dialect/Affine/Transforms/AffineParallelNormalize.cpp
22

These changes seem unrelated to changes on affine.vector_load and affine.vector_store. Please, create a independent differential for them.