From 4fc8bfb0bb69d203f08fe5e9c7f0c62d2a179c2e Mon Sep 17 00:00:00 2001 From: "William S. Moses" Date: Wed, 21 May 2025 16:06:38 -0500 Subject: [PATCH 1/6] [MLIR] Change getBackwardSlice to return a logicalresult rather than crash --- mlir/include/mlir/Analysis/SliceAnalysis.h | 10 ++-- .../mlir/Query/Matcher/SliceMatchers.h | 3 +- mlir/lib/Analysis/SliceAnalysis.cpp | 52 ++++++++++++------- .../Conversion/VectorToGPU/VectorToGPU.cpp | 4 +- .../Linalg/Transforms/HoistPadding.cpp | 7 ++- .../NVGPU/TransformOps/NVGPUTransformOps.cpp | 6 ++- .../SCF/Transforms/TileUsingInterface.cpp | 3 +- mlir/lib/Transforms/Utils/RegionUtils.cpp | 6 ++- .../Dialect/Affine/TestVectorizationUtils.cpp | 3 +- mlir/test/lib/IR/TestSlicing.cpp | 3 +- 10 files changed, 62 insertions(+), 35 deletions(-) diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h index 3b731e8bb1c22..192b096fd31f6 100644 --- a/mlir/include/mlir/Analysis/SliceAnalysis.h +++ b/mlir/include/mlir/Analysis/SliceAnalysis.h @@ -138,13 +138,15 @@ void getForwardSlice(Value root, SetVector *forwardSlice, /// Assuming all local orders match the numbering order: /// {1, 2, 5, 3, 4, 6} /// -void getBackwardSlice(Operation *op, SetVector *backwardSlice, - const BackwardSliceOptions &options = {}); +LogicalResult getBackwardSlice(Operation *op, + SetVector *backwardSlice, + const BackwardSliceOptions &options = {}); /// Value-rooted version of `getBackwardSlice`. Return the union of all backward /// slices for the op defining or owning the value `root`. -void getBackwardSlice(Value root, SetVector *backwardSlice, - const BackwardSliceOptions &options = {}); +LogicalResult getBackwardSlice(Value root, + SetVector *backwardSlice, + const BackwardSliceOptions &options = {}); /// Iteratively computes backward slices and forward slices until /// a fixed point is reached. Returns an `SetVector` which diff --git a/mlir/include/mlir/Query/Matcher/SliceMatchers.h b/mlir/include/mlir/Query/Matcher/SliceMatchers.h index 1b0e4c32dbe94..6b7d10ccaed64 100644 --- a/mlir/include/mlir/Query/Matcher/SliceMatchers.h +++ b/mlir/include/mlir/Query/Matcher/SliceMatchers.h @@ -112,7 +112,8 @@ bool BackwardSliceMatcher::matches( } return true; }; - getBackwardSlice(rootOp, &backwardSlice, options); + auto result = getBackwardSlice(rootOp, &backwardSlice, options); + assert(result.succeeded()); return options.inclusive ? backwardSlice.size() > 1 : backwardSlice.size() >= 1; } diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp index 5aebb19e3a86e..fea1356eab4c9 100644 --- a/mlir/lib/Analysis/SliceAnalysis.cpp +++ b/mlir/lib/Analysis/SliceAnalysis.cpp @@ -80,22 +80,25 @@ void mlir::getForwardSlice(Value root, SetVector *forwardSlice, forwardSlice->insert(v.rbegin(), v.rend()); } -static void getBackwardSliceImpl(Operation *op, - SetVector *backwardSlice, - const BackwardSliceOptions &options) { +static LogicalResult getBackwardSliceImpl(Operation *op, + SetVector *backwardSlice, + const BackwardSliceOptions &options) { if (!op || op->hasTrait()) - return; + return success(); // Evaluate whether we should keep this def. // This is useful in particular to implement scoping; i.e. return the // transitive backwardSlice in the current scope. if (options.filter && !options.filter(op)) - return; + return success(); + + bool succeeded = true; auto processValue = [&](Value value) { if (auto *definingOp = value.getDefiningOp()) { if (backwardSlice->count(definingOp) == 0) - getBackwardSliceImpl(definingOp, backwardSlice, options); + succeeded &= getBackwardSliceImpl(definingOp, backwardSlice, options) + .succeeded(); } else if (auto blockArg = dyn_cast(value)) { if (options.omitBlockArguments) return; @@ -106,9 +109,13 @@ static void getBackwardSliceImpl(Operation *op, // blocks of parentOp, which are not technically backward unless they flow // into us. For now, just bail. if (parentOp && backwardSlice->count(parentOp) == 0) { - assert(parentOp->getNumRegions() == 1 && - llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())); - getBackwardSliceImpl(parentOp, backwardSlice, options); + if (parentOp->getNumRegions() == 1 && + llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())) { + succeeded &= getBackwardSliceImpl(parentOp, backwardSlice, options) + .succeeded(); + } else { + succeeded = false; + } } } else { llvm_unreachable("No definingOp and not a block argument."); @@ -133,28 +140,30 @@ static void getBackwardSliceImpl(Operation *op, llvm::for_each(op->getOperands(), processValue); backwardSlice->insert(op); + return success(succeeded); } -void mlir::getBackwardSlice(Operation *op, - SetVector *backwardSlice, - const BackwardSliceOptions &options) { - getBackwardSliceImpl(op, backwardSlice, options); +LogicalResult +mlir::getBackwardSlice(Operation *op, SetVector *backwardSlice, + const BackwardSliceOptions &options) { + LogicalResult result = getBackwardSliceImpl(op, backwardSlice, options); if (!options.inclusive) { // Don't insert the top level operation, we just queried on it and don't // want it in the results. backwardSlice->remove(op); } + return result; } -void mlir::getBackwardSlice(Value root, SetVector *backwardSlice, - const BackwardSliceOptions &options) { +LogicalResult mlir::getBackwardSlice(Value root, + SetVector *backwardSlice, + const BackwardSliceOptions &options) { if (Operation *definingOp = root.getDefiningOp()) { - getBackwardSlice(definingOp, backwardSlice, options); - return; + return getBackwardSlice(definingOp, backwardSlice, options); } Operation *bbAargOwner = cast(root).getOwner()->getParentOp(); - getBackwardSlice(bbAargOwner, backwardSlice, options); + return getBackwardSlice(bbAargOwner, backwardSlice, options); } SetVector @@ -170,7 +179,9 @@ mlir::getSlice(Operation *op, const BackwardSliceOptions &backwardSliceOptions, auto *currentOp = (slice)[currentIndex]; // Compute and insert the backwardSlice starting from currentOp. backwardSlice.clear(); - getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions); + auto result = + getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions); + assert(result.succeeded()); slice.insert_range(backwardSlice); // Compute and insert the forwardSlice starting from currentOp. @@ -193,7 +204,8 @@ static bool dependsOnCarriedVals(Value value, sliceOptions.filter = [&](Operation *op) { return !ancestorOp->isAncestor(op); }; - getBackwardSlice(value, &slice, sliceOptions); + auto result = getBackwardSlice(value, &slice, sliceOptions); + assert(result.succeeded()); // Check that none of the operands of the operations in the backward slice are // loop iteration arguments, and neither is the value itself. diff --git a/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp b/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp index 8b16da387457d..22df878cfe18d 100644 --- a/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp +++ b/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp @@ -317,7 +317,9 @@ getSliceContract(Operation *op, auto *currentOp = (slice)[currentIndex]; // Compute and insert the backwardSlice starting from currentOp. backwardSlice.clear(); - getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions); + auto result = + getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions); + assert(result.succeeded()); slice.insert_range(backwardSlice); // Compute and insert the forwardSlice starting from currentOp. diff --git a/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp b/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp index d33a17af63459..ee0d44be60b59 100644 --- a/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp +++ b/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp @@ -124,10 +124,13 @@ static void computeBackwardSlice(tensor::PadOp padOp, getUsedValuesDefinedAbove(padOp.getRegion(), padOp.getRegion(), valuesDefinedAbove); for (Value v : valuesDefinedAbove) { - getBackwardSlice(v, &backwardSlice, sliceOptions); + auto result = getBackwardSlice(v, &backwardSlice, sliceOptions); + assert(result.succeeded()); } // Then, add the backward slice from padOp itself. - getBackwardSlice(padOp.getOperation(), &backwardSlice, sliceOptions); + auto result = + getBackwardSlice(padOp.getOperation(), &backwardSlice, sliceOptions); + assert(result.succeeded()); } //===----------------------------------------------------------------------===// diff --git a/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp b/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp index 75dbe0becf80d..c5b8117283b25 100644 --- a/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp +++ b/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp @@ -290,8 +290,10 @@ static void getPipelineStages( }); options.inclusive = true; for (Operation &op : forOp.getBody()->getOperations()) { - if (stage0Ops.contains(&op)) - getBackwardSlice(&op, &dependencies, options); + if (stage0Ops.contains(&op)) { + auto result = getBackwardSlice(&op, &dependencies, options); + assert(result.succeeded()); + } } for (Operation &op : forOp.getBody()->getOperations()) { diff --git a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp index 719e2c6fa459e..273fdbfbc4ffe 100644 --- a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp +++ b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp @@ -1772,7 +1772,8 @@ checkAssumptionForLoop(Operation *loopOp, Operation *consumerOp, }; llvm::SetVector slice; for (auto operand : consumerOp->getOperands()) { - getBackwardSlice(operand, &slice, options); + auto result = getBackwardSlice(operand, &slice, options); + assert(result.succeeded()); } if (!slice.empty()) { diff --git a/mlir/lib/Transforms/Utils/RegionUtils.cpp b/mlir/lib/Transforms/Utils/RegionUtils.cpp index 4985d718c1780..70ba5e321db6d 100644 --- a/mlir/lib/Transforms/Utils/RegionUtils.cpp +++ b/mlir/lib/Transforms/Utils/RegionUtils.cpp @@ -1094,7 +1094,8 @@ LogicalResult mlir::moveOperationDependencies(RewriterBase &rewriter, return !dominance.properlyDominates(sliceBoundaryOp, insertionPoint); }; llvm::SetVector slice; - getBackwardSlice(op, &slice, options); + auto result = getBackwardSlice(op, &slice, options); + assert(result.succeeded()); // If the slice contains `insertionPoint` cannot move the dependencies. if (slice.contains(insertionPoint)) { @@ -1159,7 +1160,8 @@ LogicalResult mlir::moveValueDefinitions(RewriterBase &rewriter, }; llvm::SetVector slice; for (auto value : prunedValues) { - getBackwardSlice(value, &slice, options); + auto result = getBackwardSlice(value, &slice, options); + assert(result.succeeded()); } // If the slice contains `insertionPoint` cannot move the dependencies. diff --git a/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp b/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp index f26058f30ad7b..041cc25b23cbd 100644 --- a/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp +++ b/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp @@ -154,7 +154,8 @@ void VectorizerTestPass::testBackwardSlicing(llvm::raw_ostream &outs) { patternTestSlicingOps().match(f, &matches); for (auto m : matches) { SetVector backwardSlice; - getBackwardSlice(m.getMatchedOperation(), &backwardSlice); + auto result = getBackwardSlice(m.getMatchedOperation(), &backwardSlice); + assert(result.succeeded()); outs << "\nmatched: " << *m.getMatchedOperation() << " backward static slice: "; for (auto *op : backwardSlice) diff --git a/mlir/test/lib/IR/TestSlicing.cpp b/mlir/test/lib/IR/TestSlicing.cpp index e99d5976d6d9d..53ad9b5819986 100644 --- a/mlir/test/lib/IR/TestSlicing.cpp +++ b/mlir/test/lib/IR/TestSlicing.cpp @@ -41,7 +41,8 @@ static LogicalResult createBackwardSliceFunction(Operation *op, options.omitBlockArguments = omitBlockArguments; // TODO: Make this default. options.omitUsesFromAbove = false; - getBackwardSlice(op, &slice, options); + auto result = getBackwardSlice(op, &slice, options); + assert(result.succeeded()); for (Operation *slicedOp : slice) builder.clone(*slicedOp, mapper); builder.create(loc); From f1f9ca604a86edcb63e360bac5d671762b1021a0 Mon Sep 17 00:00:00 2001 From: William Moses Date: Wed, 21 May 2025 16:39:48 -0500 Subject: [PATCH 2/6] Apply suggestions from code review Co-authored-by: Oleksandr "Alex" Zinenko --- mlir/include/mlir/Query/Matcher/SliceMatchers.h | 4 ++-- mlir/lib/Analysis/SliceAnalysis.cpp | 4 ++-- mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp | 4 ++-- mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp | 8 ++++---- mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp | 4 ++-- mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp | 4 ++-- mlir/lib/Transforms/Utils/RegionUtils.cpp | 8 ++++---- mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp | 4 ++-- mlir/test/lib/IR/TestSlicing.cpp | 4 ++-- 9 files changed, 22 insertions(+), 22 deletions(-) diff --git a/mlir/include/mlir/Query/Matcher/SliceMatchers.h b/mlir/include/mlir/Query/Matcher/SliceMatchers.h index 6b7d10ccaed64..40a39d23ca695 100644 --- a/mlir/include/mlir/Query/Matcher/SliceMatchers.h +++ b/mlir/include/mlir/Query/Matcher/SliceMatchers.h @@ -112,8 +112,8 @@ bool BackwardSliceMatcher::matches( } return true; }; - auto result = getBackwardSlice(rootOp, &backwardSlice, options); - assert(result.succeeded()); + LogicalResult result = getBackwardSlice(rootOp, &backwardSlice, options); + assert(result.succeeded() && "expected backward slice to succeed"); return options.inclusive ? backwardSlice.size() > 1 : backwardSlice.size() >= 1; } diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp index fea1356eab4c9..597f9b415280a 100644 --- a/mlir/lib/Analysis/SliceAnalysis.cpp +++ b/mlir/lib/Analysis/SliceAnalysis.cpp @@ -179,7 +179,7 @@ mlir::getSlice(Operation *op, const BackwardSliceOptions &backwardSliceOptions, auto *currentOp = (slice)[currentIndex]; // Compute and insert the backwardSlice starting from currentOp. backwardSlice.clear(); - auto result = + LogicalResult result = getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions); assert(result.succeeded()); slice.insert_range(backwardSlice); @@ -204,7 +204,7 @@ static bool dependsOnCarriedVals(Value value, sliceOptions.filter = [&](Operation *op) { return !ancestorOp->isAncestor(op); }; - auto result = getBackwardSlice(value, &slice, sliceOptions); + LogicalResult result = getBackwardSlice(value, &slice, sliceOptions); assert(result.succeeded()); // Check that none of the operands of the operations in the backward slice are diff --git a/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp b/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp index 22df878cfe18d..0ec9ddc25ff8d 100644 --- a/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp +++ b/mlir/lib/Conversion/VectorToGPU/VectorToGPU.cpp @@ -317,9 +317,9 @@ getSliceContract(Operation *op, auto *currentOp = (slice)[currentIndex]; // Compute and insert the backwardSlice starting from currentOp. backwardSlice.clear(); - auto result = + LogicalResult result = getBackwardSlice(currentOp, &backwardSlice, backwardSliceOptions); - assert(result.succeeded()); + assert(result.succeeded() && "expected a backward slice"); slice.insert_range(backwardSlice); // Compute and insert the forwardSlice starting from currentOp. diff --git a/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp b/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp index ee0d44be60b59..2c98bd3ba93af 100644 --- a/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp +++ b/mlir/lib/Dialect/Linalg/Transforms/HoistPadding.cpp @@ -124,13 +124,13 @@ static void computeBackwardSlice(tensor::PadOp padOp, getUsedValuesDefinedAbove(padOp.getRegion(), padOp.getRegion(), valuesDefinedAbove); for (Value v : valuesDefinedAbove) { - auto result = getBackwardSlice(v, &backwardSlice, sliceOptions); - assert(result.succeeded()); + LogicalResult result = getBackwardSlice(v, &backwardSlice, sliceOptions); + assert(result.succeeded() && "expected a backward slice"); } // Then, add the backward slice from padOp itself. - auto result = + LogicalResult result = getBackwardSlice(padOp.getOperation(), &backwardSlice, sliceOptions); - assert(result.succeeded()); + assert(result.succeeded() && "expected a backward slice"); } //===----------------------------------------------------------------------===// diff --git a/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp b/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp index c5b8117283b25..1046f5798ecd4 100644 --- a/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp +++ b/mlir/lib/Dialect/NVGPU/TransformOps/NVGPUTransformOps.cpp @@ -291,8 +291,8 @@ static void getPipelineStages( options.inclusive = true; for (Operation &op : forOp.getBody()->getOperations()) { if (stage0Ops.contains(&op)) { - auto result = getBackwardSlice(&op, &dependencies, options); - assert(result.succeeded()); + LogicalResult result = getBackwardSlice(&op, &dependencies, options); + assert(result.succeeded() && "expected a backward slice"); } } diff --git a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp index 273fdbfbc4ffe..9e3d3f8b10a13 100644 --- a/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp +++ b/mlir/lib/Dialect/SCF/Transforms/TileUsingInterface.cpp @@ -1772,8 +1772,8 @@ checkAssumptionForLoop(Operation *loopOp, Operation *consumerOp, }; llvm::SetVector slice; for (auto operand : consumerOp->getOperands()) { - auto result = getBackwardSlice(operand, &slice, options); - assert(result.succeeded()); + LogicalResult result = getBackwardSlice(operand, &slice, options); + assert(result.succeeded() && "expected a backward slice"); } if (!slice.empty()) { diff --git a/mlir/lib/Transforms/Utils/RegionUtils.cpp b/mlir/lib/Transforms/Utils/RegionUtils.cpp index 70ba5e321db6d..c136ff92255cd 100644 --- a/mlir/lib/Transforms/Utils/RegionUtils.cpp +++ b/mlir/lib/Transforms/Utils/RegionUtils.cpp @@ -1094,8 +1094,8 @@ LogicalResult mlir::moveOperationDependencies(RewriterBase &rewriter, return !dominance.properlyDominates(sliceBoundaryOp, insertionPoint); }; llvm::SetVector slice; - auto result = getBackwardSlice(op, &slice, options); - assert(result.succeeded()); + LogicalResult result = getBackwardSlice(op, &slice, options); + assert(result.succeeded() && "expected a backward slice"); // If the slice contains `insertionPoint` cannot move the dependencies. if (slice.contains(insertionPoint)) { @@ -1160,8 +1160,8 @@ LogicalResult mlir::moveValueDefinitions(RewriterBase &rewriter, }; llvm::SetVector slice; for (auto value : prunedValues) { - auto result = getBackwardSlice(value, &slice, options); - assert(result.succeeded()); + LogicalResult result = getBackwardSlice(value, &slice, options); + assert(result.succeeded() && "expected a backward slice"); } // If the slice contains `insertionPoint` cannot move the dependencies. diff --git a/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp b/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp index 041cc25b23cbd..77496be8967a1 100644 --- a/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp +++ b/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp @@ -154,8 +154,8 @@ void VectorizerTestPass::testBackwardSlicing(llvm::raw_ostream &outs) { patternTestSlicingOps().match(f, &matches); for (auto m : matches) { SetVector backwardSlice; - auto result = getBackwardSlice(m.getMatchedOperation(), &backwardSlice); - assert(result.succeeded()); + LogicalResult result = getBackwardSlice(m.getMatchedOperation(), &backwardSlice); + assert(result.succeeded() && "expected a backward slice"); outs << "\nmatched: " << *m.getMatchedOperation() << " backward static slice: "; for (auto *op : backwardSlice) diff --git a/mlir/test/lib/IR/TestSlicing.cpp b/mlir/test/lib/IR/TestSlicing.cpp index 53ad9b5819986..ad99be2b9d0c9 100644 --- a/mlir/test/lib/IR/TestSlicing.cpp +++ b/mlir/test/lib/IR/TestSlicing.cpp @@ -41,8 +41,8 @@ static LogicalResult createBackwardSliceFunction(Operation *op, options.omitBlockArguments = omitBlockArguments; // TODO: Make this default. options.omitUsesFromAbove = false; - auto result = getBackwardSlice(op, &slice, options); - assert(result.succeeded()); + LogicalResult result = getBackwardSlice(op, &slice, options); + assert(result.succeeded() && "expected a backward slice"); for (Operation *slicedOp : slice) builder.clone(*slicedOp, mapper); builder.create(loc); From 57f92104014df4667926b57dc1168b96940a30cf Mon Sep 17 00:00:00 2001 From: "William S. Moses" Date: Thu, 22 May 2025 12:09:20 -0500 Subject: [PATCH 3/6] fix --- mlir/include/mlir/Analysis/SliceAnalysis.h | 2 ++ mlir/lib/Analysis/SliceAnalysis.cpp | 20 +++++++++++--------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h index 192b096fd31f6..e4212e97c6ce3 100644 --- a/mlir/include/mlir/Analysis/SliceAnalysis.h +++ b/mlir/include/mlir/Analysis/SliceAnalysis.h @@ -138,6 +138,8 @@ void getForwardSlice(Value root, SetVector *forwardSlice, /// Assuming all local orders match the numbering order: /// {1, 2, 5, 3, 4, 6} /// +/// This function returns whether the backwards slice was able to be successfully +/// computed, and failure if it was unable to determine the slice. LogicalResult getBackwardSlice(Operation *op, SetVector *backwardSlice, const BackwardSliceOptions &options = {}); diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp index 597f9b415280a..976a63ac32448 100644 --- a/mlir/lib/Analysis/SliceAnalysis.cpp +++ b/mlir/lib/Analysis/SliceAnalysis.cpp @@ -92,16 +92,14 @@ static LogicalResult getBackwardSliceImpl(Operation *op, if (options.filter && !options.filter(op)) return success(); - bool succeeded = true; - auto processValue = [&](Value value) { if (auto *definingOp = value.getDefiningOp()) { if (backwardSlice->count(definingOp) == 0) - succeeded &= getBackwardSliceImpl(definingOp, backwardSlice, options) + return getBackwardSliceImpl(definingOp, backwardSlice, options) .succeeded(); } else if (auto blockArg = dyn_cast(value)) { if (options.omitBlockArguments) - return; + return success(); Block *block = blockArg.getOwner(); Operation *parentOp = block->getParentOp(); @@ -111,17 +109,18 @@ static LogicalResult getBackwardSliceImpl(Operation *op, if (parentOp && backwardSlice->count(parentOp) == 0) { if (parentOp->getNumRegions() == 1 && llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())) { - succeeded &= getBackwardSliceImpl(parentOp, backwardSlice, options) - .succeeded(); + return getBackwardSliceImpl(parentOp, backwardSlice, options); } else { - succeeded = false; + return failure(); } } } else { - llvm_unreachable("No definingOp and not a block argument."); + return failure() } }; + bool succeeded = true; + if (!options.omitUsesFromAbove) { llvm::for_each(op->getRegions(), [&](Region ®ion) { // Walk this region recursively to collect the regions that descend from @@ -132,8 +131,11 @@ static LogicalResult getBackwardSliceImpl(Operation *op, region.walk([&](Operation *op) { for (OpOperand &operand : op->getOpOperands()) { if (!descendents.contains(operand.get().getParentRegion())) - processValue(operand.get()); + if (!processValue(operand.get()).succeeded()) { + return WalkResult::interrupt(); + } } + return WalkResult::advance(); }); }); } From cec29a92d0b992c91edc1f147bf836c6cd0c3a18 Mon Sep 17 00:00:00 2001 From: "William S. Moses" Date: Thu, 22 May 2025 12:09:26 -0500 Subject: [PATCH 4/6] fmt --- mlir/include/mlir/Analysis/SliceAnalysis.h | 4 ++-- mlir/lib/Analysis/SliceAnalysis.cpp | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h index e4212e97c6ce3..d082d2d9f758b 100644 --- a/mlir/include/mlir/Analysis/SliceAnalysis.h +++ b/mlir/include/mlir/Analysis/SliceAnalysis.h @@ -138,8 +138,8 @@ void getForwardSlice(Value root, SetVector *forwardSlice, /// Assuming all local orders match the numbering order: /// {1, 2, 5, 3, 4, 6} /// -/// This function returns whether the backwards slice was able to be successfully -/// computed, and failure if it was unable to determine the slice. +/// This function returns whether the backwards slice was able to be +/// successfully computed, and failure if it was unable to determine the slice. LogicalResult getBackwardSlice(Operation *op, SetVector *backwardSlice, const BackwardSliceOptions &options = {}); diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp index 976a63ac32448..e9cb5a4270a65 100644 --- a/mlir/lib/Analysis/SliceAnalysis.cpp +++ b/mlir/lib/Analysis/SliceAnalysis.cpp @@ -96,7 +96,7 @@ static LogicalResult getBackwardSliceImpl(Operation *op, if (auto *definingOp = value.getDefiningOp()) { if (backwardSlice->count(definingOp) == 0) return getBackwardSliceImpl(definingOp, backwardSlice, options) - .succeeded(); + .succeeded(); } else if (auto blockArg = dyn_cast(value)) { if (options.omitBlockArguments) return success(); @@ -132,10 +132,10 @@ static LogicalResult getBackwardSliceImpl(Operation *op, for (OpOperand &operand : op->getOpOperands()) { if (!descendents.contains(operand.get().getParentRegion())) if (!processValue(operand.get()).succeeded()) { - return WalkResult::interrupt(); - } + return WalkResult::interrupt(); + } } - return WalkResult::advance(); + return WalkResult::advance(); }); }); } From a16ebc5bf60a4f3a1cf61b0fe8c978aaf57d0782 Mon Sep 17 00:00:00 2001 From: "William S. Moses" Date: Thu, 22 May 2025 12:13:50 -0500 Subject: [PATCH 5/6] fix --- mlir/lib/Analysis/SliceAnalysis.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp index e9cb5a4270a65..cee52980f713d 100644 --- a/mlir/lib/Analysis/SliceAnalysis.cpp +++ b/mlir/lib/Analysis/SliceAnalysis.cpp @@ -95,8 +95,7 @@ static LogicalResult getBackwardSliceImpl(Operation *op, auto processValue = [&](Value value) { if (auto *definingOp = value.getDefiningOp()) { if (backwardSlice->count(definingOp) == 0) - return getBackwardSliceImpl(definingOp, backwardSlice, options) - .succeeded(); + return getBackwardSliceImpl(definingOp, backwardSlice, options); } else if (auto blockArg = dyn_cast(value)) { if (options.omitBlockArguments) return success(); @@ -110,13 +109,10 @@ static LogicalResult getBackwardSliceImpl(Operation *op, if (parentOp->getNumRegions() == 1 && llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())) { return getBackwardSliceImpl(parentOp, backwardSlice, options); - } else { - return failure(); } } - } else { - return failure() } + return failure(); }; bool succeeded = true; From 3ff609bab52b6852d62b61dbe03e1aec3f84c64f Mon Sep 17 00:00:00 2001 From: "William S. Moses" Date: Thu, 22 May 2025 12:18:19 -0500 Subject: [PATCH 6/6] fmt --- mlir/lib/Analysis/SliceAnalysis.cpp | 6 +++--- mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp index cee52980f713d..12b9d3adb49fa 100644 --- a/mlir/lib/Analysis/SliceAnalysis.cpp +++ b/mlir/lib/Analysis/SliceAnalysis.cpp @@ -141,9 +141,9 @@ static LogicalResult getBackwardSliceImpl(Operation *op, return success(succeeded); } -LogicalResult -mlir::getBackwardSlice(Operation *op, SetVector *backwardSlice, - const BackwardSliceOptions &options) { +LogicalResult mlir::getBackwardSlice(Operation *op, + SetVector *backwardSlice, + const BackwardSliceOptions &options) { LogicalResult result = getBackwardSliceImpl(op, backwardSlice, options); if (!options.inclusive) { diff --git a/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp b/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp index 77496be8967a1..145acd99e6616 100644 --- a/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp +++ b/mlir/test/lib/Dialect/Affine/TestVectorizationUtils.cpp @@ -154,7 +154,8 @@ void VectorizerTestPass::testBackwardSlicing(llvm::raw_ostream &outs) { patternTestSlicingOps().match(f, &matches); for (auto m : matches) { SetVector backwardSlice; - LogicalResult result = getBackwardSlice(m.getMatchedOperation(), &backwardSlice); + LogicalResult result = + getBackwardSlice(m.getMatchedOperation(), &backwardSlice); assert(result.succeeded() && "expected a backward slice"); outs << "\nmatched: " << *m.getMatchedOperation() << " backward static slice: ";