[mlir][linalg] Exclude non-convolutional ops from isaConvolutionOpInterface #102087

yifeizh2 · 2024-08-06T01:27:46Z

Enhance isaConvolutionOpInterface logic.

Currently, isaConvolutionOpInterface returns false positive for linalg binary elementwise ops, because the function's underlying logic does not require the input linalg op to have convolved dims. We avoid such false positive by further checking the non-emptyness of convolved dims.

llvmbot · 2024-08-06T01:28:14Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-linalg

Author: None (yifeizh2)

Changes

Enhance convolution op judgement logic

Full diff: https://github.com/llvm/llvm-project/pull/102087.diff

1 Files Affected:

(modified) mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp (+9-1)

diff --git a/mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp b/mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
index 6ee1810c2ff2b..41143e0a5e347 100644
--- a/mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
+++ b/mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp
@@ -762,7 +762,8 @@ enum class MatchConvolutionResult {
   NotProjectedPermutations,
   NonConvolutionLoop,
   OutputDimsNotParallel,
-  NonOutputDimNotReduction
+  NonOutputDimNotReduction,
+  NoValidConvolvedDim
 };
 } // namespace mlir::linalg::detail
 
@@ -810,6 +811,8 @@ mlir::linalg::detail::isConvolutionInterfaceImpl(
   // - Depth multiplier : unconvolved in input, present in output, present in
   //   filter.
   llvm::SmallDenseSet<int64_t> allLoopDims;
+  bool hasOutputImageDim = false;
+  bool hasFilterLoopDim = false;
   for (auto outputExpr : indexingMaps.back().getResults()) {
     int64_t outputDim = cast<AffineDimExpr>(outputExpr).getPosition();
     if (inputExprWalker.unConvolvedDims.count(outputDim) &&
@@ -825,6 +828,7 @@ mlir::linalg::detail::isConvolutionInterfaceImpl(
       // Output image Loop dimension.
       if (iteratorTypes[outputDim] != utils::IteratorType::parallel)
         return MatchConvolutionResult::OutputDimsNotParallel;
+      hasOutputImageDim = true;
       allLoopDims.insert(outputDim);
       continue;
     }
@@ -862,6 +866,7 @@ mlir::linalg::detail::isConvolutionInterfaceImpl(
         return MatchConvolutionResult::NonOutputDimNotReduction;
       if (allLoopDims.count(filterDim))
         return MatchConvolutionResult::NonConvolutionLoop;
+      hasFilterLoopDim = true;
       allLoopDims.insert(filterDim);
       continue;
     }
@@ -886,6 +891,9 @@ mlir::linalg::detail::isConvolutionInterfaceImpl(
   if (allLoopDims.size() != linalgOp.getNumLoops())
     return MatchConvolutionResult::NonConvolutionLoop;
 
+  if (!hasOutputImageDim || !hasFilterLoopDim)
+    return MatchConvolutionResult::NoValidConvolvedDim;
+
   if (dimensions) {
     FailureOr<ConvolutionDimensions> res =
         inferConvolutionDimsImpl(linalgOp, inputExprWalker,

yifeizh2 · 2024-08-06T01:55:39Z

If we allow EmptyConvolvedDims, then linalg binary ops, such as linalg.add will also be considered as following the convolution op interface semantic.

yifeizh2 · 2024-08-06T02:41:28Z

If we allow EmptyConvolvedDims, then linalg binary ops, such as linalg.add will also be considered as following convolution op interface.

@qedawkins Sorry for bothering. I wonder why we allowed empty convolved dims in the initial design.

qedawkins · 2024-08-06T03:02:48Z

This has bothered me as well, especially because this helper is inconsistent with which named ops carry the convolution interface. I think it would be best to just expose allowEmptyConvolvedDims to users and default it to false.

yifeizh2 · 2024-08-06T06:00:40Z

This has bothered me as well, especially because this helper is inconsistent with which named ops carry the convolution interface. I think it would be best to just expose allowEmptyConvolvedDims to users and default it to false.

Hi. I updated the logic and hope to gain more suggestions. Currently the logic for allowEmptyConvolvedDims in isConvolutionInterfaceImpl is a duplicate of that in inferConvolutionDimsImpl, making the code not concise.

banach-space · 2024-08-06T11:46:21Z

Thanks for working on this! For those of us a bit less familiar with the logic being updated here ...

Fix isaConvolutionOpInterface logic

... what's broken and how does this PR fix it? :)

yifeizh2 · 2024-08-06T14:42:53Z

Thanks for working on this! For those of us a bit less familiar with the logic being updated here ...

Fix isaConvolutionOpInterface logic

... what's broken and how does this PR fix it? :)

Currently, the isaConvolutionOpInterface utility function returns false positive for all linalg binary elementwise ops (e.g. linalg.add, linalg.mul, ...). The purpose of this PR is to eliminate such false positives.

The isaConvolutionOpInterface analyzes the dimensions of input/filter/output and categorizes each dimension into one of the following: batch dimension/output image dimension/output channel dimension/filter loop dimension/input channel dimension/depth multiplier. The original logic only required that each dimension must belong to one of the five categories. The current PR further requires that there must be dims that truly get convolved.

qedawkins

Can you add the description you just gave to the PR description? Additionally please add a short explanation in the description for how downstream users can preserve the current behavior with the given API.

It is good to be clear about the motivation behind the change in the description: https://llvm.org/docs/DeveloperPolicy.html#commit-messages

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h

yifeizh2 · 2024-08-07T06:27:34Z

Can you add the description you just gave to the PR description? Additionally please add a short explanation in the description for how downstream users can preserve the current behavior with the given API.

It is good to be clear about the motivation behind the change in the description: https://llvm.org/docs/DeveloperPolicy.html#commit-messages

I provided a simplified explanation to both PR description section and commit message. Thanks!

qedawkins

Thanks, can you change the PR title to something more descriptive like

[mlir][linalg] Exclude non-convolutional ops from isaConvolutionOpInterface

like @banach-space was looking for?

banach-space · 2024-08-07T14:01:44Z

Thanks, can you change the PR title to something more descriptive like
[mlir][linalg] Exclude non-convolutional ops from isaConvolutionOpInterface
like @banach-space was looking for?

Thanks, that's exactly what I was after 🙏🏻

Some testing would be nice, but that would have to be a unit test and there's just none for Linalg atm :( I'm leaving that as a nice-to-have.

This change makes sense to :)

qedawkins · 2024-08-07T14:08:55Z

Some testing would be nice, but that would have to be a unit test and there's just none for Linalg atm :( I'm leaving that as a nice-to-have.

I was trying to think about what kind of test could be added, but this is pretty much just exposing an option that was already there. If we wanted to make it essentially NFC we could change the default back. We could also add a transform dialect op for isaConvolutionOpInterface, but I have the same thinking as you about nice-to-have.

yifeizh2 · 2024-08-08T09:53:58Z

Some testing would be nice, but that would have to be a unit test and there's just none for Linalg atm :( I'm leaving that as a nice-to-have.

I was trying to think about what kind of test could be added, but this is pretty much just exposing an option that was already there. If we wanted to make it essentially NFC we could change the default back. We could also add a transform dialect op for isaConvolutionOpInterface, but I have the same thinking as you about nice-to-have.

I had planned to add some unit tests for this logic change, but I failed to find existing unit tests for its counterparts (e.g. isaContractionOpInterface) as reference.

As for testing with transform dialect op, currently we have transform.match.structured.classify_convolution_dims, would you think it is better to add a new op such as transform.match.structured.convolution_op_interface which invokes isaConvolutionOpInterface?

qedawkins · 2024-08-08T14:51:53Z

I am not sure how useful such an operation would be given that transform.match.structured.classify_convolution_dims can already be used to do what transform.match.structured.convolution_op_interface would be able to do. It is up to you if you think such an operation would be useful. That also might be better as a separate PR.

yifeizh2 · 2024-08-09T05:38:57Z

I am not sure how useful such an operation would be given that transform.match.structured.classify_convolution_dims can already be used to do what transform.match.structured.convolution_op_interface would be able to do. It is up to you if you think such an operation would be useful. That also might be better as a separate PR.

I agree. The functionality of transform.match.structured.convolution_op_interface is a subset of transform.match.structured.classify_convolution_dims. Adding such an op does not have any practical use, which can only serve the purpose of adding unit test.

yifeizh2 · 2024-08-21T07:45:58Z

@qedawkins Since there is no more review comments, can I merge this PR by myself? Thanks.

qedawkins

Sounds good to me

Currently, `isaConvolutionOpInterface` returns false positive for linalg binary elementwise ops, because the function's underlying logic does not require the linalg op to have convolved dims. We avoid such false positive by further checking the non-emptyness of convolved dims.

kazutakahirata · 2024-08-26T16:29:15Z

This patch seems to cause

mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp:906:11: error: enumeration value 'EmptyConvolvedDims' not handled in switch [-Werror,-Wswitch]
  switch (res) {
          ^~~
1 error generated.

Would you mind taking a look? Thanks!

This patch fixes: mlir/lib/Dialect/Linalg/IR/LinalgInterfaces.cpp:906:11: error: enumeration value 'EmptyConvolvedDims' not handled in switch [-Werror,-Wswitch] with a workaround. I've notified the author of the new enum value in #102087.

kazutakahirata · 2024-08-26T17:13:20Z

I've submitted 2ef3dcf as a workaround. I'd appreciate if you could fix the message part of it. Thanks!

yifeizh2 · 2024-08-27T01:57:13Z

I've submitted 2ef3dcf as a workaround. I'd appreciate if you could fix the message part of it. Thanks!

Sorry for my negligence, and thanks for your providing the workaround for it. I will fix the message part soon!

yifeizh2 requested review from dcaballe, nicolasvasilache and rengolin as code owners August 6, 2024 01:27

llvmbot added mlir:linalg mlir labels Aug 6, 2024

qedawkins self-requested a review August 6, 2024 02:45

qedawkins requested changes Aug 6, 2024

View reviewed changes

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h Show resolved Hide resolved

mlir/include/mlir/Dialect/Linalg/IR/LinalgInterfaces.h Show resolved Hide resolved

yifeizh2 force-pushed the yifei/fix_isaconvolutionop branch from 5328b74 to b254151 Compare August 7, 2024 06:25

qedawkins approved these changes Aug 7, 2024

View reviewed changes

yifeizh2 changed the title ~~[mlir][linalg] Fix isaConvolutionOpInterface logic~~ [mlir][linalg] Exclude non-convolutional ops from isaConvolutionOpInterface Aug 8, 2024

qedawkins approved these changes Aug 21, 2024

View reviewed changes

yifeizh2 force-pushed the yifei/fix_isaconvolutionop branch from b254151 to 4b80eb7 Compare August 22, 2024 01:24

yifeizh2 merged commit ee737c3 into llvm:main Aug 26, 2024
8 checks passed

[mlir][linalg] Exclude non-convolutional ops from isaConvolutionOpInterface #102087

[mlir][linalg] Exclude non-convolutional ops from isaConvolutionOpInterface #102087

Uh oh!

Conversation

yifeizh2 commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yifeizh2 commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yifeizh2 commented Aug 6, 2024

Uh oh!

qedawkins commented Aug 6, 2024

Uh oh!

yifeizh2 commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

banach-space commented Aug 6, 2024

Uh oh!

yifeizh2 commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qedawkins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yifeizh2 commented Aug 7, 2024

Uh oh!

qedawkins left a comment

Choose a reason for hiding this comment

Uh oh!

banach-space commented Aug 7, 2024

Uh oh!

qedawkins commented Aug 7, 2024

Uh oh!

yifeizh2 commented Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qedawkins commented Aug 8, 2024

Uh oh!

yifeizh2 commented Aug 9, 2024

Uh oh!

yifeizh2 commented Aug 21, 2024

Uh oh!

qedawkins left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kazutakahirata commented Aug 26, 2024

Uh oh!

kazutakahirata commented Aug 26, 2024

Uh oh!

yifeizh2 commented Aug 27, 2024

Uh oh!

Uh oh!

yifeizh2 commented Aug 6, 2024 •

edited

Loading

llvmbot commented Aug 6, 2024 •

edited

Loading

yifeizh2 commented Aug 6, 2024 •

edited

Loading

yifeizh2 commented Aug 6, 2024 •

edited

Loading

yifeizh2 commented Aug 6, 2024 •

edited

Loading

yifeizh2 commented Aug 8, 2024 •

edited

Loading