This is an archive of the discontinued LLVM Phabricator instance.

[mlir][vector] Add unroll pattern for vector.gather
ClosedPublic

Authored by qedawkins on Apr 24 2023, 7:55 AM.

Details

Summary

This pattern is useful for SPIR-V to unroll to a supported vector size
before later lowerings. The unrolling pattern is closer to an
elementwise op than the transfer ops because the index values from which
to extract elements are captured by the index vector and thus there is
no need to update the base offsets when unrolling gather.

Diff Detail

Event Timeline

qedawkins created this revision.Apr 24 2023, 7:55 AM
Herald added a project: Restricted Project. · View Herald Transcript
qedawkins requested review of this revision.Apr 24 2023, 7:55 AM
kuhar added a comment.Apr 24 2023, 8:18 AM

Can we use this pattern to replace the one currently used in the vector gather lowering to vector.load/tensor.extract?

mlir/lib/Dialect/Vector/Transforms/VectorUnroll.cpp
653

nit: VectorType source...

668

nit: you can use to_vector<Value>(gatherOp.getIndices())

675

nit: ++i

Can we use this pattern to replace the one currently used in the vector gather lowering to vector.load/tensor.extract?

(I assume you're referring to FlattenGather in LowerVectorGather.cpp?) Yes, the pattern here is essentially a generalization of that one, with the exception that it is creating extract/insert_slice ops because we are no longer necessarily unrolling only the outer most dims. I think replacing the existing pattern makes sense as long as there is a way to recover the extract/insert ops currently produced.

kuhar added a comment.Apr 24 2023, 8:32 AM

Can we use this pattern to replace the one currently used in the vector gather lowering to vector.load/tensor.extract?

(I assume you're referring to FlattenGather in LowerVectorGather.cpp?) Yes, the pattern here is essentially a generalization of that one, with the exception that it is creating extract/insert_slice ops because we are no longer necessarily unrolling only the outer most dims. I think replacing the existing pattern makes sense as long as there is a way to recover the extract/insert ops currently produced.

Yes, exactly. I'd think we should be able to run some canon patterns to fold *_strided_slice into extract/insert.

Yes, exactly. I'd think we should be able to run some canon patterns to fold *_strided_slice into extract/insert.

Perfect, would it be preferred to replace that pattern now or is it fine to let it be a follow up patch?

kuhar accepted this revision.Apr 24 2023, 8:56 AM

Yes, exactly. I'd think we should be able to run some canon patterns to fold *_strided_slice into extract/insert.

Perfect, would it be preferred to replace that pattern now or is it fine to let it be a follow up patch?

Either sounds fine to me

This revision is now accepted and ready to land.Apr 24 2023, 8:56 AM
dcaballe accepted this revision.Apr 24 2023, 9:24 AM

Thanks!

mlir/lib/Dialect/Vector/Transforms/VectorUnroll.cpp
675

nit: move ub to variable

qedawkins updated this revision to Diff 516443.Apr 24 2023, 9:34 AM

Address comments

This revision was automatically updated to reflect the committed changes.