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

config: replace ax_prefix_config_h.m4 #3856

Merged
merged 3 commits into from Jun 25, 2019

Conversation

Projects
None yet
2 participants
@hzhou
Copy link
Contributor

commented Jun 17, 2019

Replace AX_PREFIX_CONFIG_H with AC_CONFIG_COMMANDS and
perl confdb/cmd_prefix_config_h.pl.

Expected Performance Changes

Known Issues

Author Checklist

  • Reference appropriate issues (with "Fixes" or "See" as appropriate)
  • Confirm whitespace/style checkers are happy (or has a good reason for being bad)
  • Commits are self-contained and do not do two things at once
  • Remove xfail from the test suite when fixing a test
  • Commit message is of the form: module: short description and follows good practice
  • Add comments such that someone without knowledge of the code could understand
  • Add Devel Docs in the doc/ directory for any new code design
  • Passes tests (included warning check)

@hzhou hzhou force-pushed the hzhou:1906_ax_prefix branch 3 times, most recently from 0932cb5 to aa465ab Jun 17, 2019

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

test:mpich/custom
env: skip_test=true

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

test:jenkins/ch3/tcp

@hzhou hzhou requested a review from raffenet Jun 18, 2019

@raffenet

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

The defines here (https://github.com/pmodels/mpich/blob/master/src/mpl/configure.ac#L816) get double prefixed after this patch, and may not work as intended. I wonder if this even worked before this patch...

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

The defines here (https://github.com/pmodels/mpich/blob/master/src/mpl/configure.ac#L816) get double prefixed after this patch, and may not work as intended. I wonder if this even worked before this patch...

I'll fix the script to detect and avoid double prefix.

config: replace ax_prefix_config_h.m4
Replace AX_PREFIX_CONFIG_H with AC_CONFIG_COMMANDS and
perl confdb/cmd_prefix_config_h.pl.

@hzhou hzhou force-pushed the hzhou:1906_ax_prefix branch from aa465ab to f318ce2 Jun 18, 2019

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

Added another commit to

  • avoid double prefix
  • add prefix to the #undef inside comments
  • direct define, matches AC_CONFIG_HEADER, deviates from AX_PREFIX_CONFIG_H

retest:
test:jenkins/ch4/most
test:jenkins/ch3/most

@raffenet

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Looking better. I'm still seeing a few issues. I don't think this is actually a problem, but looks a little weird.

/* Define to 1 if on MINIX. /
/
#undef _mpl__MINIX */

/* Define to 2 if the system does not provide POSIX.1 features except with
this defined. /
/
#undef _mpl__POSIX_1_SOURCE */

/* Define to 1 if you need to in order for `stat' and other things to work. /
/
#undef _mpl__POSIX_SOURCE */

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

Looking better. I'm still seeing a few issues. I don't think this is actually a problem, but looks a little weird.

/* Define to 1 if on MINIX. /
/
#undef _mpl__MINIX /
/
Define to 2 if the system does not provide POSIX.1 features except with
this defined. /
/
#undef _mpl__POSIX_1_SOURCE /
/
Define to 1 if you need to in order for `stat' and other things to work. /
/
#undef _mpl__POSIX_SOURCE */

That was AX_PREFIX_CONFIG_H's behavior, it prefixes _mpl_ when the symbol starts with non-capital. In those case, it starts with _. We don't have to do that. In fact, I much prefer just always prefix MPL_.

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

Actually, I am not sure what AX_PREFIX_CONFIG_H for the likes of _MINIX, maybe that is treated as all capitals. Anyway, how about always use MPL_ prefix regardless of the symbol case?

@raffenet

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2019

Here is what it did previously.

/* Define to 1 if on MINIX. /
/
#undef _MINIX */

/* Define to 2 if the system does not provide POSIX.1 features except with
this defined. /
/
#undef _POSIX_1_SOURCE */

/* Define to 1 if you need to in order for `stat' and other things to work. /
/
#undef _POSIX_SOURCE */

I don't have much opinion about always using upper-case. We just need to make sure to update all the places in the code that used the old style.

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

Here is what it did previously.

Previously, AX_PREFIX_CONFIG_H doesn't touch symbols in the comment (#undef). I'll update the script to always use uppercase prefix.

@hzhou hzhou force-pushed the hzhou:1906_ax_prefix branch 2 times, most recently from 3edc174 to d860eeb Jun 18, 2019

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

On second thought, it is better to conform to the convention AX_PREFIX_CONFIG_H. openpa in particular uses _opa_inline all over, it doesn't make sense to bring too much diffs.

I patched the script so it applies MPL_ when the whole symbol is upper case (including _), otherwise, it applies _mpl_.

test:jenkins/ch3/most
test:jenkins/ch4/most

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

Actually, for inline, const, and restrict, it is really best to leave them alone (without adding prefix) so the code can use the keyword directly. We can add #ifndef inline guard so it is still safe to #include opa_config.h on top of mpichconf.h or mplconf.h.

I'll add another commit to test it out.

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

test:jenkins/ch3/most
test:jenkins/ch4/most

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

test:mpich/quick
skip_test: true

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2019

ROMIO still have the MPI_OFFSET warnings, disable it and retest:

test:mpich/quick --with-device=ch4:ofi --disable-romio
testlist: coll/allred 4

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

@raffenet This PR is ready for your another review.

@raffenet

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

Perhaps we should remove usage of autoconf macros for const, restrict, and inline as well and just assume they are available? https://github.com/pmodels/mpich/blob/master/src/mpl/configure.ac#L52

I still see some weird stuff in mplconfig.h, but only for commented lines, so maybe it doesn't matter.

/* Define to 1 if on MINIX. /
/
#undef MPL__MINIX */

/* Define to 2 if the system does not provide POSIX.1 features except with
this defined. /
/
#undef _mpl__POSIX_1_SOURCE */

/* Define to 1 if you need to in order for `stat' and other things to work. /
/
#undef MPL__POSIX_SOURCE */

and

/* Define which x86 cycle counter to use /
/
#undef _mpl_LINUX86_CYCLE_CPUID_RDTSC32 */

/* Define which x86 cycle counter to use /
/
#undef _mpl_LINUX86_CYCLE_CPUID_RDTSC64 */

/* Define which x86 cycle counter to use /
/
#undef _mpl_LINUX86_CYCLE_RDTSC */

/* Define which x86 cycle counter to use /
/
#undef _mpl_LINUX86_CYCLE_RDTSCP */

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

Perhaps we should remove usage of autoconf macros for const, restrict, and inline as well and just assume they are available? https://github.com/pmodels/mpich/blob/master/src/mpl/configure.ac#L52

I saw that restrict gets defined as __restrict, so maybe gcc in default mode doesn't support restrict?

I still see some weird stuff in mplconfig.h, but only for commented lines, so maybe it doesn't matter.

/* Define to 1 if on MINIX. /
/
#undef MPL__MINIX /
/
Define to 2 if the system does not provide POSIX.1 features except with
this defined. /
/
#undef _mpl__POSIX_1_SOURCE /
/
Define to 1 if you need to in order for `stat' and other things to work. /
/
#undef MPL__POSIX_SOURCE */

and

/* Define which x86 cycle counter to use /
/
#undef _mpl_LINUX86_CYCLE_CPUID_RDTSC32 /
/
Define which x86 cycle counter to use /
/
#undef _mpl_LINUX86_CYCLE_CPUID_RDTSC64 /
/
Define which x86 cycle counter to use /
/
#undef _mpl_LINUX86_CYCLE_RDTSC /
/
Define which x86 cycle counter to use /
/
#undef _mpl_LINUX86_CYCLE_RDTSCP */

Ahh... numbers. I'll fix it. One thing for rolling our own is lack of testing, but we are getting there...

@hzhou hzhou force-pushed the hzhou:1906_ax_prefix branch from 55f10a1 to 0d253bb Jun 19, 2019

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

test:jenkins/ch3/most
test:jenkins/ch4/most

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2019

test:jenkins/ch4/most

@raffenet

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

I'm confused why these values get prefixed:

/* Define to 1 if on MINIX. */
/* #undef MPL__MINIX */

/* Define to 2 if the system does not provide POSIX.1 features except with
   this defined. */
/* #undef MPL__POSIX_1_SOURCE */

/* Define to 1 if you need to in order for `stat' and other things to work. */
/* #undef MPL__POSIX_SOURCE */

while these do not:

/* Enable extensions on AIX 3, Interix.  */
#ifndef _ALL_SOURCE
# define _ALL_SOURCE 1
#endif
/* Enable GNU extensions on systems that have them.  */
#ifndef _GNU_SOURCE
# define _GNU_SOURCE 1
#endif
/* Enable threading extensions on Solaris.  */
#ifndef _POSIX_PTHREAD_SEMANTICS
# define _POSIX_PTHREAD_SEMANTICS 1
#endif
/* Enable extensions on HP NonStop.  */
#ifndef _TANDEM_SOURCE
# define _TANDEM_SOURCE 1
#endif
/* Enable general extensions on Solaris.  */
#ifndef __EXTENSIONS__
# define __EXTENSIONS__ 1
#endif
@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

I'm confused why these values get prefixed:

/* Define to 1 if on MINIX. */
/* #undef MPL__MINIX */

while these do not:

/* Enable extensions on AIX 3, Interix.  */
#ifndef _ALL_SOURCE
# define _ALL_SOURCE 1
#endif

Because they are of different patterns from those generated by AC_DEFINE (notice the space between # and define). I believe AX_PREFIX_CONFIG_H doesn't touch them either. These macros (_ALL_SOURCE, ...) are not used by our code; rather they are directives for C compiler; they won't work if we add prefix.

ref: AC_USE_SYSTEM_EXTENSIONS

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

Oh, I see. What you meant was that we shouldn't touch _MINIX, _POSIX_1_SOURCE, and _POSIX_SOURCE either?

I could modify the scripts to exempt macros start with _, just like how it did with inline, const and restrict.

hzhou added some commits Jun 18, 2019

config: further tuning of ax_prefix_config_h
* avoid double prefix
* add prefix to the #undef inside comments
* direct define, matches AC_CONFIG_HEADER, deviates from AX_PREFIX_CONFIG_H
config: use c99 keywords without prefix
AX_PREFIX_CONFIG_H adds `_mpl_` or `_opa_` to substitute keywords
`inline`, `const`, and `restrict`. Now we use custom scripts,
`cmd_prefix_config_h.pl`, we can easily use the keywords as they are to
make the code (much) cleaner.

@hzhou hzhou force-pushed the hzhou:1906_ax_prefix branch from 0d253bb to ae4f263 Jun 25, 2019

@hzhou

This comment has been minimized.

Copy link
Contributor Author

commented Jun 25, 2019

test:jenkins/ch3/sock

@raffenet

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

Output from this one looks good. I'm going to merge.

@raffenet raffenet merged commit bf61c88 into pmodels:master Jun 25, 2019

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
mpich-review-ch3-sock Build finished. 7020 tests run, 359 skipped, 0 failed.
Details
whitespace-checker Build finished. No test results found.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.