[MLIR][SliceAnalysis] Add comment and restore failure condition

[wsmoses]

Restores the failure accidentally removed in #142076 and adds the comment requested in #140961 (comment)

[llvmbot]

@llvm/pr-subscribers-mlir

Author: William Moses (wsmoses)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/142223.diff

2 Files Affected:

• (modified) mlir/include/mlir/Analysis/SliceAnalysis.h (+2)
• (modified) mlir/lib/Analysis/SliceAnalysis.cpp (+2)

diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h
index d082d2d9f758b..dc0d56d6b753d 100644
--- a/mlir/include/mlir/Analysis/SliceAnalysis.h
+++ b/mlir/include/mlir/Analysis/SliceAnalysis.h
@@ -140,6 +140,8 @@ void getForwardSlice(Value root, SetVector<Operation *> *forwardSlice,
 ///
 /// 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 will presently return failure if a value to process is a blockargument
+/// whose parent op has more than region or more than one block.
 LogicalResult getBackwardSlice(Operation *op,
                                SetVector<Operation *> *backwardSlice,
                                const BackwardSliceOptions &options = {});
diff --git a/mlir/lib/Analysis/SliceAnalysis.cpp b/mlir/lib/Analysis/SliceAnalysis.cpp
index 4cdf47a405c01..20ba558507f4e 100644
--- a/mlir/lib/Analysis/SliceAnalysis.cpp
+++ b/mlir/lib/Analysis/SliceAnalysis.cpp
@@ -109,6 +109,8 @@ static LogicalResult getBackwardSliceImpl(Operation *op,
         if (parentOp->getNumRegions() == 1 &&
             llvm::hasSingleElement(parentOp->getRegion(0).getBlocks())) {
           return getBackwardSliceImpl(parentOp, backwardSlice, options);
+        } else {
+          return failure();
         }
       }
     } else {

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions h,cpp -- mlir/include/mlir/Analysis/SliceAnalysis.h mlir/lib/Analysis/SliceAnalysis.cpp

View the diff from clang-format here.

diff --git a/mlir/include/mlir/Analysis/SliceAnalysis.h b/mlir/include/mlir/Analysis/SliceAnalysis.h
index b6efdcee3..2f9ed1cba 100644
--- a/mlir/include/mlir/Analysis/SliceAnalysis.h
+++ b/mlir/include/mlir/Analysis/SliceAnalysis.h
@@ -140,8 +140,9 @@ void getForwardSlice(Value root, SetVector<Operation *> *forwardSlice,
 ///
 /// 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 will presently return failure if a value to process is a blockargument
-/// whose parent op has more than one region, or a region with more than one block.
+/// This function will presently return failure if a value to process is a
+/// blockargument whose parent op has more than one region, or a region with
+/// more than one block.
 LogicalResult getBackwardSlice(Operation *op,
                                SetVector<Operation *> *backwardSlice,
                                const BackwardSliceOptions &options = {});

[WillFroom]

Thanks!

[joker-eph]

Can this be tested?

[benvanik]

I just noticed a warning in this code that would be worth checking here, as I think this is still broken:

[310/1153] Building CXX object llvm-project\tools\mlir\lib\Analysis\CMakeFiles\obj.MLIRAnalysis.dir\SliceAnalysis.cpp.obj
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.42.34431\include\algorithm(437): warning C4834: discarding return value of function with [[nodiscard]] attribute
C:\Program Files\Microsoft Visual Studio\2022\Preview\VC\Tools\MSVC\14.42.34431\include\algorithm(437): note: the template instantiation context (the oldest one first) is
D:\Dev\iree\third_party\llvm-project\mlir\lib\Analysis\SliceAnalysis.cpp(140): note: see reference to function template instantiation 'UnaryFunction llvm::for_each<mlir::Operation::operand_range,getBackwardSliceImpl::<lambda_1>>(R &&,UnaryFunction)' being compiled

processValue (https://github.com/iree-org/llvm-project/blob/587d6fcbb685e3a57803110695a1996ac895d8b8/mlir/lib/Analysis/SliceAnalysis.cpp#L95) returns a LogicalResult, but the llvm::for_each doesn't use the LogicalResult: https://github.com/iree-org/llvm-project/blob/587d6fcbb685e3a57803110695a1996ac895d8b8/mlir/lib/Analysis/SliceAnalysis.cpp#L140

The error handing in getBackwardSliceImpl's processValue is totally ignored it seems? bool succeeded = true; -> return success(succeeded); - nothing ever changes succeeded.