[Mlir-commits] [mlir] [MLIR] Allowing unsupported conv2d op to fail gracefully from vectorization (PR #130181)

[Andrzej Warzyński]

================
@@ -3929,9 +3957,11 @@ static FailureOr<Operation *> vectorizeConvolution(
   if (!inputVecSizes.empty()) {
     // Only use the input vector size corresponding to the channel dim. Other
     // vector dims will be inferred from the Ops.
-    assert((isa<linalg::DepthwiseConv1DNwcWcOp>(*op) ||
-            isa<linalg::DepthwiseConv1DNcwCwOp>(*op)) &&
-           "Not a 1D depthwise conv!");
----------------
banach-space wrote:

> I just don't think assertion is the best error handling mechanism: when unexpected op come in, just fail to vectorize from the function.

Totally agree. The `assert` is merely documenting the assumption being made. If we get to this point and the input Op is neither `DepthwiseConv1DNwcWcOp` nor `DepthwiseConv1DNcwCwOp`, then that implies that some logic earlier on is missing. Put differently, we just shouldn't get here if processing something that is not supported. Importantly, you wouldn't be helping us to improve this code if you didn't hit that assert 😅 

Btw, can you think of an example that would trigger this `assert`? If yes, then we can work on improving our logic elsewhere. If not, next time we hit this assert we will have an example to guide us what additional checks are missing.

Just to clarify, I don't want this code to be a nuisance to anyone and really appreciate all the tidying up that you are doing. The `assert`s, IMHO, are to clarify the assumptions and to prompt us to write better error recovery whnever that's missing.

https://github.com/llvm/llvm-project/pull/130181