search

mpi-forum / mpi-forum-historic

Migration of old MPI Forum Trac Tickets to GitHub. New issues belong on mpi-forum/mpi-issues.
http://www.mpi-forum.org
2 stars 3 forks source link

MPI_UNWEIGHTED should not be NULL #294

Closed mpiforumbot closed 8 years ago

mpiforumbot commented 8 years ago

Originally by goodell on 2011-09-15 15:49:54 -0500

Description

The MPI-2.2, p.255 l.38, the standard recommends that in the C bindings the value of MPI_UNWEIGHTED should be NULL. This implementation choice makes the OUT value returned by MPI_Dist_graph_neighbors_count ambiguous for certain graphs.

For example, the implementation is unable to distinguish between these two logically distinct function calls:

/* prototype for your reference */
int MPI_Dist_graph_create(MPI_Comm comm_old, int n, int sources[],
                        int degrees[], int destinations[], int
                        weights[],
                        MPI_Info info, int reorder, MPI_Comm
                        -comm_dist_graph)

/* case 1 */
MPI_Dist_graph_create(MPI_COMM_WORLD, 0, NULL, NULL, NULL, NULL, MPI_INFO_NULL, 0, &comm);

/* case 2 */
MPI_Dist_graph_create(MPI_COMM_WORLD, 0, NULL, NULL, NULL, MPI_UNWEIGHTED, MPI_INFO_NULL, 0, &comm);

Thus NULL is not a reasonable implementation choice for MPI_UNWEIGHTED in the C bindings.

History

None.

Proposed Solution (newest proposal, based on discussion below)

The value constant MPI_UNWEIGHTED may be changed if it is currently implemented as NULL. This is only a recommendation and not mandatory. Therefore, this enhancement is source code and binary (ABI) backward compatible.

The new constant MPI_WEIGHTS_EMPTY must be implemented.

Impact on Applications / Users

None for correct applications. If an application currently implements an empty weight array with NULL and uses an MPI library with MPI_UNWEIGHTED <> NULL then this bug in the application is healed with MPI-3.0.

Alternative Solution (withdrawn)

Requiring MPI_UNWEIGHTED <> NULL and using NULL instead of MPI_WEIGHTS_EMPTY. This would be source code backward compatible, but not ABI backward compatible.

Alternative Solution (former proposal, already formally read, but now withdrawn)

(all page/line numbers are given relative to MPI-2.2)

Page 254, line 2: Delete "In C, one would expect it to be NULL."

Page 255, line 38: Delete "In C, one would expect it to be NULL."

Add the following AtoI after line 39: (in TeX format, based on text suggested by Rolf Rabenseifner)

\begin{implementors}

In the C bindings the constant \const{MPI\_UNWEIGHTED} should not be
implemented with the C constant \ctype{NULL}.  Otherwise the weight
arguments to the distributed graph interface may be ambiguous.

An application may pass \ctype{0} for \mpiarg{n} and \ctype{NULL} for
\mpiarg{sources} in calls to \mpifunc{MPI\_DIST\_GRAPH\_CREATE} on a
process if that process specifies no edges in the topology constructor
call.  Similarly, an application may pass \ctype{0} for
\mpiarg{indegree} and \mpiarg{outdegree} and \ctype{NULL} for
\mpiarg{sourceweights} and \mpiarg{destweights} in calls to
\mpifunc{MPI\_DIST\_GRAPH\_CREATE\_ADJACENT}.  Other processes
participating in the call may still contribute edges with weights to the
graph.

If the graph is unweighted, then the application must pass
\const{MPI\_UNWEIGHTED} at all processes, even those that contribute no
edges locally.

The \MPI/ library must be able to distinguish between the aforementioned
weighted scenarios and scenarios where no edges are contributed locally
yet MPI\_UNWEIGHTED is passed globally.  Furthermore
\mpifunc{MPI\_DIST\_GRAPH\_NEIGHBORS\_COUNT} requires the \MPI/
implementation to distinguish whether or not \const{MPI\_UNWEIGHTED} was
passed when a graph is constructed with zero edges.  Satisfying both
requirements is not possible if \ctype{MPI\_UNWEIGHTED==NULL}.

