[mlir][vector] Update the internal representation of in_bounds #100336
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
banach-space
changed the title
banach-space
force-pushed
the
andrzej/update_in_bounds
branch
from
467ec19 to
b6150be
Compare
|
Seems fine to me, I'm not sure if a custom printer would be better here? |
banach-space
force-pushed
the
andrzej/update_in_bounds
branch
2 times, most recently
from
2da6ed1 to
13f2559
Compare
banach-space
requested review from
grypp,
dcaballe,
hanhanW,
nicolasvasilache,
kuhar and
matthias-springer
as code owners
llvmbot
added
mlir:linalg
mlir:gpu
mlir:affine
mlir:vectorops
mlir:sparse
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir-affine Author: Andrzej Warzyński (banach-space) ChangesThis PR updates the internal representation of the Note that this means that the asm format of the vector.transfer_read %arg0[%0, %1], %cst {in_bounds = [true], permutation_map = #map3} : memref<12x16xf32>, vector<8xf32>to: vector.transfer_read %arg0[%0, %1], %cst {in_bounds = array<i1: true>, permutation_map = #map3} : memref<12x16xf32>, vector<8xf32>DEPENDS ON: #100334 (ignore the first commit) Patch is 581.23 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/100336.diff 79 Files Affected:
diff --git a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
index 434ff3956c250..bc80e8e995571 100644
--- a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
+++ b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
@@ -1362,7 +1362,7 @@ def Vector_TransferReadOp :
AffineMapAttr:$permutation_map,
AnyType:$padding,
Optional<VectorOf<[I1]>>:$mask,
- BoolArrayAttr:$in_bounds)>,
+ DenseBoolArrayAttr:$in_bounds)>,
Results<(outs AnyVectorOfAnyRank:$vector)> {
let summary = "Reads a supervector from memory into an SSA vector value.";
@@ -1557,7 +1557,7 @@ def Vector_TransferReadOp :
"Value":$source,
"ValueRange":$indices,
"AffineMapAttr":$permutationMapAttr,
- "ArrayAttr":$inBoundsAttr)>,
+ "DenseBoolArrayAttr":$inBoundsAttr)>,
/// 2. Builder that sets padding to zero and an empty mask (variant without attrs).
OpBuilder<(ins "VectorType":$vectorType,
"Value":$source,
@@ -1609,7 +1609,7 @@ def Vector_TransferWriteOp :
Variadic<Index>:$indices,
AffineMapAttr:$permutation_map,
Optional<VectorOf<[I1]>>:$mask,
- BoolArrayAttr:$in_bounds)>,
+ DenseBoolArrayAttr:$in_bounds)>,
Results<(outs Optional<AnyRankedTensor>:$result)> {
let summary = "The vector.transfer_write op writes a supervector to memory.";
@@ -1720,13 +1720,13 @@ def Vector_TransferWriteOp :
"ValueRange":$indices,
"AffineMapAttr":$permutationMapAttr,
"Value":$mask,
- "ArrayAttr":$inBoundsAttr)>,
+ "DenseBoolArrayAttr":$inBoundsAttr)>,
/// 2. Builder with type inference that sets an empty mask (variant with attrs).
OpBuilder<(ins "Value":$vector,
"Value":$dest,
"ValueRange":$indices,
"AffineMapAttr":$permutationMapAttr,
- "ArrayAttr":$inBoundsAttr)>,
+ "DenseBoolArrayAttr":$inBoundsAttr)>,
/// 3. Builder with type inference that sets an empty mask (variant without attrs).
OpBuilder<(ins "Value":$vector,
"Value":$dest,
diff --git a/mlir/include/mlir/Interfaces/VectorInterfaces.td b/mlir/include/mlir/Interfaces/VectorInterfaces.td
index 7ea62c2ae2ab1..b2a381b451008 100644
--- a/mlir/include/mlir/Interfaces/VectorInterfaces.td
+++ b/mlir/include/mlir/Interfaces/VectorInterfaces.td
@@ -98,7 +98,7 @@ def VectorTransferOpInterface : OpInterface<"VectorTransferOpInterface"> {
dimension whether it is in-bounds or not. (Broadcast dimensions are
always in-bounds).
}],
- /*retTy=*/"::mlir::ArrayAttr",
+ /*retTy=*/"::mlir::ArrayRef<bool>",
/*methodName=*/"getInBounds",
/*args=*/(ins)
>,
@@ -241,7 +241,7 @@ def VectorTransferOpInterface : OpInterface<"VectorTransferOpInterface"> {
if ($_op.isBroadcastDim(dim))
return true;
auto inBounds = $_op.getInBounds();
- return ::llvm::cast<::mlir::BoolAttr>(inBounds[dim]).getValue();
+ return inBounds[dim];
}
/// Helper function to account for the fact that `permutationMap` results
diff --git a/mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp b/mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp
index 19f02297bfbb7..01fb63ddba610 100644
--- a/mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp
+++ b/mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp
@@ -242,10 +242,11 @@ static void generateInBoundsCheck(
}
/// Given an ArrayAttr, return a copy where the first element is dropped.
-static ArrayAttr dropFirstElem(OpBuilder &b, ArrayAttr attr) {
+static DenseBoolArrayAttr dropFirstElem(OpBuilder &b, DenseBoolArrayAttr attr) {
if (!attr)
return attr;
- return ArrayAttr::get(b.getContext(), attr.getValue().drop_front());
+ return DenseBoolArrayAttr::get(b.getContext(),
+ attr.asArrayRef().drop_front());
}
/// Add the pass label to a vector transfer op if its rank is not the target
diff --git a/mlir/lib/Dialect/ArmSME/Transforms/VectorLegalization.cpp b/mlir/lib/Dialect/ArmSME/Transforms/VectorLegalization.cpp
index 53df7af00aee8..52351807ffd19 100644
--- a/mlir/lib/Dialect/ArmSME/Transforms/VectorLegalization.cpp
+++ b/mlir/lib/Dialect/ArmSME/Transforms/VectorLegalization.cpp
@@ -497,8 +497,7 @@ struct LegalizeMultiTileTransferWriteAsStoreLoop
loc, slice, writeOp.getSource(), ValueRange{storeRow, storeCol},
AffineMapAttr::get(writeOp.getPermutationMap().dropResult(0)),
sliceMask,
- rewriter.getBoolArrayAttr(
- ArrayRef<bool>(writeOp.getInBoundsValues()).drop_front()));
+ rewriter.getDenseBoolArrayAttr(writeOp.getInBounds().drop_front()));
}
rewriter.eraseOp(writeOp);
@@ -691,13 +690,12 @@ struct LiftIllegalVectorTransposeToMemory
transposeOp.getPermutation(), getContext());
auto transposedSubview = rewriter.create<memref::TransposeOp>(
loc, readSubview, AffineMapAttr::get(transposeMap));
- ArrayAttr inBoundsAttr = illegalRead.getInBoundsAttr();
+ DenseBoolArrayAttr inBoundsAttr = illegalRead.getInBoundsAttr();
// - The `in_bounds` attribute
if (inBoundsAttr) {
- SmallVector<Attribute> inBoundsValues(inBoundsAttr.begin(),
- inBoundsAttr.end());
+ SmallVector<bool> inBoundsValues(inBoundsAttr.asArrayRef());
applyPermutationToVector(inBoundsValues, transposeOp.getPermutation());
- inBoundsAttr = rewriter.getArrayAttr(inBoundsValues);
+ inBoundsAttr = rewriter.getDenseBoolArrayAttr(inBoundsValues);
}
VectorType legalReadType = resultType.clone(readType.getElementType());
@@ -990,7 +988,7 @@ struct LowerIllegalTransposeStoreViaZA
rewriter.create<arith::AddIOp>(loc, transposedCol, writeIndices[1]);
auto smeWrite = rewriter.create<vector::TransferWriteOp>(
loc, tile, destTensorOrMemref, ValueRange{destRow, destCol},
- transposeMap, subMask, writeOp.getInBounds());
+ transposeMap, subMask, writeOp.getInBoundsAttr());
if (writeOp.hasPureTensorSemantics())
destTensorOrMemref = smeWrite.getResult();
diff --git a/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp b/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
index 3d0d6abf702d7..d33f2d01676a1 100644
--- a/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
+++ b/mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
@@ -646,7 +646,7 @@ static Value buildVectorWrite(RewriterBase &rewriter, Value value,
if (auto maskOp = dyn_cast<vector::MaskingOpInterface>(write)) {
auto maskedWriteOp = cast<vector::TransferWriteOp>(maskOp.getMaskableOp());
SmallVector<bool> inBounds(maskedWriteOp.getVectorType().getRank(), true);
- maskedWriteOp.setInBoundsAttr(rewriter.getBoolArrayAttr(inBounds));
+ maskedWriteOp.setInBoundsAttr(rewriter.getDenseBoolArrayAttr(inBounds));
}
LDBG("vectorized op: " << *write << "\n");
@@ -1373,7 +1373,7 @@ vectorizeAsLinalgGeneric(RewriterBase &rewriter, VectorizationState &state,
if (auto maskOp = dyn_cast<vector::MaskingOpInterface>(read)) {
SmallVector<bool> inBounds(readType.getRank(), true);
cast<vector::TransferReadOp>(maskOp.getMaskableOp())
- .setInBoundsAttr(rewriter.getBoolArrayAttr(inBounds));
+ .setInBoundsAttr(rewriter.getDenseBoolArrayAttr(inBounds));
}
// 3.c. Not all ops support 0-d vectors, extract the scalar for now.
@@ -2406,7 +2406,7 @@ struct PadOpVectorizationWithTransferReadPattern
rewriter.modifyOpInPlace(xferOp, [&]() {
SmallVector<bool> inBounds(xferOp.getVectorType().getRank(), false);
xferOp->setAttr(xferOp.getInBoundsAttrName(),
- rewriter.getBoolArrayAttr(inBounds));
+ rewriter.getDenseBoolArrayAttr(inBounds));
xferOp.getSourceMutable().assign(padOp.getSource());
xferOp.getPaddingMutable().assign(padValue);
});
@@ -2485,7 +2485,7 @@ struct PadOpVectorizationWithTransferWritePattern
auto newXferOp = rewriter.replaceOpWithNewOp<vector::TransferWriteOp>(
xferOp, padOp.getSource().getType(), xferOp.getVector(),
padOp.getSource(), xferOp.getIndices(), xferOp.getPermutationMapAttr(),
- xferOp.getMask(), rewriter.getBoolArrayAttr(inBounds));
+ xferOp.getMask(), rewriter.getDenseBoolArrayAttr(inBounds));
rewriter.replaceOp(trimPadding, newXferOp->getResult(0));
return success();
@@ -2789,7 +2789,7 @@ LogicalResult LinalgCopyVTRForwardingPattern::matchAndRewrite(
Value res = rewriter.create<vector::TransferReadOp>(
xferOp.getLoc(), vectorType, in, xferOp.getIndices(),
xferOp.getPermutationMapAttr(), xferOp.getPadding(), xferOp.getMask(),
- rewriter.getBoolArrayAttr(
+ rewriter.getDenseBoolArrayAttr(
SmallVector<bool>(vectorType.getRank(), false)));
if (maybeFillOp)
@@ -2848,7 +2848,7 @@ LogicalResult LinalgCopyVTWForwardingPattern::matchAndRewrite(
rewriter.create<vector::TransferWriteOp>(
xferOp.getLoc(), vector, out, xferOp.getIndices(),
xferOp.getPermutationMapAttr(), xferOp.getMask(),
- rewriter.getBoolArrayAttr(
+ rewriter.getDenseBoolArrayAttr(
SmallVector<bool>(vector.getType().getRank(), false)));
rewriter.eraseOp(copyOp);
@@ -3348,7 +3348,7 @@ struct Conv1DGenerator
SmallVector<bool> inBounds(maskShape.size(), true);
auto xferOp = cast<VectorTransferOpInterface>(opToMask);
xferOp->setAttr(xferOp.getInBoundsAttrName(),
- rewriter.getBoolArrayAttr(inBounds));
+ rewriter.getDenseBoolArrayAttr(inBounds));
SmallVector<OpFoldResult> mixedDims = vector::getMixedSizesXfer(
cast<LinalgOp>(op).hasPureTensorSemantics(), opToMask, rewriter);
diff --git a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
index 5047bd925d4c5..884da78e0456e 100644
--- a/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
+++ b/mlir/lib/Dialect/Vector/IR/VectorOps.cpp
@@ -3792,7 +3792,7 @@ void ExtractStridedSliceOp::getCanonicalizationPatterns(
void TransferReadOp::build(OpBuilder &builder, OperationState &result,
VectorType vectorType, Value source,
ValueRange indices, AffineMapAttr permutationMapAttr,
- /*optional*/ ArrayAttr inBoundsAttr) {
+ /*optional*/ DenseBoolArrayAttr inBoundsAttr) {
Type elemType = llvm::cast<ShapedType>(source.getType()).getElementType();
Value padding = builder.create<arith::ConstantOp>(
result.location, elemType, builder.getZeroAttr(elemType));
@@ -3807,8 +3807,8 @@ void TransferReadOp::build(OpBuilder &builder, OperationState &result,
std::optional<ArrayRef<bool>> inBounds) {
auto permutationMapAttr = AffineMapAttr::get(permutationMap);
auto inBoundsAttr = (inBounds && !inBounds.value().empty())
- ? builder.getBoolArrayAttr(inBounds.value())
- : builder.getBoolArrayAttr(
+ ? builder.getDenseBoolArrayAttr(inBounds.value())
+ : builder.getDenseBoolArrayAttr(
SmallVector<bool>(vectorType.getRank(), false));
build(builder, result, vectorType, source, indices, permutationMapAttr,
inBoundsAttr);
@@ -3823,8 +3823,8 @@ void TransferReadOp::build(OpBuilder &builder, OperationState &result,
llvm::cast<ShapedType>(source.getType()), vectorType);
auto permutationMapAttr = AffineMapAttr::get(permutationMap);
auto inBoundsAttr = (inBounds && !inBounds.value().empty())
- ? builder.getBoolArrayAttr(inBounds.value())
- : builder.getBoolArrayAttr(
+ ? builder.getDenseBoolArrayAttr(inBounds.value())
+ : builder.getDenseBoolArrayAttr(
SmallVector<bool>(vectorType.getRank(), false));
build(builder, result, vectorType, source, indices, permutationMapAttr,
padding,
@@ -3876,7 +3876,7 @@ static LogicalResult
verifyTransferOp(VectorTransferOpInterface op, ShapedType shapedType,
VectorType vectorType, VectorType maskType,
VectorType inferredMaskType, AffineMap permutationMap,
- ArrayAttr inBounds) {
+ ArrayRef<bool> inBounds) {
if (op->hasAttr("masked")) {
return op->emitOpError("masked attribute has been removed. "
"Use in_bounds instead.");
@@ -3949,8 +3949,7 @@ verifyTransferOp(VectorTransferOpInterface op, ShapedType shapedType,
<< AffineMapAttr::get(permutationMap)
<< " vs inBounds of size: " << inBounds.size();
for (unsigned int i = 0, e = permutationMap.getNumResults(); i < e; ++i)
- if (isa<AffineConstantExpr>(permutationMap.getResult(i)) &&
- !llvm::cast<BoolAttr>(inBounds.getValue()[i]).getValue())
+ if (isa<AffineConstantExpr>(permutationMap.getResult(i)) && !inBounds[i])
return op->emitOpError("requires broadcast dimensions to be in-bounds");
return success();
@@ -4031,7 +4030,7 @@ ParseResult TransferReadOp::parse(OpAsmParser &parser, OperationState &result) {
Attribute inBoundsAttr = result.attributes.get(inBoundsAttrName);
if (!inBoundsAttr) {
result.addAttribute(inBoundsAttrName,
- builder.getBoolArrayAttr(
+ builder.getDenseBoolArrayAttr(
SmallVector<bool>(permMap.getNumResults(), false)));
}
if (parser.resolveOperand(sourceInfo, shapedType, result.operands) ||
@@ -4159,7 +4158,7 @@ static LogicalResult foldTransferInBoundsAttribute(TransferOp op) {
return failure();
// OpBuilder is only used as a helper to build an I64ArrayAttr.
OpBuilder b(op.getContext());
- op.setInBoundsAttr(b.getBoolArrayAttr(newInBounds));
+ op.setInBoundsAttr(b.getDenseBoolArrayAttr(newInBounds));
return success();
}
@@ -4329,7 +4328,7 @@ void TransferWriteOp::build(OpBuilder &builder, OperationState &result,
Value vector, Value dest, ValueRange indices,
AffineMapAttr permutationMapAttr,
/*optional*/ Value mask,
- /*optional*/ ArrayAttr inBoundsAttr) {
+ /*optional*/ DenseBoolArrayAttr inBoundsAttr) {
Type resultType = llvm::dyn_cast<RankedTensorType>(dest.getType());
build(builder, result, resultType, vector, dest, indices, permutationMapAttr,
mask, inBoundsAttr);
@@ -4339,7 +4338,7 @@ void TransferWriteOp::build(OpBuilder &builder, OperationState &result,
void TransferWriteOp::build(OpBuilder &builder, OperationState &result,
Value vector, Value dest, ValueRange indices,
AffineMapAttr permutationMapAttr,
- /*optional*/ ArrayAttr inBoundsAttr) {
+ /*optional*/ DenseBoolArrayAttr inBoundsAttr) {
build(builder, result, vector, dest, indices, permutationMapAttr,
/*mask=*/Value(), inBoundsAttr);
}
@@ -4353,8 +4352,8 @@ void TransferWriteOp::build(OpBuilder &builder, OperationState &result,
auto permutationMapAttr = AffineMapAttr::get(permutationMap);
auto inBoundsAttr =
(inBounds && !inBounds.value().empty())
- ? builder.getBoolArrayAttr(inBounds.value())
- : builder.getBoolArrayAttr(SmallVector<bool>(
+ ? builder.getDenseBoolArrayAttr(inBounds.value())
+ : builder.getDenseBoolArrayAttr(SmallVector<bool>(
llvm::cast<VectorType>(vector.getType()).getRank(), false));
build(builder, result, vector, dest, indices, permutationMapAttr,
/*mask=*/Value(), inBoundsAttr);
@@ -4412,7 +4411,7 @@ ParseResult TransferWriteOp::parse(OpAsmParser &parser,
Attribute inBoundsAttr = result.attributes.get(inBoundsAttrName);
if (!inBoundsAttr) {
result.addAttribute(inBoundsAttrName,
- builder.getBoolArrayAttr(
+ builder.getDenseBoolArrayAttr(
SmallVector<bool>(permMap.getNumResults(), false)));
}
if (parser.resolveOperand(vectorInfo, vectorType, result.operands) ||
@@ -4765,7 +4764,7 @@ struct SwapExtractSliceOfTransferWrite
auto newTransferWriteOp = rewriter.create<TransferWriteOp>(
transferOp.getLoc(), transferOp.getVector(), newExtractOp.getResult(),
transferOp.getIndices(), transferOp.getPermutationMapAttr(),
- rewriter.getBoolArrayAttr(newInBounds));
+ rewriter.getDenseBoolArrayAttr(newInBounds));
rewriter.modifyOpInPlace(insertOp, [&]() {
insertOp.getSourceMutable().assign(newTransferWriteOp.getResult());
});
diff --git a/mlir/lib/Dialect/Vector/Transforms/LowerVectorTransfer.cpp b/mlir/lib/Dialect/Vector/Transforms/LowerVectorTransfer.cpp
index b3c6dec47f6be..321600c14dcfb 100644
--- a/mlir/lib/Dialect/Vector/Transforms/LowerVectorTransfer.cpp
+++ b/mlir/lib/Dialect/Vector/Transforms/LowerVectorTransfer.cpp
@@ -22,15 +22,14 @@ using namespace mlir::vector;
/// Transpose a vector transfer op's `in_bounds` attribute by applying reverse
/// permutation based on the given indices.
-static ArrayAttr
-inverseTransposeInBoundsAttr(OpBuilder &builder, ArrayAttr attr,
+static DenseBoolArrayAttr
+inverseTransposeInBoundsAttr(OpBuilder &builder, ArrayRef<bool> inBounds,
const SmallVector<unsigned> &permutation) {
SmallVector<bool> newInBoundsValues(permutation.size());
size_t index = 0;
for (unsigned pos : permutation)
- newInBoundsValues[pos] =
- cast<BoolAttr>(attr.getValue()[index++]).getValue();
- return builder.getBoolArrayAttr(newInBoundsValues);
+ newInBoundsValues[pos] = inBounds[index++];
+ return builder.getDenseBoolArrayAttr(newInBoundsValues);
}
/// Extend the rank of a vector Value by `addedRanks` by adding outer unit
@@ -132,7 +131,7 @@ struct TransferReadPermutationLowering
}
// Transpose in_bounds attribute.
- ArrayAttr newInBoundsAttr =
+ DenseBoolArrayAttr newInBoundsAttr =
inverseTransposeInBoundsAttr(rewriter, op.getInBounds(), permutation);
// Generate new transfer_read operation.
@@ -205,7 +204,7 @@ struct TransferWritePermutationLowering
});
// Transpose in_bounds attribute.
- ArrayAttr newInBoundsAttr =
+ DenseBoolArrayAttr newInBoundsAttr =
inverseTransposeInBoundsAttr(rewriter, op.getInBounds(), permutation);
// Generate new transfer_write operation.
@@ -298,7 +297,8 @@ struct TransferWriteNonPermutationLowering
for (int64_t i = 0, e = op.getVectorType().getRank(); i < e; ++i) {
newInBoundsValues.push_back(op.isDimInBounds(i));
}
- ArrayAttr newInBoundsAttr = rewriter.getBoolArrayAttr(newInBoundsValues);
+ DenseBoolArrayAttr newInBoundsAttr =
+ rewriter.getDenseBoolArrayAttr(newInBoundsValues);
auto newWrite = rewriter.create<vector::TransferWriteOp>(
op.getLoc(), newVec, op.getSource(), op.getIndices(),
AffineMapAttr::get(newMap), newMask, newInBoundsAttr);
@@ -386,11 +386,8 @@ struct TransferOpReduceRank
VectorType newReadType = VectorType::get(
newShape, originalVecType.getElementType(), newScalableDims);
- ArrayAttr newInBoundsAttr =
- op.getInBounds()
- ? rewriter.getArrayAttr(
- op.getInBoundsAttr().getValue().take_back(reducedShapeRank))
- : ArrayAttr();
+ DenseBoolArrayAttr newInBoundsAttr = rewriter.getDenseBoolArrayAttr(
+ op.getInBounds().take_back(reducedShapeRank));
Value newRead = rewriter.create<vector::TransferReadOp>(
op.getLoc(), newReadType, op.getSource(), op.getIndices(),
AffineMapAttr::get(newMap), op.getPadding(), op.getMask(),
diff --git a/mlir/lib/Dialect/Vector/Transforms/VectorDropLeadUnitDim.cpp b/mlir/lib/Dialect/Vector/Transforms/VectorDropL...
[truncated]
|
+1. I think it would be important to minimize the conflicts so if we could keep the |
I've explored that and just didn't like the extra parsing/printing specialisation required for this (as opposed re-using the existing hooks to parse/print attributes). @MacDue suggested the following to make parsing somewhat easier. This way we'd know what to expect (DenseBoolAttrArray) and where (i.e. not a random location within vector.transfer_read %arg0[%0, %1], %cst, in_bounds = [true] { permutation_map = #map3} : memref<12x16xf32>, vector<8xf32>Having played with that a bit, I just feel it's too different to what we use in other places. IMO, EDIT #! /bin/env bash
set -x
files_to_update=($(rg -l in_bounds mlir/test/ -g '*.{mlir}'))
for file in "${files_to_update[@]}"
do
echo "$file"
gsed -i -r 's/(in_bounds \= )\[(.*)(true|false)\]/\1array<i1: \2\3>/g' $file
done |
banach-space
force-pushed
the
andrzej/update_in_bounds
branch
from
13f2559 to
380f202
Compare
MacDue
reviewed
Aug 13, 2024
MacDue
reviewed
Aug 13, 2024
MacDue
reviewed
Aug 14, 2024
|
I have some concerns about keeping a dictionary of attribute syntax while making it appear as something different: that seems like counter-intuitive to the users who think that they see an |
I would be surprised if people think that much when they see something like Also, as this is an inherent attribute over in C++ (along with tablegen and the docs), you can see the type of the attribute is a dense array. I don't really have a strong preference on the syntax here though (any of the possible changes seem fine). I was just trying to find a solution that'd keep everyone happy -- though it seems like that may not be possible 😅 |
|
@joker-eph IIUC, you feel that this format (consistent with the in-memory representation) would be more intuitive: vector.transfer_read %arg0[%0, %1], %cst {in_bounds = array<i1: true>, permutation_map = #map3} : memref<12x16xf32>, vector<8xf32>Which brings us back to @dcaballe 's comment:
Lets wait for Diego to get back to this (and all the new context since his original comment😅). Thanks for all the discussion so far 🙏🏻 |
|
Actually the more modern way would be to ensure that no inherent attribute is mixed up in the attr-dict, but pending such upgrade yes I would rather not have attr-dict not reflect the in-memory format. |
|
Could we have someone to look at the parser/printer changes? I can take a look at the rest. Perhaps @River707 or @joker-eph can help? |
@dcaballe Given the discussion under #103304, IMHO we should avoid updating the parser. The idea is to take this change to square one and change the syntax instead. Apologies, I should've made that clearer. I am about to update this PR accordingly. |
This PR updates the internal representation of the `in_bounds` attribute
for `xfer_read`/`xfer_write` Ops. Currently we use `ArrayAttr` - that's
being updated to `DenseBoolArrayAttribute`.
Note that this means that the asm format of the `xfer_{read|_write}`
will change from:
```mlir
vector.transfer_read %arg0[%0, %1], %cst {in_bounds = [true], permutation_map = #map3} : memref<12x16xf32>, vector<8xf32>
```
to:
```mlir
vector.transfer_read %arg0[%0, %1], %cst {in_bounds = array<i1: true>, permutation_map = #map3} : memref<12x16xf32>, vector<8xf32>
```
banach-space
force-pushed
the
andrzej/update_in_bounds
branch
from
392b90c to
f0bf455
Compare
|
I'm not working on this ATM and clearly we need to discuss more, so I will just close this one for now. Thank you all for your help/inputs! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note that this means that the asm format of the xfer_{read|_write}
will change from:
to: