[mlir][affine] Remove isValidAffineIndexOperand
#73027
Closed
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
|
@llvm/pr-subscribers-mlir-affine Author: Rik Huijzer (rikhuijzer) ChangesThe function static bool isValidAffineIndexOperand(Value value, Region *region) {
return isValidDim(value, region) || isValidSymbol(value, region);
}is redundant because bool mlir::affine::isValidDim(Value value, Region *region) {
// The value must be an index type.
if (!value.getType().isIndex())
return false;
// All valid symbols are okay.
if (isValidSymbol(value, region))
return true;
auto *op = value.getDefiningOp();
if (!op) {
// This value has to be a block argument for an affine.for or an
// affine.parallel.
auto *parentOp = llvm::cast<BlockArgument>(value).getOwner()->getParentOp();
return isa<AffineForOp, AffineParallelOp>(parentOp);
}
// Affine apply operation is ok if all of its operands are ok.
if (auto applyOp = dyn_cast<AffineApplyOp>(op))
return applyOp.isValidDim(region);
// The dim op is okay if its operand memref/tensor is defined at the top
// level.
if (auto dimOp = dyn_cast<ShapedDimOpInterface>(op))
return isTopLevelValue(dimOp.getShapedValue());
return false;
}and bool mlir::affine::isValidSymbol(Value value, Region *region) {
// The value must be an index type.
if (!value.getType().isIndex())
return false;
[...]
}To see that the function is redundant, consider the following cases:
Or, put differently, consider the following cases:
In all cases, Full diff: https://github.com/llvm/llvm-project/pull/73027.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
index d22a7539fb75018..61c66361ce1fb32 100644
--- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
+++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
@@ -434,13 +434,6 @@ bool mlir::affine::isValidSymbol(Value value, Region *region) {
return false;
}
-// Returns true if 'value' is a valid index to an affine operation (e.g.
-// affine.load, affine.store, affine.dma_start, affine.dma_wait) where
-// `region` provides the polyhedral symbol scope. Returns false otherwise.
-static bool isValidAffineIndexOperand(Value value, Region *region) {
- return isValidDim(value, region) || isValidSymbol(value, region);
-}
-
/// Prints dimension and symbol list.
static void printDimAndSymbolList(Operation::operand_iterator begin,
Operation::operand_iterator end,
@@ -1650,19 +1643,19 @@ LogicalResult AffineDmaStartOp::verifyInvariantsImpl() {
for (auto idx : getSrcIndices()) {
if (!idx.getType().isIndex())
return emitOpError("src index to dma_start must have 'index' type");
- if (!isValidAffineIndexOperand(idx, scope))
+ if (!isValidDim(idx, scope))
return emitOpError("src index must be a dimension or symbol identifier");
}
for (auto idx : getDstIndices()) {
if (!idx.getType().isIndex())
return emitOpError("dst index to dma_start must have 'index' type");
- if (!isValidAffineIndexOperand(idx, scope))
+ if (!isValidDim(idx, scope))
return emitOpError("dst index must be a dimension or symbol identifier");
}
for (auto idx : getTagIndices()) {
if (!idx.getType().isIndex())
return emitOpError("tag index to dma_start must have 'index' type");
- if (!isValidAffineIndexOperand(idx, scope))
+ if (!isValidDim(idx, scope))
return emitOpError("tag index must be a dimension or symbol identifier");
}
return success();
@@ -1751,7 +1744,7 @@ LogicalResult AffineDmaWaitOp::verifyInvariantsImpl() {
for (auto idx : getTagIndices()) {
if (!idx.getType().isIndex())
return emitOpError("index to dma_wait must have 'index' type");
- if (!isValidAffineIndexOperand(idx, scope))
+ if (!isValidDim(idx, scope))
return emitOpError("index must be a dimension or symbol identifier");
}
return success();
@@ -2913,8 +2906,7 @@ static void composeSetAndOperands(IntegerSet &set,
}
/// Canonicalize an affine if op's conditional (integer set + operands).
-LogicalResult AffineIfOp::fold(FoldAdaptor,
- SmallVectorImpl<OpFoldResult> &) {
+LogicalResult AffineIfOp::fold(FoldAdaptor, SmallVectorImpl<OpFoldResult> &) {
auto set = getIntegerSet();
SmallVector<Value, 4> operands(getOperands());
composeSetAndOperands(set, operands);
@@ -3005,17 +2997,17 @@ static LogicalResult
verifyMemoryOpIndexing(Operation *op, AffineMapAttr mapAttr,
Operation::operand_range mapOperands,
MemRefType memrefType, unsigned numIndexOperands) {
- AffineMap map = mapAttr.getValue();
- if (map.getNumResults() != memrefType.getRank())
- return op->emitOpError("affine map num results must equal memref rank");
- if (map.getNumInputs() != numIndexOperands)
- return op->emitOpError("expects as many subscripts as affine map inputs");
+ AffineMap map = mapAttr.getValue();
+ if (map.getNumResults() != memrefType.getRank())
+ return op->emitOpError("affine map num results must equal memref rank");
+ if (map.getNumInputs() != numIndexOperands)
+ return op->emitOpError("expects as many subscripts as affine map inputs");
Region *scope = getAffineScope(op);
- for (auto idx : mapOperands) {
+ for (Value idx : mapOperands) {
if (!idx.getType().isIndex())
return op->emitOpError("index to load must have 'index' type");
- if (!isValidAffineIndexOperand(idx, scope))
+ if (!isValidDim(idx, scope))
return op->emitOpError("index must be a dimension or symbol identifier");
}
@@ -3604,7 +3596,7 @@ LogicalResult AffinePrefetchOp::verify() {
Region *scope = getAffineScope(*this);
for (auto idx : getMapOperands()) {
- if (!isValidAffineIndexOperand(idx, scope))
+ if (!isValidDim(idx, scope))
return emitOpError("index must be a dimension or symbol identifier");
}
return success();
|
|
@llvm/pr-subscribers-mlir Author: Rik Huijzer (rikhuijzer) ChangesThe function static bool isValidAffineIndexOperand(Value value, Region *region) {
return isValidDim(value, region) || isValidSymbol(value, region);
}is redundant because bool mlir::affine::isValidDim(Value value, Region *region) {
// The value must be an index type.
if (!value.getType().isIndex())
return false;
// All valid symbols are okay.
if (isValidSymbol(value, region))
return true;
auto *op = value.getDefiningOp();
if (!op) {
// This value has to be a block argument for an affine.for or an
// affine.parallel.
auto *parentOp = llvm::cast<BlockArgument>(value).getOwner()->getParentOp();
return isa<AffineForOp, AffineParallelOp>(parentOp);
}
// Affine apply operation is ok if all of its operands are ok.
if (auto applyOp = dyn_cast<AffineApplyOp>(op))
return applyOp.isValidDim(region);
// The dim op is okay if its operand memref/tensor is defined at the top
// level.
if (auto dimOp = dyn_cast<ShapedDimOpInterface>(op))
return isTopLevelValue(dimOp.getShapedValue());
return false;
}and bool mlir::affine::isValidSymbol(Value value, Region *region) {
// The value must be an index type.
if (!value.getType().isIndex())
return false;
[...]
}To see that the function is redundant, consider the following cases:
Or, put differently, consider the following cases:
In all cases, Full diff: https://github.com/llvm/llvm-project/pull/73027.diff 1 Files Affected:
diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
index d22a7539fb75018..61c66361ce1fb32 100644
--- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
+++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
@@ -434,13 +434,6 @@ bool mlir::affine::isValidSymbol(Value value, Region *region) {
return false;
}
-// Returns true if 'value' is a valid index to an affine operation (e.g.
-// affine.load, affine.store, affine.dma_start, affine.dma_wait) where
-// `region` provides the polyhedral symbol scope. Returns false otherwise.
-static bool isValidAffineIndexOperand(Value value, Region *region) {
- return isValidDim(value, region) || isValidSymbol(value, region);
-}
-
/// Prints dimension and symbol list.
static void printDimAndSymbolList(Operation::operand_iterator begin,
Operation::operand_iterator end,
@@ -1650,19 +1643,19 @@ LogicalResult AffineDmaStartOp::verifyInvariantsImpl() {
for (auto idx : getSrcIndices()) {
if (!idx.getType().isIndex())
return emitOpError("src index to dma_start must have 'index' type");
- if (!isValidAffineIndexOperand(idx, scope))
+ if (!isValidDim(idx, scope))
return emitOpError("src index must be a dimension or symbol identifier");
}
for (auto idx : getDstIndices()) {
if (!idx.getType().isIndex())
return emitOpError("dst index to dma_start must have 'index' type");
- if (!isValidAffineIndexOperand(idx, scope))
+ if (!isValidDim(idx, scope))
return emitOpError("dst index must be a dimension or symbol identifier");
}
for (auto idx : getTagIndices()) {
if (!idx.getType().isIndex())
return emitOpError("tag index to dma_start must have 'index' type");
- if (!isValidAffineIndexOperand(idx, scope))
+ if (!isValidDim(idx, scope))
return emitOpError("tag index must be a dimension or symbol identifier");
}
return success();
@@ -1751,7 +1744,7 @@ LogicalResult AffineDmaWaitOp::verifyInvariantsImpl() {
for (auto idx : getTagIndices()) {
if (!idx.getType().isIndex())
return emitOpError("index to dma_wait must have 'index' type");
- if (!isValidAffineIndexOperand(idx, scope))
+ if (!isValidDim(idx, scope))
return emitOpError("index must be a dimension or symbol identifier");
}
return success();
@@ -2913,8 +2906,7 @@ static void composeSetAndOperands(IntegerSet &set,
}
/// Canonicalize an affine if op's conditional (integer set + operands).
-LogicalResult AffineIfOp::fold(FoldAdaptor,
- SmallVectorImpl<OpFoldResult> &) {
+LogicalResult AffineIfOp::fold(FoldAdaptor, SmallVectorImpl<OpFoldResult> &) {
auto set = getIntegerSet();
SmallVector<Value, 4> operands(getOperands());
composeSetAndOperands(set, operands);
@@ -3005,17 +2997,17 @@ static LogicalResult
verifyMemoryOpIndexing(Operation *op, AffineMapAttr mapAttr,
Operation::operand_range mapOperands,
MemRefType memrefType, unsigned numIndexOperands) {
- AffineMap map = mapAttr.getValue();
- if (map.getNumResults() != memrefType.getRank())
- return op->emitOpError("affine map num results must equal memref rank");
- if (map.getNumInputs() != numIndexOperands)
- return op->emitOpError("expects as many subscripts as affine map inputs");
+ AffineMap map = mapAttr.getValue();
+ if (map.getNumResults() != memrefType.getRank())
+ return op->emitOpError("affine map num results must equal memref rank");
+ if (map.getNumInputs() != numIndexOperands)
+ return op->emitOpError("expects as many subscripts as affine map inputs");
Region *scope = getAffineScope(op);
- for (auto idx : mapOperands) {
+ for (Value idx : mapOperands) {
if (!idx.getType().isIndex())
return op->emitOpError("index to load must have 'index' type");
- if (!isValidAffineIndexOperand(idx, scope))
+ if (!isValidDim(idx, scope))
return op->emitOpError("index must be a dimension or symbol identifier");
}
@@ -3604,7 +3596,7 @@ LogicalResult AffinePrefetchOp::verify() {
Region *scope = getAffineScope(*this);
for (auto idx : getMapOperands()) {
- if (!isValidAffineIndexOperand(idx, scope))
+ if (!isValidDim(idx, scope))
return emitOpError("index must be a dimension or symbol identifier");
}
return success();
|
Lewuathe
approved these changes
Dec 8, 2023
There was a problem hiding this comment.
- value.getType().isIndex() is true
- isValidDim is false which implies that isValidSymbol must be false
- isValidDim is true which means that isValidDim(value, region) || isValidSymbol(value, region) must be true.
Sorry, I didn't get this part. If I understand correctly, the first case states if isValidSymbol is false, isValidDim(value, region) || isValidSymbol(value, region) is identical to isValidDim(value, region). The second case is if isValidSymbol is true, isValidDim(value, region) || isValidSymbol(value, region) is identical to isValidDim(value, region) which is true anyway. Is my understanding correct?
Anyway, this change seems good. Could you resolve the conflict when you get a chance?
I tried to go through the logic again but I think I've made a mistake somewhere. I'll just close this. Thanks for your review anyway. |
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.
The function
is redundant because
isValidDimis defined asand
isValidSymbolis defined asTo see that the function is redundant, consider the following cases:
isValidDim(value, region)is true, thenisValidDim(value, region) || isValidSymbol(value, region)must be true.isValidDim(value, region)is false, then eithervalue.getType().isIndex()is false, which means that bothisValidDimandisValidSymbolmust be false, orvalue.getType().isIndex()is true, but thenisValidSymbolmust be false too orisValidDim(value, region)wouldn't be false.Or, put differently, consider the following cases:
value.getType().isIndex()is false, then bothisValidDim(value, region) || isValidSymbol(value, region)are false.value.getType().isIndex()is true. Also assumeisValidDimis false, thenisValidSymbolmust be false orisValidDimmust be true which means thatisValidDim(value, region) || isValidSymbol(value, region)must be true.In all cases,
isValidDim(value, region) || isValidSymbol(value, region)is equivalent toisValidDim(value, region).