ComposeSubView pattern does not check that strides are all 1 but assumes it

MLIR
1

Hello All,

I have been using MLIR for about 6 months now, I spotted a problem in the ComposeSubView.cpp file, essentially the code does not check that all strides in the subviews involved in the transformation are 1 but it assumes so, hence a bug.

// Offsets, sizes and strides OpFoldResult for the combined 'SubViewOp'.

    SmallVector<OpFoldResult> offsets, sizes, strides;

    // Because we only support input strides of 1, the output stride is also

    // always 1.

    // NOTE: strides is an empty small vector here, hence the test below will not
    // work as intended, the check for all strides being 1 is never performed, but it
    // is assumed later
    if (llvm::all_of(strides, [](OpFoldResult &valueOrAttr) {

          Attribute attr = valueOrAttr.dyn_cast<Attribute>();

          return attr && attr.cast<IntegerAttr>().getInt() == 1;

        })) {

      strides = SmallVector<OpFoldResult>(sourceOp.getMixedStrides().size(),

                                          rewriter.getI64IntegerAttr(1));

    } else {

      return failure();

    }
2

Yes, this looks like a clear bug. I think the intention was to check the strides of the sourceOp instead.

3

It is more than that. I believe the code assumes that both “op” and “sourceOp” need to have only ones as strides. Something like:

for (auto elm : sourceOp.getMixedStrides()) {

      strides.push_back(elm);

    }

    for (auto elm : op.getMixedStrides()) {

      strides.push_back(elm);

    }

In the spot of the note above should fix it. I am not sure if this is the cleanest fix though.

4

I am not sure I see the bug. It is testing that all strides are 1 as I read the code, but I might be missing something.

In any case, it is my experience that subviews with non-unit strides have very strange behavior. People who know more might have stronger opinions, but I think it is worth dropping strides from subviews to begin with. For most cases where I have to handle subviews, I check that the strides are all 1.