\end{implementors}

Entry for the Change Log

Section 2.5.4 on page 14, and Section 7.5.4 on page 252.[[BR]] The recommended C implementation value for MPI_UNWEIGHTED was changed from NULL to non-NULL. An additional weight array constant (MPI_WEIGHTS_EMPTY) was introduced.

mpiforumbot commented 8 years ago

Originally by goodell on 2011-09-15 15:53:53 -0500

(add Rolf, Jesper, and Torsten to the CC list)

mpiforumbot commented 8 years ago

Originally by moody20 on 2011-10-25 14:45:10 -0500

When a user calls MPI_DIST_GRAPH_CREATE with n==0, we do not specify what value a user should pass when weights is not MPI_UNWEIGHTED. The user may happen to pick a value equal to MPI_UNWEIGHTED. We should provide another constant that should be specified when n==0 and weights != MPI_UNWEIGHTED.

mpiforumbot commented 8 years ago

Originally by moody20 on 2011-10-25 14:46:11 -0500

Rolf pointed out to me that this constant cannot be NULL, because it won't work for Fortran, so we need something like MPI_WEIGHTS_NULL.

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2011-10-25 15:35:21 -0500

No, it is not needed, because it is expected that the user passes any variable or array address in C or any variable or array in Fortran. The content of the storage is untouched if n==0.

Therefore no special value is needed.

mpiforumbot commented 8 years ago

Originally by goodell on 2012-01-19 14:21:54 -0600

I think Adam has a point here (not sure how I missed this correspondence until now). Rolf's comment is incorrect, since we do attempt to read the storage even if n==0. It might not be a big deal if we only had C bindings (since NULL is very natural for this), but the Fortran bindings cause a problem here.

The best fix would have been a separate boolean parameter for weighted/unweighted, but that's not backwards compatible with MPI-2.2. So something like MPI_WEIGHTS_NULL probably makes more sense. Maybe call it MPI_WEIGHTS_EMPTY to match up better with our other EMPTY/NULL conventions?

Torsten, what do you think about adding MPI_WEIGHTS_NULL/EMPTY? If you're OK with it too then I'll write up the appropriate text instead of what we have in the ticket right now.

Also, we should consider taking this modification out of the ticket process. It seems like exactly the sort of thing that can and should be handled by the chapter committee.

mpiforumbot commented 8 years ago

Originally by htor on 2012-01-21 20:19:53 -0600

Replying to goodell:

I think Adam has a point here (not sure how I missed this correspondence until now). Rolf's comment is incorrect, since we do attempt to read the storage even if n==0. It might not be a big deal if we only had C bindings (since NULL is very natural for this), but the Fortran bindings cause a problem here. Why would we attempt to read the storage if n==0? I would understand that we check for weighted/unweighted? But I see the issue and agree (see below).

The best fix would have been a separate boolean parameter for weighted/unweighted, but that's not backwards compatible with MPI-2.2. So something like MPI_WEIGHTS_NULL probably makes more sense. Maybe call it MPI_WEIGHTS_EMPTY to match up better with our other EMPTY/NULL conventions?

Torsten, what do you think about adding MPI_WEIGHTS_NULL/EMPTY? If you're OK with it too then I'll write up the appropriate text instead of what we have in the ticket right now. That sounds good, however, I'd see it as an extension to what we have right now. MPI_UNWEIGHTED would still remain valid.

Also, we should consider taking this modification out of the ticket process. It seems like exactly the sort of thing that can and should be handled by the chapter committee. Yes but I'm not sure if we should have a telecon for this small change. We should be able to involve the chapter committee via email once we have a fix. We will need to reset this ticket anyway.

Torsten

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-01-22 12:30:36 -0600

Replying to htor:

Replying to goodell:

I think Adam has a point here (not sure how I missed this correspondence until now). Rolf's comment is incorrect, since we do attempt to read the storage even if n==0. It might not be a big deal if we only had C bindings (since NULL is very natural for this), but the Fortran bindings cause a problem here. Why would we attempt to read the storage if n==0? I would understand that we check for weighted/unweighted? But I see the issue and agree (see below).

