This is an archive of the discontinued LLVM Phabricator instance.

[mlir][vector] Use DenseI64ArrayAttr for ExtractOp/InsertOp positions
ClosedPublic

Authored by springerm on Jul 31 2023, 5:49 AM.

Details

Summary

DenseI64ArrayAttr provides a better API than I64ArrayAttr. E.g., accessors returning ArrayRef<int64_t> (instead of ArrayAttr) are generated.

Diff Detail

Event Timeline

springerm created this revision.Jul 31 2023, 5:49 AM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a reviewer: kuhar. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
springerm requested review of this revision.Jul 31 2023, 5:49 AM
Herald added a project: Restricted Project. · View Herald Transcript
nicolasvasilache accepted this revision.Jul 31 2023, 6:03 AM

Great, thanks much for the cleanup!

This revision is now accepted and ready to land.Jul 31 2023, 6:03 AM
This revision was landed with ongoing or failed builds.Jul 31 2023, 6:29 AM
This revision was automatically updated to reflect the committed changes.

I'm currently working on extending ExtractOp/InsertOp to support Value indices. I should be able to have something this week. Is this change blocking you? If not, it would help if we can revert it and wait for my change so that I don't have to deal with all the conflicts, as I'm touching the exact same lines and more. Do you think that would be feasible?

Can you explain a bit how you are planning to change these ops? I think it was on purpose that these ops only support static indices. How does this lower to LLVM? There are comments such as this, which sounds like it may not be possible to lower such ops to LLVM without bigger changes:

In VectorToSCF.h
[...]
/// This is consistent with the lack of an LLVM instruction to dynamically
/// index into an aggregate (see the Vector dialect lowering to LLVM deep dive).

(Also see https://mlir.llvm.org/docs/Dialects/Vector/#deeperdive.)

Also, how are vectors different from tensors after this change? At the moment, one main difference is that tensors can be indexed dynamically (vector cannot).

If we actually need Value indices, can we make these mixed static/dynamic? Like tensor.extract_slice, tensor.insert_slice, memref.subview, etc. These accept a mix of SSA values and int64_t. This composes better with the remaining OpFoldResult/getMixedSizes/OpBuilder::createOrFold/affine::makeComposedFoldedAffineApply/... APIs that we have throughout MLIR. If we have only Value indices, we have to create many arith.constant ops. E.g., this is happening at moment when unrolling n-D vector transfers with VectorToSCF because many vector ops do not yet support "mixed" indices.

We can add a new interface called MixedIndicesOpInterface to ViewLikeInterfaces.td that is like OffsetSizeAndStrideOpInterface but has only one offsets/indices, but no sizes or strides. This change here would actually be the first step towards that direction, because the static offsets should be a DenseI64ArrayAttr.

Can you explain a bit how you are planning to change these ops?

The following links should give you enough context about the motivation:

https://discourse.llvm.org/t/rfc-psa-remove-vector-extractelement-and-vector-insertelement-ops-in-favor-of-vector-extract-and-vector-insert-ops/71116/5
https://reviews.llvm.org/D155034

I think it was on purpose that these ops only support static indices. How does this lower to LLVM? There are comments such as this, which sounds like it may not be possible to lower such ops to LLVM without bigger changes:

LLVM shouldn't be a limitation for what we can model at MLIR level, esp. now that we have "real" 2D vector operations in MLIR, such as SME.

Also, how are vectors different from tensors after this change? At the moment, one main difference is that tensors can be indexed dynamically (vector cannot).

We can insert/extract elements from dynamic positions even in LLVM, right? https://llvm.org/docs/LangRef.html#extractvalue-instruction

If we actually need Value indices, can we make these mixed static/dynamic? Like tensor.extract_slice, tensor.insert_slice, memref.subview, etc. These accept a mix of SSA values and int64_t. This composes better with the remaining OpFoldResult/getMixedSizes/OpBuilder::createOrFold/affine::makeComposedFoldedAffineApply/... APIs that we have throughout MLIR. If we have only Value indices, we have to create many arith.constant ops. E.g., this is happening at moment when unrolling n-D vector transfers with VectorToSCF because many vector ops do not yet support "mixed" indices.

We can add a new interface called MixedIndicesOpInterface to ViewLikeInterfaces.td that is like OffsetSizeAndStrideOpInterface but has only one offsets/indices, but no sizes or strides. This change here would actually be the first step towards that direction, because the static offsets should be a DenseI64ArrayAttr.

Yeah, using mixed static/dynamic indices was pointed out in the review and I have all of that implemented locally. I don't have the interface refactoring, though, which is something we can introduce after the fact. However, if you think you have cycles to help with the interface refactoring I would be more than happy to refactor what I have on top of that :).

Hopefully it makes more sense now!

We can insert/extract elements from dynamic positions even in LLVM, right? https://llvm.org/docs/LangRef.html#extractvalue-instruction

This instruction won't work for LLVM vectors:

The ‘extractvalue’ instruction extracts the value of a member field from an aggregate value.
[...]
Aggregate Types are a subset of derived types that can contain multiple member types. Arrays and structs are aggregate types. Vectors are not considered to be aggregate types.

Also, the indices must be static:

The other operands are constant indices to specify which value to extract in a similar manner as indices in a ‘getelementptr’ instruction.

In D155034 (mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVM.cpp), you are currently using convertConstantsToInts, so there is an implicit assumption that all indices are static when lowering to LLVM.

Yeah, using mixed static/dynamic indices was pointed out in the review and I have all of that implemented locally. I don't have the interface refactoring, though, which is something we can introduce after the fact. However, if you think you have cycles to help with the interface refactoring I would be more than happy to refactor what I have on top of that :).

I can add that tomorrow.

Sorry, I sent you the wrong link. This is it: https://llvm.org/docs/LangRef.html#extractelement-instruction
From which I wanted to point to: The second operand is an index indicating the position from which to extract the element. The index may be a variable of any integer type, and will be treated as an unsigned integer.

D155034 is not up-to-date. I have a version locally that works with variable indices.

The LLVM lowering expects constant indices, which is fine. We will reject those ops without constant indices when it comes to lowering to LLVM but we will have a single op to represent vector extraction regardless of the number of vector dimensions. We currently have duplicated patterns in MLIR, for vector.extract and for vector.extractelement, which is a pain.

Thanks for helping with this!