This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Vectorization for conv_1d_ncw_fcw
ClosedPublic

Authored by raikonenfnu on Sep 11 2022, 11:35 AM.

Details

Summary

Most computer vision torch models uses nchw/ncw convolution. In a previous patch we added decomposition conv2dNchw to conv1dNcw. To enhance the performance on torch models we add this vectorization pattern for conv1dNcw which would consquently also improve the performance on conv2dNchw.

On IREE + Intel Xeon 8360 + Resnet50, we were able to get ~7x speed up ~880ms to 126ms.

Diff Detail

Event Timeline

raikonenfnu created this revision.Sep 11 2022, 11:35 AM
raikonenfnu requested review of this revision.Sep 11 2022, 11:35 AM
nicolasvasilache requested changes to this revision.Sep 13 2022, 8:00 AM

Please add a motivation in the commit message that describes why and more precisely what is done in this PR in plain English.
Ideally some perf numbers would back things up.

This revision now requires changes to proceed.Sep 13 2022, 8:00 AM
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
1393

Should this be an

enum Conv1DOpOrder {
Ncw, // This corresponds to a computation that traverses the input in order In(n, c, w)
Nwc  // This corresponds to a computation that traverses the input in order In(n, w, c)
};

etc .. ?

Note: I called this Conv1DOpOrder, it is definitely not a "data layout" but really an op definition property.

1449

Please use an enum and spell out the shapes instead of swapping.
Swapping has a level of indirection that makes things tricky to follow.

1454

You could say:

// The base vectorization case is input: nwc, weight: ..., output: ...
if (enum == Conv1DOpOrder::Nwc)
 ;
// To match the base vectorization case, we pre/post transpose the case we have.
if (enum == Conv1DOpOrder::Ncw) {
 // Convert input: ncw -> nwc.
 lhs = builder.create<vector::TransposeOp>(loc, lhs, ArrayRef<int64_t>{0, 2, 1}); 
... 
}

I find this makes the

raikonenfnu edited the summary of this revision. (Show Details)

-Updated commit message for motivation
-Added enum for better legibility

raikonenfnu edited the summary of this revision. (Show Details)

updated commit msg

raikonenfnu added inline comments.Sep 13 2022, 12:53 PM
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
1454

Thanks for the great comments and suggestions! I have addressed most of these, please let me know if I am miss anything :)

Updated Conv1DGenerator comment to have conv1dNcw details.

hanchung requested changes to this revision.Sep 13 2022, 1:52 PM
hanchung added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
95–100

let's use enum class here, and we can use switch-case in conv.

https://abseil.io/tips/86

1439–1442

nit: auto

VectorType::get already spells the type.

1524

It's better to use switch-case. With enum class, we won't miss updating the snippet when adding a new order.

1527–1528

nit: permRes, maybe just rename it to perm.

This revision now requires changes to proceed.Sep 13 2022, 1:52 PM
hanchung added inline comments.Sep 13 2022, 1:56 PM
mlir/test/Dialect/Linalg/vectorize-convolution.mlir
257–258

remove the empty test?

Using switch-case instead of if-statments

raikonenfnu added inline comments.Sep 13 2022, 2:41 PM
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
95–100

great idea, done, thanks! :)

mlir/test/Dialect/Linalg/vectorize-convolution.mlir
257–258

good catch, done!

hanchung requested changes to this revision.Sep 13 2022, 4:33 PM
hanchung added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
95–100

We should use enum class Conv1DOpOrder, not enum Conv1DOpOrder.

This revision now requires changes to proceed.Sep 13 2022, 4:33 PM

Using enum class

raikonenfnu marked an inline comment as done.Sep 13 2022, 6:34 PM
raikonenfnu added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
95–100

done! :)

nicolasvasilache accepted this revision.Sep 14 2022, 9:10 AM

LG!

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

can we rephrase to avoid newlines?

raikonenfnu marked an inline comment as done.

Repharesed struct info to fit in a line

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

Done!

hanchung accepted this revision.Sep 14 2022, 11:06 AM

LGTM, nice work!

This revision is now accepted and ready to land.Sep 14 2022, 11:06 AM
This revision was landed with ongoing or failed builds.Sep 14 2022, 11:08 AM
This revision was automatically updated to reflect the committed changes.

Thanks for letting me know! pushed up a fix here https://reviews.llvm.org/D133889

Thanks for letting me know! pushed up a fix here https://reviews.llvm.org/D133889

Thanks!