In6a8dde04a0, it changes the method to return LogicalFailure, so callers can handle the failure instead of crashing, if I read the intention correctly. However, it changes the behavior of the implementation; it breaks several integratino tests in downstream projects (e.g., IREE). Before the change, processValue does not treat it as a failure if the check below TODO has a false condition. However, with the new change, it starts treating it as a failure. The revision updates the final `else` branch (i.e., `llvm_unreachable` line) to return a failure, and return success at the end; the behavior is recovered. ```cpp auto processValue = [&](Value value) { if (auto *definingOp = value.getDefiningOp()) { if (backwardSlice->count(definingOp) == 0) getBackwardSliceImpl(definingOp, backwardSlice, options); } else if (auto blockArg = dyn_cast<BlockArgument>(value)) { if (options.omitBlockArguments) return; Block *block = blockArg.getOwner(); Operation *parentOp = block->getParentOp(); // TODO: determine whether we want to recurse backward into the other // 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); } } else { llvm_unreachable("No definingOp and not a block argument."); } ``` No additional tests are added, like the previous commit. This revision is mostly a post-fix for6a8dde04a0Co-authored-by: Ian Wood <ianwood2024@u.northwestern.edu> Signed-off-by: hanhanW <hanhan0912@gmail.com>
12 KiB
12 KiB