This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Add support for dynamic pad op masking vectorization.
ClosedPublic

Authored by hanchung on May 12 2023, 4:47 PM.

Diff Detail

Event Timeline

hanchung created this revision.May 12 2023, 4:47 PM
Herald added a project: Restricted Project. · View Herald Transcript
hanchung requested review of this revision.May 12 2023, 4:47 PM
hanchung updated this revision to Diff 521846.May 12 2023, 5:07 PM

clang-format

Thanks for working on this @hanchung ! Just a few minor comments inline.

mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
1330

Could you add a test to exercise this?

1473

[nit] I appreciate that this is unrelated to this change, but "Input vector sizes" is a bit misleading IMHO. "Requested vector sizes" or "Target vector sizes" would be more descriptive.

IIUC, inputVectorSizes is an input to vectorize, i.e. the vectorizer. It's just too easy to confuse this with with "input to the PadOp that's being analyzed here". Perhaps that's just me.

1475–1476

This isn't required for this change, is it? I don't mind, but if that's the case then could you add a note in the commit msg?

Also, I'd move this to the very top of this method (it's one of they key assumptions, isn't it?).

1483

[nit] Same note about "input" as above - perhaps "targert"?

Awesome! Just a few comments. LGTM overall

mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
1330

Adding some comments here would also help understanding.

I understand that this condition is only met for dynamic cases because the padOp semantics prevent the use of any other types that mismatch, right?

1472–1475

I think all this code is shared with vectorize. Is it possible to refactor it to a utility function?

Cool, thanks for extending to also support this case.

LGTM once comments are addressed.

hanchung updated this revision to Diff 523123.May 17 2023, 11:24 AM
hanchung marked 2 inline comments as done.

address comments

mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
1330

This is because the pad op is not a DPS operation, we don't have a destination tensor to do transfer_write. We should just create the empty op with given low, high and source shape.

1473

I agree with the point, it sounds really reasonable to me. There are many other places using input vector terms in this file, and I'd like to keep the consistency. This should be done in a separate patch, IMO.

1475–1476

I think this is required for all masking vectorization? My understanding is that we need the static value to create the mask. This is the same check in generic op version. I refactored the common logic to a util, please take a look.

awarzynski accepted this revision.May 17 2023, 12:18 PM

Thank you!

mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
1475–1476

I think this is required for all masking vectorization?

Yes, that was my point :) Your patch is about "dynamic pad op masking vectorization" and this change applies to everything. Like I said, I don't mind :)

This revision is now accepted and ready to land.May 17 2023, 12:18 PM
dcaballe accepted this revision.May 18 2023, 12:02 PM

LGTM, thanks! Please, wait for @awarzynski approval before landing.

mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
1363

doc

Oh, my review page needed a refresh :)

hanchung updated this revision to Diff 523530.May 18 2023, 1:12 PM

rebase to main

hanchung updated this revision to Diff 523535.May 18 2023, 1:26 PM

fixes for transform ops changes