comm: avoid creating invalid intercomm then destroy it

In MPIR_Comm_create_inter, we know whether the remote group is empty
after the exchange, thus it is unnecessary to create and commit the
intercomm then delete it later. Simply don't create it in the first
place.

The device layer is not necessarily equipped to handle intercomm commit
with empty groups.

src/mpi/comm/comm_impl.c

@@ -409,31 +409,6 @@ int MPIR_Comm_create_inter(MPIR_Comm * comm_ptr, MPIR_Group * group_ptr, MPIR_Co
     mpi_errno = MPII_Comm_create_calculate_mapping(group_ptr, comm_ptr, &mapping, &mapping_comm);
     MPIR_ERR_CHECK(mpi_errno);
 
-    *newcomm_ptr = NULL;
-
-    if (group_ptr->rank != MPI_UNDEFINED) {
-        /* Get the new communicator structure and context id */
-        mpi_errno = MPIR_Comm_create(newcomm_ptr);
-        if (mpi_errno)
-            goto fn_fail;
-
-        (*newcomm_ptr)->recvcontext_id = new_context_id;
-        (*newcomm_ptr)->rank = group_ptr->rank;
-        (*newcomm_ptr)->comm_kind = comm_ptr->comm_kind;
-        /* Since the group has been provided, let the new communicator know
-         * about the group */
-        (*newcomm_ptr)->local_comm = 0;
-        (*newcomm_ptr)->local_group = group_ptr;
-        MPIR_Group_add_ref(group_ptr);
-
-        (*newcomm_ptr)->local_size = group_ptr->size;
-        (*newcomm_ptr)->remote_group = 0;
-
-        (*newcomm_ptr)->is_low_group = comm_ptr->is_low_group;
-
-        MPIR_Comm_set_session_ptr(*newcomm_ptr, session_ptr);
-    }
-
     /* There is an additional step.  We must communicate the
      * information on the local context id and the group members,
      * given by the ranks so that the remote process can construct the
@@ -456,9 +431,6 @@ int MPIR_Comm_create_inter(MPIR_Comm * comm_ptr, MPIR_Group * group_ptr, MPIR_Co
                                   rinfo, 2, MPI_INT, 0, 0, comm_ptr, MPI_STATUS_IGNORE,
                                   MPIR_ERR_NONE);
         MPIR_ERR_CHECK(mpi_errno);
-        if (*newcomm_ptr != NULL) {
-            (*newcomm_ptr)->context_id = rinfo[0];
-        }
         remote_size = rinfo[1];
 
         MPIR_CHKLMEM_MALLOC(remote_mapping, int *,
@@ -482,9 +454,7 @@ int MPIR_Comm_create_inter(MPIR_Comm * comm_ptr, MPIR_Group * group_ptr, MPIR_Co
         /* Broadcast to the other members of the local group */
         mpi_errno = MPIR_Bcast(rinfo, 2, MPI_INT, 0, comm_ptr->local_comm, MPIR_ERR_NONE);
         MPIR_ERR_CHECK(mpi_errno);
-        if (*newcomm_ptr != NULL) {
-            (*newcomm_ptr)->context_id = rinfo[0];
-        }
+
         remote_size = rinfo[1];
         MPIR_CHKLMEM_MALLOC(remote_mapping, int *,
                             remote_size * sizeof(int),
@@ -495,10 +465,45 @@ int MPIR_Comm_create_inter(MPIR_Comm * comm_ptr, MPIR_Group * group_ptr, MPIR_Co
     }
 
     MPIR_Assert(remote_size >= 0);
+    if (group_ptr->rank == MPI_UNDEFINED || remote_size <= 0) {
+        /* If we are not part of the group, or -
+         * It's possible that no members of the other side of comm were
+         * members of the group that they passed, which we only know after
+         * receiving/bcasting the remote_size above.  We must return
+         * MPI_COMM_NULL in this case.
+         */
+        MPIR_Free_contextid(new_context_id);
+        *newcomm_ptr = NULL;
+        goto fn_exit;
+    }
+
+    /* FIXME: the branch was kept to minimize line changes. Remove the if-check. */
+    if (group_ptr->rank != MPI_UNDEFINED) {
+        /* Get the new communicator structure and context id */
+        mpi_errno = MPIR_Comm_create(newcomm_ptr);
+        if (mpi_errno)
+            goto fn_fail;
 
+        (*newcomm_ptr)->context_id = rinfo[0];
+        (*newcomm_ptr)->remote_size = rinfo[1];
+        (*newcomm_ptr)->recvcontext_id = new_context_id;
+        (*newcomm_ptr)->rank = group_ptr->rank;
+        (*newcomm_ptr)->comm_kind = comm_ptr->comm_kind;
+        /* Since the group has been provided, let the new communicator know
+         * about the group */
+        (*newcomm_ptr)->local_comm = 0;
+        (*newcomm_ptr)->local_group = group_ptr;
+        MPIR_Group_add_ref(group_ptr);
+
+        (*newcomm_ptr)->local_size = group_ptr->size;
+        (*newcomm_ptr)->remote_group = 0;
+
+        (*newcomm_ptr)->is_low_group = comm_ptr->is_low_group;
+
+        MPIR_Comm_set_session_ptr(*newcomm_ptr, session_ptr);
+    }
 
     if (group_ptr->rank != MPI_UNDEFINED) {
-        (*newcomm_ptr)->remote_size = remote_size;
         /* Now, everyone has the remote_mapping, and can apply that to
          * the network address mapping. */
 
@@ -528,23 +533,6 @@ int MPIR_Comm_create_inter(MPIR_Comm * comm_ptr, MPIR_Group * group_ptr, MPIR_Co
         (*newcomm_ptr)->tainted = comm_ptr->tainted;
         mpi_errno = MPIR_Comm_commit(*newcomm_ptr);
         MPIR_ERR_CHECK(mpi_errno);
-
-        if (remote_size <= 0) {
-            /* It's possible that no members of the other side of comm were
-             * members of the group that they passed, which we only know after
-             * receiving/bcasting the remote_size above.  We must return
-             * MPI_COMM_NULL in this case, but we can't free the newcomm_ptr
-             * immediately after the communication above because
-             * MPIR_Comm_release won't work correctly with a half-constructed
-             * comm. */
-            mpi_errno = MPIR_Comm_release(*newcomm_ptr);
-            MPIR_ERR_CHECK(mpi_errno);
-            *newcomm_ptr = NULL;
-        }
-    } else {
-        /* This process is not in the group */
-        MPIR_Free_contextid(new_context_id);
-        *newcomm_ptr = NULL;
     }
 
   fn_exit: