search

openshmem-org / specification

OpenSHMEM Application Programming Interface
http://www.openshmem.org
51 stars 41 forks source link

Add Vendor Version Info #463

Open jdinan opened 3 years ago

jdinan commented 3 years ago

Problem

OpenSHMEM specifies only the string SHMEM_VENDOR_STRING for implementations to convey version information. This does not give programmers an easy way to determine what version of an OpenSHMEM library they are using.

OpenSHMEM also does not provide a single integer to identify the spec version supported, which complicates version checks in user code.

Proposed Solution

Add compile-time integer constants:

SHMEM_MAJOR_VERSION // Already have this
SHMEM_MINOR_VERSION // Already have this
SHMEM_VERSION       // Specify as 100*MAJOR + MINOR
SHMEM_VERSIONIFY

SHMEM_VENDOR_+NAME+   // e.g. SHMEM_VENDOR_NVSHMEM (Vendors must declare one or more)
// skip // SHMEM_VENDOR_MAJOR_VERSION
// skip // SHMEM_VENDOR_MINOR_VERSION
// skip // SHMEM_VENDOR_PATCH_VERSION
SHMEM_VENDOR_VERSION // Vendor defined, e.g. 10000*MAJOR + 100*MINOR + PATCH, monotonic increasing
SHMEM_VENDOR_VERSIONIFY // Macro to generate compile time constant for vendor version, component parts implementation defined

Add runtime query functions:

void shmem_info_get_version(int *major, int *minor); // Already have this
long shmem_info_get_version_number(void);

// skip // void shmem_info_get_vendor_version(int *major, int *minor, int *patch);
long shmem_info_get_vendor_version_number(void);
BryantLam commented 3 years ago

I prefer an approach that wouldn't require me to remember what the scheme is. Would we also want to standardize a version-builder macro?

#define SHMEM_VERSIONIFY(x, y) <standard-defined or vendor-defined scheme>

#if SHMEM_VERSION >= SHMEM_VERSIONIFY(1, 5)
# ...
#endif

(or some other typical name for this macro).

Is there merit to forcing a standard-defined scheme vs. leaving it to the vendors?

jdinan commented 3 years ago

I agree that we should specify how SHMEM_VERSION is built and we can add a SHMEM_VERSIONIFY macro if it's helpful to users.

For SHMEM_VENDOR_VERSION, I would assume, in the spirit of these constants, that would want to leave the definition up to implementors with the stipulation that the constant for release D.E.F should compare as greater than release A.B.C if (D > A) or (D == A && E > B) or (D == A && E == B && F > C).

nspark commented 3 years ago

In my current code, I do this manually for the OpenSHMEM implementations I primarily target and have version-specific conditions. However, the macros I use are implementation specific, while this one is implementation agnostic. I think this is an important difference. My primary use for this is to avoid certain features that may be buggy in a particular version.

For example: If I'm using Foo SHMEM and version 1.2.0 is known to have a introduced a bug in shmem_align (e.g., swapped the alignment and size arguments) that is later fixed in version 1.2.3, then I'll do something like:

#if FOO_SHMEM_VERSION >= VERSIONIFY(1, 2, 0) && FOO_SHMEM_VERSION < VERSIONIFY(1, 2, 3)
  ptr = shmem_align(size, alignment);
#else
  ptr = shmem_align(alignment, size);
#endif

However, this won't work portably if the vendor version macro is SHMEM_VENDOR_VERSION, since this could match with any implementation that hits this versioning range. The vendor-specific version macro has to be named unique for each implementation. (We also can't use SHMEM_VENDOR_STRING to distinguish implementations since we can't do compile-time string comparisons.)

IMO, this could be a helpful feature for reporting the implementation's internal version number, but not for providing the types of preprocessor guards applications occasionally need to remain portable across a range of implementations. (If this is for informational purposes only, I think it should probably be a string, since some versions may include git commit hashes for prerelease builds.)

nspark commented 3 years ago

OpenSHMEM also does not provide a single integer to identify the spec version supported, which complicates version checks in user code.

I use code very similar to @BryantLam's example for SHMEM_VERSION and SHMEM_VERSIONIFY as preprocessor guards across different levels of OpenSHMEM Specification support.

jdinan commented 3 years ago

