Page MenuHomePhabricator

[MLIR][Affine] Add custom builders for AffineVectorLoadOp/AffineVectorStoreOp
ClosedPublic

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).

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
2929

unsigned

3015

unsigned

mlir/lib/Dialect/Affine/Transforms/AffineParallelNormalize.cpp
22 ↗(On Diff #287125)

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 ↗(On Diff #287125)

It has users outside of this repository.

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

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
2929

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 ↗(On Diff #287125)

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

Hey @flaub! What happened to this patch? Do you plan to finish it and land it?

Thanks,
Diego

flaub added inline comments.Nov 18 2020, 4:18 PM
mlir/lib/Dialect/Affine/Transforms/AffineParallelNormalize.cpp
22 ↗(On Diff #287125)

Did this get updated in your recent commit @dcaballe? If so, is there anything blocking this diff from landing pretty much as is?

dcaballe added inline comments.Nov 18 2020, 4:27 PM
mlir/lib/Dialect/Affine/Transforms/AffineParallelNormalize.cpp
22 ↗(On Diff #287125)

Yeah, I think just a rebase would work. Not sure, though, if @bondhugula has any other comments.

flaub updated this revision to Diff 306327.Nov 19 2020, 12:35 AM
flaub edited the summary of this revision. (Show Details)

Addressing CR comments

flaub marked 3 inline comments as done.Nov 19 2020, 12:35 AM

@bondhugula Ready when you are :)

It's been exactly two months since I submitted a review!! - this was I think a simple patch. I've forgotten all context! @dcaballe - could you please review this as it's directly on top of your work? @flaub please go with @dcaballe's review.

dcaballe accepted this revision.Nov 23 2020, 11:39 AM

Thanks @bondhugula. Yeah, LGTM! It just needed a rebase. Thanks @flaub!

bondhugula accepted this revision.Nov 24 2020, 11:13 AM
This revision is now accepted and ready to land.Nov 24 2020, 11:13 AM