For n > 0 the weights argument is the address-value MPI_UNWEIGHTED or an array with weights in Fortran or C. In Fortran and C, MPI_UNWEIGHTED must be implemented in a way that it address value is different from any array that can be supplied.

For n=0, the weights argument can be the address-value MPI_UNWEIGHTED or any other address value, which may or may not point to an array. The array-vlues are not read. The address-value itself is read.

The two questions are only,

  1. which address-values do we allow to implement MPI_UNWEIGHTED
  2. which address-values do we allow that the user uses in the case of a weighted graph with n=0 at this process.

For the 1st question, the old MPI-2.2 recommended MPI_UNWEIGHTED=NULL, this ticket proposes to forbid the use of NULL for MPI_UNWEIGHTED.

For the 2nd question, the user can use any dummy/scratch array in C and Fortran. In C, he/she can also use the address of a dummy/scratch variable and he/she might want to use NULL. He/she must not use any other address-value that might be identical to MPI_UNWEIGHTED.

Implication A: With MPI_2.2, the user was not allowed to use NULL in question 2.

This means that this proposal additionally allows now the use of NULL for question 2. This is helpful, because this is natural in C.

The best fix would have been a separate boolean parameter for weighted/unweighted, but that's not backwards compatible with MPI-2.2. So something like MPI_WEIGHTS_NULL probably makes more sense. Maybe call it MPI_WEIGHTS_EMPTY to match up better with our other EMPTY/NULL conventions?

Torsten, what do you think about adding MPI_WEIGHTS_NULL/EMPTY? If you're OK with it too then I'll write up the appropriate text instead of what we have in the ticket right now. That sounds good, however, I'd see it as an extension to what we have right now. MPI_UNWEIGHTED would still remain valid.

Also, we should consider taking this modification out of the ticket process. It seems like exactly the sort of thing that can and should be handled by the chapter committee. Yes but I'm not sure if we should have a telecon for this small change. We should be able to involve the chapter committee via email once we have a fix. We will need to reset this ticket anyway.

Torsten

We should answer question 2 for the user to guarantee that there is never the same value as MPI_UNWEIGHTED.

Yes, MPI_WEIGHTS_EMPTY would be a meaningful input in a weighted graph in a process with n=0. But for me, it seems to be a too heavy soultion. For n=0, it is enough to add the following sentence after p255:39 and before the advice defined in this ticket:

In a weighted graph, a process with n==0 may specify the weights argument with a scratch array (in C or Fortran), or NULL in C.

The total solution can be done also very different:

I would recommend to put both solutions into this ticket and to make a vote, which one should be used in MPI-3.0. The ticket should have the full form, because

The change-log should reflect this too.

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-01-23 09:27:16 -0600

Based on the discussion above, I would like to withdraw my original proposal (Had 1st Reading) by a new proposal that is a compromise between simple and solving all problems and being source code and ABI backward compatible.

To speed up the process, added it already to the ticket body, but I kept the old proposal for the case that somebody still wants to keep it for voting.

If all agree, then I would like to withdraw the old proposal and run only with the new one. And returning the priority to "Scheduled".

mpiforumbot commented 8 years ago

Originally by goodell on 2012-01-23 13:05:10 -0600

minor text improvements

mpiforumbot commented 8 years ago

Originally by goodell on 2012-01-23 13:05:48 -0600

I'm now happy enough with Rolf's counter proposal and am happy to support it in place of my original proposal.

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-01-24 02:54:04 -0600

Your wording of the advices is better. Thank you.

I detected and corrected another small bug: rather it is a special value for the total array argument. changed into rather they are special values for the total array argument.

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-01-24 03:05:25 -0600

Torsten and Adam,

please can you (as topology chapter author and committee member) take 15 minutes and review the new text as shown in "Proposed Solution (new alternative based on discussion below)".

If it is okay with you both, then we can withdraw/remove the first proposal and reset the priority to "scheduled" for "March 2012, Chicago".

Thanks and best regards[[BR]] Rolf

mpiforumbot commented 8 years ago

Originally by htor on 2012-02-01 21:52:13 -0600