We could fix this by adding a SHMEM_VENDOR_NAME constant. Where NAME is the name could be SHMEM_VENDOR_NVSHMEM, SHMEM_VENDOR_SOS, etc. WDYT?

BryantLam commented 3 years ago

@jdinan -- Could you provide an example? That sounds like it would run into the same problem of SHMEM_VENDOR_VERSION , but with an extra step.

In practice, we could specify something like:

#define  SHMEM_VENDOR_MAJOR_VERSION  1
#define  SHMEM_VENDOR_MINOR_VERSION  2
#define  SHMEM_VENDOR_PATCH_VERSION  3
#define  <vendor-defined VENDOR_VERSION or simply VERSION>  <vendor-defined version scheme>
#ifdef FOOSHMEM_VERSION // or FOOSHMEM_VENDOR_VERSION for consistency
#if FOOSHMEM_VERSION >= SHMEM_VENDOR_VERSIONIFY(1, 2, 0) && FOOSHMEM_VERSION < SHMEM_VENDOR_VERSIONIFY(1, 2, 3)
  // Remedy the bug.
  ptr = shmem_align(size, alignment);
#else
  // Do the correct thing.
  ptr = shmem_align(alignment, size);
#endif
#endif

IMO, this quickly devolves:

BryantLam commented 3 years ago

We could choose to not standardize a VENDOR_VERSION. It's not that hard for a user to create one themselves and throw it in a global header.

#if defined(FOOSHMEM_VERSION_MAJOR) && defined(FOOSHMEM_VERSION_MINOR) && defined(FOOSHMEM_VERSION_PATCH)
  #define  MY_FOOSHMEM_VERSION  <my custom version scheme for FOOSHMEM>
  #define  MY_FOOSHMEM_VERSIONIFY(x, y, z)  <my custom versionify for FOOSHMEM>
#else
  #error "Fix your implementation"
#endif

(this is also true for SHMEM_VERSIONIFY, so this might not be a great justification)

nspark commented 3 years ago

Am I correct to think you mean something like the following?

// vendor FOO's shmem.h
#define SHMEM_VENDOR_NAME "FOO"
#define SHMEM_VENDOR_VERSION_FOO VERSIONIFY(1, 2, 3)

An application user would read this shmem.h and then use SHMEM_VENDOR_VERSION_FOO; e.g.,

// user's application code
#if SHMEM_VENDOR_VERSION_FOO >= 1.2.0 && SHMEM_VENDOR_VERSION_FOO < 1.2.4
  // vendor FOO workaround...
#endif

#if SHMEM_VENDOR_VERSION_BAR >= 3.1.0 && SHMEM_VENDOR_VERSION_BAR < 3.2.0
  // vendor BAR workaround...
#endif
nspark commented 3 years ago

Separately, is there any sense in requiring SHMEM_VENDOR_NAME to be a valid C token (and not a string), so one can do something like the following...?

#define SHMEM_VENDOR_NAME FOO
#define JOIN(A, B) JOIN1(A, B)
#define JOIN1(A, B) A##_##B
#define VENDORIFY(BASE) JOIN(BASE, SHMEM_VENDOR_NAME)
#define SHMEM_VENDOR_VERSION VENDORIFY(SHMEM_VENDOR_VERSION)

Or, am I just having too much fun with the C preprocessor?

BryantLam commented 3 years ago 
#define SHMEM_VENDOR_VERSION VENDORIFY(SHMEM_VENDOR_VERSION)

I thought about that, but in practice, I don't think users would want to use this definition. It makes tracing your code a lot harder to figure out when you see a guard like:

#if SHMEM_VENDOR_VERSION >= 1.2
  < some C code here without a comment >
#endif

Whose implementation was remedied?

nspark commented 3 years ago

I thought about that, but in practice, I don't think users would want to use this definition. It makes tracing your code a lot harder to figure out when you see a guard like: [...] Whose implementation was remedied?

Yes, which circles back to my earlier comment that explicitly-named macros are imporant.

I guess I just got a little too sucked into the preprocessor.

jdinan commented 3 years ago

I was thinking of something like this:

#if defined(SHMEM_VENDOR_NVSHMEM) && \
    SHMEM_VENDOR_VERSION >= SHMEM_VENDOR_VERSIONIFY(1, 2, 0) && \
    SHMEM_VENDOR_VERSION  < SHMEM_VENDOR_VERSIONIFY(1, 2, 3)
  // Remedy the bug.
  ptr = shmem_align(size, alignment);
