Fix Unchecked errors in Partitioner #3505 (#3511)

Summary:
Fix unchecked errors in Partitioner, rewrite the way to report an error.
#3505
Documentation:

[Optional Fixes]
Pull Request resolved: #3511

Test Plan:
ninja test.
Please see a detailed explanation of how to fill out the fields in the relevant sections in PULL_REQUEST.md.

Differential Revision: D17403326

Pulled By: beicy

fbshipit-source-id: fbab4dbf1d235e148e40b344665459533f60fdde

Author: Man Wang

include/glow/Support/Error.h

@@ -69,6 +69,8 @@ class GlowErr final : public llvm::ErrorInfo<GlowErr> {
     MODEL_LOADER_UNSUPPORTED_ONNX_VERSION,
     // Model loader encountered an invalid protobuf.
     MODEL_LOADER_INVALID_PROTOBUF,
+    // Partitioner error.
+    PARTITIONER_ERROR,
     // Runtime error, out of device memory.
     RUNTIME_ERROR,
     // Runtime error, out of device memory.
@@ -148,6 +150,8 @@ class GlowErr final : public llvm::ErrorInfo<GlowErr> {
       return "MODEL_LOADER_UNSUPPORTED_ONNX_VERSION";
     case ErrorCode::MODEL_LOADER_INVALID_PROTOBUF:
       return "MODEL_LOADER_INVALID_PROTOBUF";
+    case ErrorCode::PARTITIONER_ERROR:
+      return "PARTITIONER_ERROR";
     case ErrorCode::RUNTIME_ERROR:
       return "RUNTIME_ERROR";
     case ErrorCode::RUNTIME_OUT_OF_DEVICE_MEMORY:

lib/Partitioner/Partitioner.cpp

@@ -77,9 +77,11 @@ llvm::Error Partitioner::finalize(const DAGListTy &partitions,
 
   // Validate the functions after partitioning.
   for (Function *subF : module_->getFunctions()) {
-    RETURN_ERR_IF_NOT(subF->verify(),
-                      strFormat("Conversion led to invalid function: %s",
-                                subF->getName().str().data()));
+    if (!subF->verify()) {
+      return MAKE_ERR(GlowErr::ErrorCode::PARTITIONER_ERROR,
+                      "Conversion led to invalid function " +
+                          subF->getName().str());
+    }
   }
 
   if (logPartition) {
@@ -94,8 +96,10 @@ llvm::Error Partitioner::finalize(const DAGListTy &partitions,
   if (dumpPartition) {
     for (const auto &node : partitions[0].nodes) {
       Function *subF = module_->getFunction(node->name);
-      RETURN_ERR_IF_NOT(
-          subF, strFormat("Invalid function name %s.", node->name.data()));
+      if (!subF) {
+        return MAKE_ERR(GlowErr::ErrorCode::PARTITIONER_ERROR,
+                        "Invalid function name " + node->name);
+      }
       subF->dumpDAG("partitionLogicalID" +
                     std::to_string(node->logicalDevices[0]) + "__" +
                     subF->getName().str() + "__" + node->backendName + ".dot");
@@ -284,8 +288,10 @@ llvm::Expected<DAGListTy> Partitioner::backendBasedPartition(
         break;
       }
     }
-    RETURN_ERR_IF_NOT(nodeToBackendName.find(&N) != nodeToBackendName.end(),
+    if (nodeToBackendName.find(&N) == nodeToBackendName.end()) {
+      return MAKE_ERR(GlowErr::ErrorCode::PARTITIONER_ERROR,
                       "Node is not supported by any of the provided backends");
+    }
   }
 
   BFSLevel bfs = getBFSLevel(F);
@@ -427,11 +433,13 @@ llvm::Expected<DAGListTy> Partitioner::createDAGWithoutPartition(
 llvm::Expected<DAGListTy>
 Partitioner::loadBalancedPartition(CompilationContext &cctx,
                                    size_t numDevices) {
-  RETURN_ERR_IF_NOT(
-      module_->getFunctions().size() == 1,
-      strFormat("Invalid : %lu functions in a module. Now in load-balanced "
-                "partition flow, the module can only contain 1 function",
-                module_->getFunctions().size()));
+  if (module_->getFunctions().size() != 1) {
+    return MAKE_ERR(
+        GlowErr::ErrorCode::PARTITIONER_ERROR,
+        strFormat("Invalid : %lu functions in a module. Now in load-balanced "
+                  "partition flow, the module can only contain 1 function",
+                  module_->getFunctions().size()));
+  }
 
   if (multiBackendNames_) {
     VLOG(1) << "For multi backend types, load-balanced partition can't be "
@@ -570,8 +578,10 @@ Partitioner::loadBalancedPartition(CompilationContext &cctx,
       }
 
       // Throw error if we were not able to put this node into any partition
-      RETURN_ERR_IF_NOT(curPartition < numDevices,
+      if (curPartition >= numDevices) {
+        return MAKE_ERR(GlowErr::ErrorCode::PARTITIONER_ERROR,
                         "Load balance partition error");
+      }
     }
   }
   for (size_t i = 0; i < numDevices; i++) {
@@ -601,12 +611,14 @@ llvm::Expected<DAGListTy>
 Partitioner::quantizationProfilingPartition(CompilationContext &cctx) {
   // For quantization profiling flow, currently we assume there is only 1
   // function in a module.
-  RETURN_ERR_IF_NOT(
-      module_->getFunctions().size() == 1,
-      strFormat(
-          "Invalid : %lu functions in a module. In quantization profiling "
-          "partition flow, the module can only contain 1 function",
-          module_->getFunctions().size()));
+  if (module_->getFunctions().size() != 1) {
+    return MAKE_ERR(
+        GlowErr::ErrorCode::PARTITIONER_ERROR,
+        strFormat(
+            "Invalid : %lu functions in a module. In quantization profiling "
+            "partition flow, the module can only contain 1 function",
+            module_->getFunctions().size()));
+  }
 
   // Quantization profiling flow is run under CPU backend, so we don't really
   // need the concrete partition. The backendBasedPartition is necessary since
@@ -668,19 +680,24 @@ Partitioner::heterogeneousPartition(CompilationContext &cctx) {
     }
     // NOTE: the following error detection will be removed once multi-functions
     // in a module is supported.
-    RETURN_ERR_IF_NOT(
-        module_->getFunctions().size() == 1,
-        strFormat("Invalid : %lu functions in a module. Now in heterogeneous "
-                  "partition flow, the module can only contain 1 function",
-                  module_->getFunctions().size()));
+    if (module_->getFunctions().size() != 1) {
+      return MAKE_ERR(
+          GlowErr::ErrorCode::PARTITIONER_ERROR,
+          strFormat("Invalid : %lu functions in a module. Now in heterogeneous "
+                    "partition flow, the module can only contain 1 function",
+                    module_->getFunctions().size()));
+    }
   } else {
     // NOTE: the following error detection will be removed once multi-functions
     // in a module is supported.
-    RETURN_ERR_IF_NOT(
-        module_->getFunctions().size() == 1,
-        strFormat("Invalid : %lu functions in a module. Now in heterogeneous "
-                  "partition flow, the module can only contain 1 function",
-                  module_->getFunctions().size()));
+    if (module_->getFunctions().size() != 1) {
+      return MAKE_ERR(
+          GlowErr::ErrorCode::PARTITIONER_ERROR,
+          strFormat(
+              "Invalid : %lu functions in a module. Now in heterogeneous partition\
+ flow, the module can only contain 1 function",
+              module_->getFunctions().size()));
+    }
     ASSIGN_VALUE_OR_RETURN_ERR(
         partitions, backendBasedPartition(funcToBackend, F_, backends, cctx));
     module_->eraseFunction(F_);
@@ -750,8 +767,11 @@ Partitioner::partitionFromConfig(const PartitionConfig &partitionConfig) {
   std::vector<Backend *> backends;
   genBackendMap(backendMap_, backendHolder, backends);
   Function *F = module_->getFunction(partitionConfig.funcName);
-  RETURN_ERR_IF_NOT(F, strFormat("Can't find function %s in current module.",
-                                 F->getName().str().data()));
+  if (!F) {
+    return MAKE_ERR(GlowErr::ErrorCode::PARTITIONER_ERROR,
+                    strFormat("Can't find function %s in current module.",
+                              F->getName().str().data()));
+  }
 
   DCHECK(
       partitionConfig.numOfPartitions == partitionConfig.backendNames.size() &&

lib/Runtime/HostManager/HostManager.cpp

@@ -187,9 +187,11 @@ llvm::Error HostManager::addNetwork(std::unique_ptr<Module> module,
   if (cctx.precisionConfig.quantMode == QuantizationMode::Profile) {
     // Since for profiling the provisioner will be reset, we only allow one
     // network in one HM.
-    RETURN_ERR_IF_NOT(networks_.size() == 0,
+    if (networks_.size() > 0) {
+      return MAKE_ERR(GlowErr::ErrorCode::RUNTIME_ERROR,
                       "For quantization profiling flow, there can't be other "
                       "registered networks before this one");
+    }
     // For profiling, we use CPU backend. Overwrite Provisioner and Executor
     // to force the network is compiled and run in profilingBackend. backend.
     size_t devicesNum = devices_.size();

tests/unittests/PartitionerTest.cpp

@@ -250,6 +250,7 @@ TEST_F(PartitionerTest, Basic2) {
   Partitioner myPartitioner(&EEP.getModule(), devices, /* saturateHost */ true);
   CompilationContext cctx;
   auto dagList = myPartitioner.partition(cctx);
+  EXPECT_TRUE((bool)dagList);
   EXPECT_EQ(EEP.getModule().getFunctions().size(), 2);
   EXPECT_EQ(dagList->size(), 1);
   ASSERT_TRUE(checkSaveNode(EEP.getModule()));
@@ -343,7 +344,7 @@ TEST_F(PartitionerTest, Error1) {
   Partitioner myPartitioner(&EEP.getModule(), devices);
   CompilationContext cctx;
   auto dagList = myPartitioner.partition(cctx);
-  EXPECT_FALSE((bool)dagList);
+  EXPECT_TRUE(glow::errToBool(dagList.takeError()));
 }
 
 /// This one tests the roofline computed with compute, memory and
@@ -890,7 +891,7 @@ TEST_F(PartitionerTest, memoryUsageValidation1) {
   Partitioner myPartitioner(&mod_, devices);
   CompilationContext cctx;
   auto dagList = myPartitioner.partition(cctx);
-  EXPECT_FALSE((bool)dagList);
+  EXPECT_TRUE(glow::errToBool(dagList.takeError()));
 }
 
 /// This one test dagValidation in partitioner : p1->p2, p2->p1.
@@ -919,7 +920,7 @@ TEST_F(PartitionerTest, dagValidation1) {
   auto partitioner = Partitioner(&mod_, devices, false, false, partitionConfig);
   CompilationContext cctx;
   auto dagList = partitioner.partition(cctx);
-  EXPECT_FALSE((bool)dagList);
+  EXPECT_TRUE(glow::errToBool(dagList.takeError()));
 }
 
 /// This one test dagValidation in partitioner: p0->p1, p1->p2, p2->p1.
@@ -951,7 +952,7 @@ TEST_F(PartitionerTest, dagValidation2) {
   auto partitioner = Partitioner(&mod_, devices, false, false, partitionConfig);
   CompilationContext cctx;
   auto dagList = partitioner.partition(cctx);
-  EXPECT_FALSE((bool)dagList);
+  EXPECT_TRUE(glow::errToBool(dagList.takeError()));
 }
 
 /// This one tests partition from a user-defined config.