[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]

✅ With the latest revision this PR passed the C/C++ code formatter.

[WillFroom]

Thanks!

[joker-eph]

Can this be tested?

[jpienaar]

+1, I was missing one too in the original. Thanks for updating the comment.

So this just fails due to missing support in getBackwardSlice? E.g., its difficult to determine that to slice and semantics of ops in this case and so just bail. An alternative would have been to have the filter function capture this (fail to propagate post out into such ops and record the need/failure, and handle post - less efficient than this early exit but there are just a lot of ignoring of this return now).

[wsmoses]

hm somehow missed these comments/review. rebased the PR.

I'm not quite sure how to test this as the condition here is restore a broken condition from #142076 to ensure the analysis is conservative.

Happy to add a test if you know a good way to add one.

That said given #163749 it may be prudent to add this to restore correctness

[joker-eph]

We're looking for an operation which is not IsolatedFromAbove, have multiple regions or a region with multiple blocks?

[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.