[mlir] Mark LogicalResult as LLVM_NODISCARD

This makes ignoring a result explicit by the user, and helps to prevent accidental errors with dropped results. Marking LogicalResult as no discard was always the intention from the beginning, but got lost along the way.

Differential Revision: https://reviews.llvm.org/D95841

Author: River707

mlir/include/mlir/IR/OpDefinition.h

@@ -41,7 +41,7 @@ class ParseResult : public LogicalResult {
   ParseResult(const Diagnostic &) : LogicalResult(failure()) {}
 
   /// Failure is true in a boolean context.
-  explicit operator bool() const { return failed(*this); }
+  explicit operator bool() const { return failed(); }
 };
 /// This class implements `Optional` functionality for ParseResult. We don't
 /// directly use Optional here, because it provides an implicit conversion

mlir/include/mlir/IR/SymbolTable.h

@@ -189,17 +189,17 @@ class SymbolTable {
   /// 'from'. This does not traverse into any nested symbol tables. If there are
   /// any unknown operations that may potentially be symbol tables, no uses are
   /// replaced and failure is returned.
-  LLVM_NODISCARD static LogicalResult replaceAllSymbolUses(StringRef oldSymbol,
-                                                           StringRef newSymbol,
-                                                           Operation *from);
-  LLVM_NODISCARD static LogicalResult
-  replaceAllSymbolUses(Operation *oldSymbol, StringRef newSymbolName,
-                       Operation *from);
-  LLVM_NODISCARD static LogicalResult
-  replaceAllSymbolUses(StringRef oldSymbol, StringRef newSymbol, Region *from);
-  LLVM_NODISCARD static LogicalResult
-  replaceAllSymbolUses(Operation *oldSymbol, StringRef newSymbolName,
-                       Region *from);
+  static LogicalResult replaceAllSymbolUses(StringRef oldSymbol,
+                                            StringRef newSymbol,
+                                            Operation *from);
+  static LogicalResult replaceAllSymbolUses(Operation *oldSymbol,
+                                            StringRef newSymbolName,
+                                            Operation *from);
+  static LogicalResult replaceAllSymbolUses(StringRef oldSymbol,
+                                            StringRef newSymbol, Region *from);
+  static LogicalResult replaceAllSymbolUses(Operation *oldSymbol,
+                                            StringRef newSymbolName,
+                                            Region *from);
 
 private:
   Operation *symbolTableOp;

mlir/include/mlir/Pass/PassManager.h

@@ -178,7 +178,6 @@ class PassManager : public OpPassManager {
   /// Run the passes within this manager on the provided operation. The
   /// specified operation must have the same name as the one provided the pass
   /// manager on construction.
-  LLVM_NODISCARD
   LogicalResult run(Operation *op);
 
   /// Return an instance of the context.

mlir/include/mlir/Support/LogicalResult.h

@@ -14,38 +14,62 @@
 
 namespace mlir {
 
-/// Values that can be used to signal success/failure. This should be used in
-/// conjunction with the utility functions below.
-struct LogicalResult {
-  enum ResultEnum { Success, Failure } value;
-  LogicalResult(ResultEnum v) : value(v) {}
+/// This class represents an efficient way to signal success or failure. It
+/// should be preferred over the use of `bool` when appropriate, as it avoids
+/// all of the ambiguity that arises in interpreting a boolean result. This
+/// class is marked as NODISCARD to ensure that the result is processed. Users
+/// may explicitly discard a result by using `(void)`, e.g.
+/// `(void)functionThatReturnsALogicalResult();`. Given the intended nature of
+/// this class, it generally shouldn't be used as the result of functions that
+/// very frequently have the result ignored. This class is intended to be used
+/// in conjunction with the utility functions below.
+struct LLVM_NODISCARD LogicalResult {
+public:
+  /// If isSuccess is true a `success` result is generated, otherwise a
+  /// 'failure' result is generated.
+  static LogicalResult success(bool isSuccess = true) {
+    return LogicalResult(isSuccess);
+  }
+
+  /// If isFailure is true a `failure` result is generated, otherwise a
+  /// 'success' result is generated.
+  static LogicalResult failure(bool isFailure = true) {
+    return success(!isFailure);
+  }
+
+  /// Returns true if the provided LogicalResult corresponds to a success value.
+  bool succeeded() const { return isSuccess; }
+
+  /// Returns true if the provided LogicalResult corresponds to a failure value.
+  bool failed() const { return !succeeded(); }
+
+private:
+  LogicalResult(bool isSuccess) : isSuccess(isSuccess) {}
+
+  /// Boolean indicating if this is a success result, if false this is a
+  /// failure result.
+  bool isSuccess;
 };
 
 /// Utility function to generate a LogicalResult. If isSuccess is true a
 /// `success` result is generated, otherwise a 'failure' result is generated.
 inline LogicalResult success(bool isSuccess = true) {
-  return LogicalResult{isSuccess ? LogicalResult::Success
-                                 : LogicalResult::Failure};
+  return LogicalResult::success(isSuccess);
 }
 
 /// Utility function to generate a LogicalResult. If isFailure is true a
 /// `failure` result is generated, otherwise a 'success' result is generated.
 inline LogicalResult failure(bool isFailure = true) {
-  return LogicalResult{isFailure ? LogicalResult::Failure
-                                 : LogicalResult::Success};
+  return LogicalResult::failure(isFailure);
 }
 
 /// Utility function that returns true if the provided LogicalResult corresponds
 /// to a success value.
-inline bool succeeded(LogicalResult result) {
-  return result.value == LogicalResult::Success;
-}
+inline bool succeeded(LogicalResult result) { return result.succeeded(); }
 
 /// Utility function that returns true if the provided LogicalResult corresponds
 /// to a failure value.
-inline bool failed(LogicalResult result) {
-  return result.value == LogicalResult::Failure;
-}
+inline bool failed(LogicalResult result) { return result.failed(); }
 
 /// This class provides support for representing a failure result, or a valid
 /// value of type `T`. This allows for integrating with LogicalResult, while

mlir/include/mlir/Transforms/DialectConversion.h

@@ -829,11 +829,11 @@ class ConversionTarget {
 /// legalizable to the given `target` are placed within that set. (Note that if
 /// there is an op explicitly marked as illegal, the conversion terminates and
 /// the `unconvertedOps` set will not necessarily be complete.)
-LLVM_NODISCARD LogicalResult
+LogicalResult
 applyPartialConversion(ArrayRef<Operation *> ops, ConversionTarget &target,
                        const FrozenRewritePatternList &patterns,
                        DenseSet<Operation *> *unconvertedOps = nullptr);
-LLVM_NODISCARD LogicalResult
+LogicalResult
 applyPartialConversion(Operation *op, ConversionTarget &target,
                        const FrozenRewritePatternList &patterns,
                        DenseSet<Operation *> *unconvertedOps = nullptr);
@@ -842,12 +842,11 @@ applyPartialConversion(Operation *op, ConversionTarget &target,
 /// operations. This method returns failure if the conversion of any operation
 /// fails, or if there are unreachable blocks in any of the regions nested
 /// within 'ops'.
-LLVM_NODISCARD LogicalResult
-applyFullConversion(ArrayRef<Operation *> ops, ConversionTarget &target,
-                    const FrozenRewritePatternList &patterns);
-LLVM_NODISCARD LogicalResult
-applyFullConversion(Operation *op, ConversionTarget &target,
-                    const FrozenRewritePatternList &patterns);
+LogicalResult applyFullConversion(ArrayRef<Operation *> ops,
+                                  ConversionTarget &target,
+                                  const FrozenRewritePatternList &patterns);
+LogicalResult applyFullConversion(Operation *op, ConversionTarget &target,
+                                  const FrozenRewritePatternList &patterns);
 
 /// Apply an analysis conversion on the given operations, and all nested
 /// operations. This method analyzes which operations would be successfully
@@ -857,14 +856,13 @@ applyFullConversion(Operation *op, ConversionTarget &target,
 /// operations on success and only pre-existing operations are added to the set.
 /// This method only returns failure if there are unreachable blocks in any of
 /// the regions nested within 'ops'.
-LLVM_NODISCARD LogicalResult
-applyAnalysisConversion(ArrayRef<Operation *> ops, ConversionTarget &target,
-                        const FrozenRewritePatternList &patterns,
-                        DenseSet<Operation *> &convertedOps);
-LLVM_NODISCARD LogicalResult
-applyAnalysisConversion(Operation *op, ConversionTarget &target,
-                        const FrozenRewritePatternList &patterns,
-                        DenseSet<Operation *> &convertedOps);
+LogicalResult applyAnalysisConversion(ArrayRef<Operation *> ops,
+                                      ConversionTarget &target,
+                                      const FrozenRewritePatternList &patterns,
+                                      DenseSet<Operation *> &convertedOps);
+LogicalResult applyAnalysisConversion(Operation *op, ConversionTarget &target,
+                                      const FrozenRewritePatternList &patterns,
+                                      DenseSet<Operation *> &convertedOps);
 } // end namespace mlir
 
 #endif // MLIR_TRANSFORMS_DIALECTCONVERSION_H_

mlir/include/mlir/Transforms/LoopUtils.h

@@ -85,7 +85,6 @@ void promoteSingleIterationLoops(FuncOp f);
 /// order, and are multiplied by the loop 'step' before being applied. If
 /// `unrollPrologueEpilogue` is set, fully unroll the prologue and epilogue
 /// loops when possible.
-LLVM_NODISCARD
 LogicalResult affineForOpBodySkew(AffineForOp forOp, ArrayRef<uint64_t> shifts,
                                   bool unrollPrologueEpilogue = false);
 
@@ -97,7 +96,6 @@ void getTileableBands(FuncOp f,
 
 /// Tiles the specified band of perfectly nested loops creating tile-space loops
 /// and intra-tile loops. A band is a contiguous set of loops.
-LLVM_NODISCARD
 LogicalResult
 tilePerfectlyNested(MutableArrayRef<AffineForOp> input,
                     ArrayRef<unsigned> tileSizes,
@@ -106,7 +104,6 @@ tilePerfectlyNested(MutableArrayRef<AffineForOp> input,
 /// Tiles the specified band of perfectly nested loops creating tile-space
 /// loops and intra-tile loops, using SSA values as tiling parameters. A band
 /// is a contiguous set of loops.
-LLVM_NODISCARD
 LogicalResult tilePerfectlyNestedParametric(
     MutableArrayRef<AffineForOp> input, ArrayRef<Value> tileSizes,
     SmallVectorImpl<AffineForOp> *tiledNest = nullptr);

mlir/lib/Analysis/Presburger/Simplex.cpp

@@ -272,7 +272,7 @@ LogicalResult Simplex::restoreRow(Unknown &u) {
 
     pivot(*maybePivot);
     if (u.orientation == Orientation::Column)
-      return LogicalResult::Success; // the unknown is unbounded above.
+      return success(); // the unknown is unbounded above.
   }
   return success(tableau(u.pos, 1) >= 0);
 }
@@ -509,7 +509,7 @@ Optional<Fraction> Simplex::computeOptimum(Direction direction, Unknown &u) {
   Optional<Fraction> optimum = computeRowOptimum(direction, row);
   if (u.restricted && direction == Direction::Down &&
       (!optimum || *optimum < Fraction(0, 1)))
-    restoreRow(u);
+    (void)restoreRow(u);
   return optimum;
 }
 
@@ -572,7 +572,7 @@ void Simplex::detectRedundant() {
     if (!minimum || *minimum < Fraction(0, 1)) {
       // Constraint is unbounded below or can attain negative sample values and
       // hence is not redundant.
-      restoreRow(u);
+      (void)restoreRow(u);
       continue;
     }
 

mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp

@@ -379,7 +379,8 @@ static Value getIdentityValue(AtomicRMWKind op, OpBuilder &builder,
     return builder.create<ConstantOp>(loc, builder.getI32IntegerAttr(1));
   // TODO: Add remaining reduction operations.
   default:
-    emitOptionalError(loc, "Reduction operation type not supported");
+    (void)emitOptionalError(loc, "Reduction operation type not supported");
+    break;
   }
   return nullptr;
 }
@@ -399,7 +400,8 @@ static Value getReductionOp(AtomicRMWKind op, OpBuilder &builder, Location loc,
     return builder.create<MulIOp>(loc, lhs, rhs);
   // TODO: Add remaining reduction operations.
   default:
-    emitOptionalError(loc, "Reduction operation type not supported");
+    (void)emitOptionalError(loc, "Reduction operation type not supported");
+    break;
   }
   return nullptr;
 }

mlir/lib/Conversion/GPUToNVVM/LowerGpuOpsToNVVMOps.cpp

@@ -128,7 +128,7 @@ struct LowerGpuOpsToNVVMOpsPass
     // which need to be lowered further, which is not supported by a single
     // conversion pass.
     populateGpuRewritePatterns(m.getContext(), patterns);
-    applyPatternsAndFoldGreedily(m, std::move(patterns));
+    (void)applyPatternsAndFoldGreedily(m, std::move(patterns));
 
     populateStdToLLVMConversionPatterns(converter, llvmPatterns);
     populateGpuToNVVMConversionPatterns(converter, llvmPatterns);

mlir/lib/Conversion/GPUToROCDL/LowerGpuOpsToROCDLOps.cpp

@@ -62,7 +62,7 @@ struct LowerGpuOpsToROCDLOpsPass
     OwningRewritePatternList patterns, llvmPatterns;
 
     populateGpuRewritePatterns(m.getContext(), patterns);
-    applyPatternsAndFoldGreedily(m, std::move(patterns));
+    (void)applyPatternsAndFoldGreedily(m, std::move(patterns));
 
     populateVectorToLLVMConversionPatterns(converter, llvmPatterns);
     populateVectorToROCDLConversionPatterns(converter, llvmPatterns);

mlir/lib/Conversion/GPUToSPIRV/GPUToSPIRV.cpp

@@ -208,7 +208,8 @@ lowerAsEntryFunction(gpu::GPUFuncOp funcOp, TypeConverter &typeConverter,
     return nullptr;
   rewriter.eraseOp(funcOp);
 
-  spirv::setABIAttrs(newFuncOp, entryPointInfo, argABIInfo);
+  if (failed(spirv::setABIAttrs(newFuncOp, entryPointInfo, argABIInfo)))
+    return nullptr;
   return newFuncOp;
 }
 

mlir/lib/Conversion/SPIRVToLLVM/ConvertLaunchFuncToLLVMCalls.cpp

@@ -295,7 +295,7 @@ class LowerHostCodeToLLVM
     // Finally, modify the kernel function in SPIR-V modules to avoid symbolic
     // conflicts.
     for (auto spvModule : module.getOps<spirv::ModuleOp>())
-      encodeKernelName(spvModule);
+      (void)encodeKernelName(spvModule);
   }
 };
 } // namespace

mlir/lib/Conversion/SPIRVToLLVM/SPIRVToLLVM.cpp

@@ -888,9 +888,9 @@ class LoadStorePattern : public SPIRVToLLVMConversion<SPIRVop> {
                   ConversionPatternRewriter &rewriter) const override {
 
     if (!op.memory_access().hasValue()) {
-      replaceWithLoadOrStore(op, rewriter, this->typeConverter, /*alignment=*/0,
-                             /*isVolatile=*/false, /*isNonTemporal=*/false);
-      return success();
+      return replaceWithLoadOrStore(
+          op, rewriter, this->typeConverter, /*alignment=*/0,
+          /*isVolatile=*/false, /*isNonTemporal=*/false);
     }
     auto memoryAccess = op.memory_access().getValue();
     switch (memoryAccess) {
@@ -902,9 +902,8 @@ class LoadStorePattern : public SPIRVToLLVMConversion<SPIRVop> {
           memoryAccess == spirv::MemoryAccess::Aligned ? *op.alignment() : 0;
       bool isNonTemporal = memoryAccess == spirv::MemoryAccess::Nontemporal;
       bool isVolatile = memoryAccess == spirv::MemoryAccess::Volatile;
-      replaceWithLoadOrStore(op, rewriter, this->typeConverter, alignment,
-                             isVolatile, isNonTemporal);
-      return success();
+      return replaceWithLoadOrStore(op, rewriter, this->typeConverter,
+                                    alignment, isVolatile, isNonTemporal);
     }
     default:
       // There is no support of other memory access attributes.

mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp

@@ -3111,9 +3111,10 @@ struct ReturnOpLowering : public ConvertOpToLLVMPattern<ReturnOp> {
       }
     } else {
       updatedOperands = llvm::to_vector<4>(operands);
-      copyUnrankedDescriptors(rewriter, loc, *getTypeConverter(),
-                              op.getOperands().getTypes(), updatedOperands,
-                              /*toDynamic=*/true);
+      (void)copyUnrankedDescriptors(rewriter, loc, *getTypeConverter(),
+                                    op.getOperands().getTypes(),
+                                    updatedOperands,
+                                    /*toDynamic=*/true);
     }
 
     // If ReturnOp has 0 or 1 operand, create it and return immediately.

mlir/lib/Conversion/StandardToSPIRV/LegalizeStandardForSPIRV.cpp

@@ -214,8 +214,8 @@ void SPIRVLegalization::runOnOperation() {
   OwningRewritePatternList patterns;
   auto *context = &getContext();
   populateStdLegalizationPatternsForSPIRVLowering(context, patterns);
-  applyPatternsAndFoldGreedily(getOperation()->getRegions(),
-                               std::move(patterns));
+  (void)applyPatternsAndFoldGreedily(getOperation()->getRegions(),
+                                     std::move(patterns));
 }
 
 std::unique_ptr<Pass> mlir::createLegalizeStdOpsForSPIRVLoweringPass() {

mlir/lib/Conversion/TosaToLinalg/TosaToLinalg.cpp

@@ -137,7 +137,7 @@ createLinalgBodyCalculationForElementwiseOp(Operation *op, ValueRange args,
   if (isa<tosa::FloorOp>(op) && elementTy.isa<FloatType>())
     return rewriter.create<mlir::FloorFOp>(loc, resultTypes, args);
 
-  rewriter.notifyMatchFailure(
+  (void)rewriter.notifyMatchFailure(
       op, "unhandled op for linalg body calculation for elementwise op");
   return nullptr;
 }

mlir/lib/Conversion/VectorToLLVM/ConvertVectorToLLVMPass.cpp

@@ -61,7 +61,7 @@ void LowerVectorToLLVMPass::runOnOperation() {
     populateVectorToVectorCanonicalizationPatterns(patterns, &getContext());
     populateVectorSlicesLoweringPatterns(patterns, &getContext());
     populateVectorContractLoweringPatterns(patterns, &getContext());
-    applyPatternsAndFoldGreedily(getOperation(), std::move(patterns));
+    (void)applyPatternsAndFoldGreedily(getOperation(), std::move(patterns));
   }
 
   // Convert to the LLVM IR dialect.

mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp

@@ -713,7 +713,7 @@ struct ConvertVectorToSCFPass
     auto *context = getFunction().getContext();
     populateVectorToSCFConversionPatterns(
         patterns, context, VectorTransferToSCFOptions().setUnroll(fullUnroll));
-    applyPatternsAndFoldGreedily(getFunction(), std::move(patterns));
+    (void)applyPatternsAndFoldGreedily(getFunction(), std::move(patterns));
   }
 };
 

mlir/lib/Dialect/Affine/Transforms/AffineDataCopyGeneration.cpp

@@ -155,7 +155,7 @@ AffineDataCopyGeneration::runOnBlock(Block *block,
       if (recurseInner) {
         // We'll recurse and do the copies at an inner level for 'forInst'.
         // Recurse onto the body of this loop.
-        runOnBlock(forOp.getBody(), copyNests);
+        (void)runOnBlock(forOp.getBody(), copyNests);
       } else {
         // We have enough capacity, i.e., copies will be computed for the
         // portion of the block until 'it', and for 'it', which is 'forOp'. Note
@@ -207,7 +207,7 @@ void AffineDataCopyGeneration::runOnFunction() {
   copyNests.clear();
 
   for (auto &block : f)
-    runOnBlock(&block, copyNests);
+    (void)runOnBlock(&block, copyNests);
 
   // Promote any single iteration loops in the copy nests and collect
   // load/stores to simplify.
@@ -217,7 +217,7 @@ void AffineDataCopyGeneration::runOnFunction() {
     // continuation of the walk or the collection of load/store ops.
     nest->walk([&](Operation *op) {
       if (auto forOp = dyn_cast<AffineForOp>(op))
-        promoteIfSingleIteration(forOp);
+        (void)promoteIfSingleIteration(forOp);
       else if (isa<AffineLoadOp, AffineStoreOp>(op))
         copyOps.push_back(op);
     });
@@ -230,5 +230,5 @@ void AffineDataCopyGeneration::runOnFunction() {
   AffineStoreOp::getCanonicalizationPatterns(patterns, &getContext());
   FrozenRewritePatternList frozenPatterns(std::move(patterns));
   for (Operation *op : copyOps)
-    applyOpPatternsAndFold(op, frozenPatterns);
+    (void)applyOpPatternsAndFold(op, frozenPatterns);
 }