I am mostly ok with the recommended change. Some comments:

1) We should really not refer back to MPI-2.2, i.e., remove the statement (twice) "(as was erroneously recommended in MPI-2.2)"

2) The ticket needs to be cleaned up (i.e., replace the old proposed change with the new one) to ensure everybody knows what we are voting on.

3) The clock will likely be reset :-)

Thanks, Torsten

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-02-05 02:00:35 -0600

Replying to htor:

I am mostly ok with the recommended change. Some comments:

1) We should really not refer back to MPI-2.2, i.e., remove the statement (twice) "(as was erroneously recommended in MPI-2.2)"

I'll remove this sentence from the advice to users because it is a rationale. I'll add the following rationale:

Rationale. For backward compatibility reasons, it is not mandatory that MPIUNWEIGHTED not be implemented as NULL, see Annex B.1^)^ on page 593. -(End of rationale.)_

An alternative text without the ... not ... not ... may be:

Rationale. For backward compatibility reasons, it is still valid that MPIUNWEIGHTED may be implemented as NULL, see Annex B.1^)^ on page 593. -(End of rationale.)_

^_)^ Remark: B.1 is the new change-log from 2.2 to 3.0, which contains the sentence:[[BR]] *The recommended C implementation value for MPIUNWEIGHTED was changed from NULL to non-NULL.

2) The ticket needs to be cleaned up (i.e., replace the old proposed change with the new one) to ensure everybody knows what we are voting on.

Yes, but I wannted to wait until more than Dave and me replied positive. The ticket has only a reporter, not an owner. Therefore I feel free to do it now, after Dave and you returned support for the new text. I'll keep the old text (because it is already formally read [people may remember that they already agreed with its content] and because it's Dave's text), but telling that it is withdrawn.

3) The clock will likely be reset :-)

Yes, of course.

Thanks, Torsten

Thank you for your review.

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-02-05 02:26:22 -0600

I implemented all the changes I proposed in the previous comment.

Please tell in your review, whether you prefer the

I prefer the first version.

mpiforumbot commented 8 years ago

Originally by htor on 2012-02-05 11:36:17 -0600

Replying to RolfRabenseifner:

Replying to htor:

I am mostly ok with the recommended change. Some comments:

1) We should really not refer back to MPI-2.2, i.e., remove the statement (twice) "(as was erroneously recommended in MPI-2.2)"

I'll remove this sentence from the advice to users because it is a rationale. I'll add the following rationale:

*Rationale. For backward compatibility reasons, it is not mandatory that MPIUNWEIGHTED not be implemented as NULL, see Annex B.1^)^ on page 593. (End of rationale.)_**

An alternative text without the ... not ... not ... may be:

*Rationale. For backward compatibility reasons, it is still valid that MPIUNWEIGHTED may be implemented as NULL, see Annex B.1^)^ on page 593. (End of rationale.)_**

^_)^ Remark: B.1 is the new change-log from 2.2 to 3.0, which contains the sentence:[[BR]] *The recommended C implementation value for MPIUNWEIGHTED was changed from NULL to non-NULL. I don't see the reason for the text being there. The statement in MPI-2.2 was non-binding and thus, strictly seen, not part of the API anyway. Adding this rationale is just cluttering the standard. I'm against both versions.

2) The ticket needs to be cleaned up (i.e., replace the old proposed change with the new one) to ensure everybody knows what we are voting on.

Yes, but I wannted to wait until more than Dave and me replied positive. The ticket has only a reporter, not an owner. Therefore I feel free to do it now, after Dave and you returned support for the new text. I'll keep the old text (because it is already formally read [people may remember that they already agreed with its content] and because it's Dave's text), but telling that it is withdrawn. I see.

Best, Torsten

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-02-06 03:28:07 -0600

Replying to htor:

Replying to RolfRabenseifner:

Replying to htor:

...

1) We should really not refer back to MPI-2.2, i.e., remove the statement (twice) "(as was erroneously recommended in MPI-2.2)"

I'll remove this sentence from the advice to users because it is a rationale. I'll add the following rationale:

*Rationale. For backward compatibility reasons, it is not mandatory that MPIUNWEIGHTED not be implemented as NULL, see Annex B.1^)^ on page 593. (End of rationale.)_**

An alternative text without the ... not ... not ... may be:

*Rationale. For backward compatibility reasons, it is still valid that MPIUNWEIGHTED may be implemented as NULL, see Annex B.1^)^ on page 593. (End of rationale.)_**

^_)^ Remark: B.1 is the new change-log from 2.2 to 3.0, which contains the sentence:[[BR]] *The recommended C implementation value for MPIUNWEIGHTED was changed from NULL to non-NULL. I don't see the reason for the text being there. The statement in MPI-2.2 was non-binding and thus, strictly seen, not part of the API anyway. Adding this rationale is just cluttering the standard. I'm against both versions.

The idea behind such a rationale is that the standard is the Forum's memory. We have seen this in the past with one other bug in the standard, the wrong C binding of MPI_Cancel. The discussion came up again and again after some years. To stop this we added the rationale MPI-2.2 p68 lines 29-32.

To prevent further discussions in the future on this topic or that reviewers in such a discussion do not oversee backward compatible issues coming from MPI-2.2, such a rationale may be helpful.

...

mpiforumbot commented 8 years ago

Originally by goodell on 2012-02-06 15:01:44 -0600

Replying to RolfRabenseifner:

I implemented all the changes I proposed in the previous comment.

Please tell in your review, whether you prefer the

  • first ("it is not mandatory that MPI_UNWEIGHTED not be implemented as NULL")
  • or second ("it is still valid that MPI_UNWEIGHTED may be implemented as NULL") version of the Rationale.

I prefer the first version.

The first version sounds quite bad to my ear, please don't use that one. The second one can be slightly improved:

-Rationale.* For backward compatibility reasons, it is still valid to implement MPI_UNWEIGHTED as NULL, see Annex B.1 on page 593. *(End of rationale.)*
mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-02-16 10:14:04 -0600

I changed the text and removed the first version of the rationale and changed the second one into Dave Goodell's text.

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-02-18 13:47:50 -0600

I changed the Fortran type for MPI_UNWEIGHTED and MPI_WEIGHTS_EMPTY from "INTEGER, DIMENSION(*)" to "INTEGER array".

Reasons: to be consistent with the definition of MPI_ERRCODES_IGNORE and ticket #172.

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-02-21 13:19:56 -0600

I removed the ugly alternative rationale version with "not ... not ...".

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-03-06 13:03:00 -0600

Changes from formal reading in March 2012 Chicago meeting.

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-03-06 15:03:59 -0600

Had formal reading in the Chicago meeting, March 2012.

mpiforumbot commented 8 years ago

Originally by goodell on 2012-07-17 09:51:14 -0500

Wording improvement suggested by Jim Dinan. Change:

Rationale. For backward compatibility reasons, it is still valid that MPI_UNWEIGHTED may be implemented as NULL, see Annex B.1*) on page 593. (End of rationale.)

to:

Rationale.  To ensure backward compatibility, MPI_UNWEIGHTED may still be implemented as NULL.  See Annex B.1*) on page 593. (End of rationale.)

Don't forget to change both rationales.

mpiforumbot commented 8 years ago

Originally by htor on 2012-07-17 10:05:14 -0500

Committed in approved/topol. Please review! Keep in mind that this ticket touches two more chapters.

http://www.unixer.de/sec/mpi-report.pdf

Torsten

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-07-17 15:24:41 -0500

Committed in approved/chap-term (r1345, by Bill) and approved/chap-appLang (r1362).

mpiforumbot commented 8 years ago

Originally by RolfRabenseifner on 2012-07-17 15:35:03 -0500

Committed in approved/chap-binding (r1364).

With this, all shozuld be done and it can be move to "Waiting for PDF Reviews"

mpiforumbot commented 8 years ago

Originally by jsquyres on 2012-07-17 17:49:24 -0500

Reviewd T&C and Bindings chapters ok.

mpiforumbot commented 8 years ago

Originally by goodell on 2012-07-18 14:32:48 -0500

Topo chapter review:

OK in r1417.