Add Vendor Version Info

[jdinan]

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]

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]

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]

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]

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]

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]

@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:

• What does SHMEM_VENDOR_VERSIONIFY do that a user couldn't do themselves with SHMEM_VENDOR_{MAJOR, MINOR, PATCH}_VERSION? I.e., if we standardize this, would a user ever use the standardized MAJOR, MINOR, PATCH identifiers?
• Is that sufficient justification to include MAJOR, MINOR, and PATCH when the vendors could just do it themselves?

  #define  FOOSHMEM_VERSION_MAJOR
  #define  FOOSHMEM_VERSION_MINOR
  #define  FOOSHMEM_VERSION_PATCH
  #define  SHMEM_VENDOR_VERSIONIFY(x, y, z)  <use FOOSHMEM_VERSION_*>

[BryantLam]

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]

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]

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]

#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]

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]

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]

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

[nspark]

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

[tonycurtis]

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 <#463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB6C2HZC6WMVMHTF3S4MEDTLLV5VANCNFSM4Z3OKMDQ>.

[nspark]

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

[tonycurtis]

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 <#463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB6C2BUT2CSUEZ6JMPAX4LTLLXGVANCNFSM4Z3OKMDQ>.

[tonycurtis]

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 <#463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABB6C2BUT2CSUEZ6JMPAX4LTLLXGVANCNFSM4Z3OKMDQ>.

[nspark]

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]

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]

@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]

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

[nspark]

@jdinan Is this an example of what you mean?

[nspark]

Poking @jdinan...