#else
  // Do the correct thing.
  ptr = shmem_align(alignment, size);
#endif

This has the advantage that SHMEM_VENDOR_NAME being defined also implies that the rest of the preprocessor tokens are also be defined.

jdinan commented 3 years ago

@jdinan Verify that preprocessor would stop evaluating the above expression at defined(SHMEM_VENDOR_NVSHMEM) when compiling with a different SHMEM library.

nspark commented 3 years ago

SHMEM_VENDOR_VERSIONIFY seems to require the same number of arguments: https://godbolt.org/z/9G6j6z9Ta

tonycurtis commented 3 years ago

From what I found at cppreference, it’s not clear if the expression evaluation can short-circuit. I changed the godbolt code to something that should work for everyone (famous last words).

On Apr 30, 2021, at 2:00 PM, Nick Park @.***> wrote:

SHMEM_VENDOR_VERSIONIFY seems to require the same number of arguments: https://godbolt.org/z/9G6j6z9Ta https://godbolt.org/z/9G6j6z9Ta — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/openshmem-org/specification/issues/463#issuecomment-830265647, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABB6C2HZC6WMVMHTF3S4MEDTLLV5VANCNFSM4Z3OKMDQ.

nspark commented 3 years ago

@tonycurtis If you change something in Godbolt, you need to (re)share to get a new URL to display your changes.

tonycurtis commented 3 years ago

Oh, ok. Never used it (or at least, not for ages).

On Apr 30, 2021, at 2:11 PM, Nick Park @.***> wrote:

@tonycurtis https://github.com/tonycurtis If you change something in Godbolt, you need to (re)share to get a new URL to display your changes.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openshmem-org/specification/issues/463#issuecomment-830271934, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABB6C2BUT2CSUEZ6JMPAX4LTLLXGVANCNFSM4Z3OKMDQ.

tonycurtis commented 3 years ago

https://godbolt.org/z/G37Eqqb5j https://godbolt.org/z/G37Eqqb5j

On Apr 30, 2021, at 2:11 PM, Nick Park @.***> wrote:

@tonycurtis https://github.com/tonycurtis If you change something in Godbolt, you need to (re)share to get a new URL to display your changes.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/openshmem-org/specification/issues/463#issuecomment-830271934, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABB6C2BUT2CSUEZ6JMPAX4LTLLXGVANCNFSM4Z3OKMDQ.

nspark commented 3 years ago

From C11 6.10.1-4 (emphasis mine):

Prior to evaluation, macro invocations in the list of preprocessing tokens that will become the controlling constant expression are replaced (except for those macro names modified by the defined unary operator), just as in normal text. [...] After all replacements due to macro expansion and the defined unary operator have been performed, all remaining identifiers (including those lexically identical to keywords) are replaced with the pp-number 0, and then each preprocessing token is converted into a token. The resulting tokens compose the controlling constant expression which is evaluated according to the rules of 6.6.

jdinan commented 3 years ago

Thanks for checking on this! So, we would need to write the above example like this:


#if defined(SHMEM_VENDOR_NVSHMEM)
#if SHMEM_VENDOR_VERSION >= SHMEM_VENDOR_VERSIONIFY(1, 2, 0) && \
    SHMEM_VENDOR_VERSION  < SHMEM_VENDOR_VERSIONIFY(1, 2, 3)
  // Remedy the bug.
  ptr = shmem_align(size, alignment);
#else
  // Do the correct thing.
  ptr = shmem_align(alignment, size);
#endif
nspark commented 3 years ago

@jdinan Except you're missing a closing #endif—which, I think, is a perfect example of how closely nested preprocessor guards are a real pain. Stylistically, most code formatters don't indent nested preprocessor directives, which makes such errors harder to catch.

This makes me somewhat hesitant to allow for an implementation-defined function-like macro "prototype." I'd much rather have the "one-line" version (as shown here).

jdinan commented 3 years ago

@nspark What about making SHMEM_VENDOR_VERSIONIFY variadic? That should still support the one-line version.

nspark commented 3 years ago

@jdinan Is this an example of what you mean?

nspark commented 3 years ago

Poking @jdinan...