Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate short-typed wait/test & update tables to std AMO #3

Conversation

davidozog
Copy link

Addresses issue openshmem-org#334

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@naveen-rn
Copy link
Owner

Changes looks fine to me. @nspark

@naveen-rn
Copy link
Owner

@nspark and @BryantLam - can i get approvals for merging this change?

@naveen-rn
Copy link
Owner

@davidozog can you change the merge branch from master to naveen-rn:naveen-rn/openshmem-1.5 in this PR?

@davidozog davidozog changed the base branch from master to naveen-rn/openshmem-1.5 February 18, 2020 18:00
@davidozog
Copy link
Author

@naveen-rn Done - sorry about that!

Copy link
Collaborator

@nspark nspark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the \refs to the standard AMO types look good, but the accompanying text needs to be revised as well.

content/shmem_test.tex Outdated Show resolved Hide resolved
content/shmem_test.tex Outdated Show resolved Hide resolved
content/shmem_test_all.tex Outdated Show resolved Hide resolved
content/shmem_test_all.tex Outdated Show resolved Hide resolved
content/shmem_test_all_vector.tex Outdated Show resolved Hide resolved
content/shmem_wait_until_some.tex Outdated Show resolved Hide resolved
content/shmem_wait_until_some.tex Outdated Show resolved Hide resolved
content/shmem_wait_until_some_vector.tex Outdated Show resolved Hide resolved
content/shmem_wait_until_some_vector.tex Outdated Show resolved Hide resolved
content/p2p_sync_intro.tex Outdated Show resolved Hide resolved
Copy link
Collaborator

@nspark nspark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the \refs to the standard AMO types look good, but the accompanying text needs to be revised as well.

It looks like GitHub will let you merge all these changes as a single commit (instead of one at a time) under the "Files" view by selecting "Add suggestion to batch".

@davidozog
Copy link
Author

Thanks @nspark.

Ha, I did several individual commits before seeing your suggestion with the batch commit. That seems to work well.

content/p2p_sync_intro.tex Outdated Show resolved Hide resolved
Copy link

@BryantLam BryantLam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall changes look good. I'm almost certain the backmatter edits will cause a merge conflict though, but there's not much we can do about it at this point.

content/backmatter.tex Outdated Show resolved Hide resolved
@@ -44,6 +45,7 @@
\TableCaptionRef{Point-to-Point Synchronization Types and Names}
\label{p2psynctypes}
\end{center}
\end{DeprecateBlock}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a sentence either in the backmatter deprecation rationale or somewhere "nearby" this table that talks about why this entire table is marked deprecated. Otherwise, why have this table at all if there aren't any outstanding references to it now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still references to it:

\begin{DeprecateBlock}
\begin{CsynopsisCol}
void @\FuncDecl{shmem\_wait\_until}@(long *ivar, int cmp, long cmp_value);
void @\FuncDecl{shmem\_wait}@(long *ivar, long cmp_value);
void @\FuncDecl{shmem\_\FuncParam{TYPENAME}\_wait}@(TYPE *ivar, TYPE cmp_value);
\end{CsynopsisCol}
where \TYPE{} is one of \{\CTYPE{short}, \CTYPE{int}, \CTYPE{long},
\CTYPE{long long}\} and has a corresponding \TYPENAME{} specified by
Table~\ref{p2psynctypes}.
\end{DeprecateBlock}

I guess one could ask, do we need to deprecate the table itself if all the interfaces the reference it are deprecated?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@npark, @BryantLam Yes - I would think deprecating the table is appropriate to avoid user's being confused about which table to look at when using the non-deprecated p2p sync routines.

Would it help to add a row to the deprecation table regarding the p2p type-table deprecation? Perhaps similar to:

    \minitab{Table~\ref{p2psynctypes}: point-to-point synchronization types}
        & 1.5 & Current & Table~\ref{stdamotypes}: standard AMO types \\ \hline

I also drafted a rationale section for that:

\subsection{Table~\ref{p2psynctypes}: point-to-point synchronization types}
The point-to-point synchronization routines are only compatible with \acp{AMO}
(as of \openshmem 1.5), so all point-to-point synchronization types must be
compatible with the \ac{AMO} types.  Accordingly, all non-deprecated
point-to-point synchronization interfaces are defined by referring to the standard \ac{AMO} type
table.

Do you want to add it, or is it ok without?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are still outstanding references to it, we should leave in the p2psynctypes table.

I'm okay with adding this entry into the deprecation table + rationale. I like that it makes it clear why the table is deprecated. The last sentence of the rationale reads a bit weird, so maybe it could be rephrased. Otherwise it looks good!

Copy link
Owner

@naveen-rn naveen-rn Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like @BryantLam kind of approved this PR :) I'm fine with the deparate table + rationale approach.

Repository owner deleted a comment from bryant Feb 28, 2020
@naveen-rn
Copy link
Owner

Can we merge this PR? @BryantLam

Copy link

@BryantLam BryantLam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@naveen-rn -- Sorry for the delay! Yes, I'm good with these changes.

.. I think I missed out on an email notification because I wasn't watching this repo. Still trying to figure out how to get consistent email notifications so this doesn't happen again. Never mind. I figured it out. My email filters were bad.

@naveen-rn naveen-rn merged commit 493fa7f into naveen-rn:naveen-rn/openshmem-1.5 Mar 11, 2020
BryantLam added a commit to BryantLam/openshmem-specification that referenced this pull request Apr 2, 2020
Addendum to naveen-rn#3

This PR specifies the deprecated `short` and `unsigned short` types from
the p2p sync types table to the C11 and C/C++ function synopses of
`shmem_wait_until` and `shmem_test`.

Otherwise, the specification for these these functions would only be
found in the deprecation table.
BryantLam added a commit to BryantLam/openshmem-specification that referenced this pull request Apr 2, 2020
Move these changes to #18

--

Revert "Add deprecation rationale for p2psynch types table"

This reverts commit 238d700.

Revert "Reorder short/ushort p2psync deprecation rationale"

This reverts commit 22f89a4.

Revert backmatter.tex changes from 4cc60ba
as represented in commit cedbe5a.
BryantLam added a commit to BryantLam/openshmem-specification that referenced this pull request Apr 2, 2020
naveen-rn added a commit that referenced this pull request Apr 2, 2020
naveen-rn pushed a commit that referenced this pull request May 8, 2020
section/rma: replace default team with world team
naveen-rn pushed a commit that referenced this pull request May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants