Allow setting CC, CXX, and other variables to indicate compiler path #17923

mppf · 2021-06-16T15:40:38Z

This PR implements changes to how users can indicate a particular path to
a particular compiler. It is sort of a follow-up to PR #17800 which added
CHPL_LLVM_PREFIX for selecting an LLVM installation.

This PR makes the following variables settable by users (as environment
variables or in a chplconfig):

for setting all the compilers (host and target) at once using
relatively well known environment variable names:
- CC, CXX
for setting host and target compilers separately:
- CHPL_HOST_CC, CHPL_HOST_CXX
- CHPL_TARGET_CC, CHPL_TARGET_CXX
for indicating which system LLVM installation should be used / the
path to it by providing the path to the llvm-config command:
- CHPL_LLVM_CONFIG

Additionally it does some clean-ups for LLVM Makefiles.

For printchplenv, the new variables are in the compiler or runtime
categories but aren't printed by default (so printchplenv --all will
print them). This PR adjusts printchplenv.py to include an idea that
some variables shouldn't be in --path output since it doesn't make
sense to store the path to CC say in the compiler/runtime build path.

PR #17931 contains the documentation changes to go along with these changes.

Reviewed by @daviditen - thanks!

full local testing

Tricky Pieces:

are we happy with the names CHPL_HOST_CC etc ? These names are
chosen to both try to be intuitive / map from CC / CXX but also
because autotools projects might have HOSTCC that has a different
meaning. (At least autotools arguably a different meaning for host
than what we use). An interesting alternative is CHPL_CC_HOST but
CHPL_HOST_CC fits better with CHPL_TARGET_COMPILER.
should printchplenv --all show CHPL_TARGET_CC etc and
CHPL_LLVM_CONFIG ? In the PR it does, but these don't show up with
printchplenv without --all.
should printchplenv show CC or CXX being set in some way? In the
PR it does not, other than their values showing up in CHPL_TARGET_CC
etc.
If one of CC and CXX is set but not both, should we warn? The PR
does warn for this unless the CC value is the default for that
compiler family (e.g. CC=gcc means we know CXX=g++). I think the
warning is important because the variable CC is more well known and
we have lots of code that might use a mix of C and C++ (e.g. in the
compiler or in the runtime when including C++ libraries like RE2)
If e.g. CHPL_TARGET_CC is set but CHPL_TARGET_COMPILER is not,
should we infer CHPL_TARGET_COMPILER the way we would for CC?
Currently the PR does not do this and additionally it does not warn in
that case.

Future Work:

adding testing for setting CHPL_HOST_CC / CC / CHPL_LLVM_CONFIG etc.

* fold LLVM_VERSION into chpl_llvm.py * remove find-llvm-config.sh and write it in chpl_llvm.py * clean up compiler selection --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

e.g. CHPL_TARGET_COMPILER_COMMAND_CXX was clang++ Just removed _COMMAND_s from this file and additionally handled the + and consolidated the sed substitutions. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

* removes some unused makefiles * changes from test/run strategy to ifdef HAVE_LLVM_CONFIG * adds an error for building LLVM with itself * gets LLVM_CONFIG, CHPL_LLVM_CLANG_C/CXX from printchlplenv * stop installing find-llvm-config.sh and LLVM_VERSION --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

don't want CHPL_LLVM_CONFIG / CHPL_HOST_CC etc to end up in build paths. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

To include the new variables with --runtime / --compiler but not in those combine with --path. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Follow-up to commit Avoid problems with + in chpl-env-gen.h Replace . with _ and don't make defines for CHPL_*_CC / CHPL_*_CXX --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

for not having CHPL_HOST_CC etc in the chpl-env-gen.h --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

stonea

Hi Michael,

Just a few questions I have about this PR:

I assume host refers to the compiler being used to compile Chapel (the compiler) itself and target refers to the compiler used to compile emitted .c\llvmir from Chapel. What is CHPL_MAKE_COMPILER then?
Is CHPL_LLVM_CONFIG meant to only set the path for LLVM is or is intended to set some other parameters? If just the path I think I'd name it something like CHPL_LLVM_PATH
Do we have tests that cover all these different configuration options? I guess a lot of it gets covered with our Jenkins configurations. Does anything test CHPL_LLVM_CONFIG? I imagine getting a test set up for that might be a bit of a pain, but it also seems like the sort of thing that could regress and no one would catch it since most of us are probably either using the bundled LLVM or the default system LLVM.

Just a brainstorming idea:

It seems like we need to have a lot of logic spread throughout the makefiles to validate \ set defaults to these environmental variables. I think if I added a new variable it may not be obvious to me where to configure it, and having all this logic embedded inside makefiles may make it harder to move to a different make system (cmake or something) later on if we decide to do that. Would it be worth factoring out all this logic into a single Python script? The make system could then just invoke this script and if it returns successfully assume everything has been configured sanely and proceed.

mppf · 2021-06-23T14:35:32Z

I assume host refers to the compiler being used to compile Chapel (the compiler) itself and target refers to the compiler used to compile emitted .c\llvmir from Chapel. What is CHPL_MAKE_COMPILER then?

Yes, that's right. CHPL_MAKE_COMPILER is used inside the Makefiles and is the compiler appropriate for the current job (which is either compiling the compiler, the runtime, or a generated C program).

Also, a nit, in the case of the LLVM backend, we generate LLVM IR and then emit an object file and use CHPL_TARGET_COMPILER to link.

Is CHPL_LLVM_CONFIG meant to only set the path for LLVM is or is intended to set some other parameters? If just the path I think I'd name it something like CHPL_LLVM_PATH

It sets the path to the llvm-config command in a corresponding way to CC / CHPL_CC_HOST. I am not excited about changing it to CHPL_LLVM_PATH both because it's after the deadline I asked for comments on the design and also because it would cause me to expect that setting it to say /usr/local/opt/llvm-11 would work. But that is not what the variable means - one needs to provide /usr/local/opt/llvm-11/bin/llvm-config in that event. Note that the program is not always called literally llvm-config and at a different path. On some systems it might be called llvm-config-11 for example.

This is described in the accompanying documentation PR -- https://github.com/chapel-lang/chapel/pull/17931/files#diff-b71d5b3486fed20a9661b02deca46e394853b16cabd6a320e987ea14fbbc067bR711

Do we have tests that cover all these different configuration options? I guess a lot of it gets covered with our Jenkins configurations. Does anything test CHPL_LLVM_CONFIG? I imagine getting a test set up for that might be a bit of a pain, but it also seems like the sort of thing that could regress and no one would catch it since most of us are probably either using the bundled LLVM or the default system LLVM.

CHPL_LLVM_CONFIG is set by the llvm-by-default detection of the system LLVM and we have that working in a few different ways on different systems. As a result I would expect we would notice if there is something severely wrong with it.

About the testing, the same could be said for CC / CXX / CHPL_HOST_CC etc. I agree it would be nice to add tests for these but I'm not quite sure how to do. I am open to feedback but I would not consider it a requirement for this PR.

Just a brainstorming idea:

It seems like we need to have a lot of logic spread throughout the makefiles to validate \ set defaults to these environmental variables. I think if I added a new variable it may not be obvious to me where to configure it, and having all this logic embedded inside makefiles may make it harder to move to a different make system (cmake or something) later on if we decide to do that. Would it be worth factoring out all this logic into a single Python script? The make system could then just invoke this script and if it returns successfully assume everything has been configured sanely and proceed.

Yes I am of the opinion that we are better off if we can gradually move some of this logic to util/chplenv Python code. This PR does that for the find-llvm-config.sh script. I think it'd be nice to move the compiler flags we set for different compilers to the Python code as well but I am uncertain if that will have broad support.

daviditen

Looks good, thanks!

@daviditen

Documentation changes for CC and LLVM_CONFIG settings This PR contains the documentation changes to go along with PR #17923. Reviewed by @daviditen - thanks!

stonea · 2021-06-24T14:29:24Z

Is CHPL_LLVM_CONFIG meant to only set the path for LLVM is or is intended to set some other parameters? If just the path I think I'd name it something like CHPL_LLVM_PATH

It sets the path to the llvm-config command in a corresponding way to CC / CHPL_CC_HOST. I am not excited about changing it to CHPL_LLVM_PATH both because it's after the deadline I asked for comments on the design and also because it would cause me to expect that setting it to say /usr/local/opt/llvm-11 would work. But that is not what the variable means - one needs to provide /usr/local/opt/llvm-11/bin/llvm-config in that event. Note that the program is not always called literally llvm-config and at a different path. On some systems it might be called llvm-config-11 for example.

I see, thanks for clarifying.

About the testing, the same could be said for CC / CXX / CHPL_HOST_CC etc. I agree it would be nice to add tests for these but I'm not quite sure how to do. I am open to feedback but I would not consider it a requirement for this PR.

For sure, if we wanted to add tests that would be its own project. I just get nervous when code is submitted without any sort of regression test. But this is the sort of thing where the time investment to make a test may not outweigh the risk of regression and maybe our existing Jenkins configurations already covers a lot of what that kind of regression test would be used for.
If we wanted to go that route. I imagine we’d need to do the following:

To test what the host compiler is we could have some kind of hidden flag you pass to chpl that would have it print it out. Just use compiler specific macros like __clang__ or __GNUC__ to discover what compiler is being used. We’d then have to configure Jenkins to build chpl in a couple different compilers (I think maybe we already do?) and run this test.
To test the target compiler have a flag passed to chpl that prints out the commands used to compile (I think we might already have this).

@daviditen

Fix bundled LLVM builds Fixes `CHPL_LLVM=bundled` builds after PR #17923. * Allows the configured `CHPL_LLVM_CONFIG` to not exist yet for `CHPL_LLVM=bundled` without giving an error * Improves the errors from `make` for a failed `printchplenv` to be less confusing Reviewed by @daviditen - thanks! - [x] a `CHPL_LLVM=bundled` build now works and `make check` succeeds

@daviditen

Relax - cannot set CHPL_LLVM_CONFIG with bundled Follow-up to PR #17923. * changes the error into a warning * adjusts get_llvm_config to always set `CHPL_LLVM_CONFIG` for bundled * resolves a problem with failing to run the prediff for test/compflags/link/sungeun/static_dynamic among others Reviewed by @daviditen - thanks! - [x] full local testing

ronawho · 2021-07-10T14:17:16Z

util/chplenv/chpl_compiler.py

+            error("Conflicting compiler families for CC and CXX settings\n"
+                  "  {0} -> {1}\n"
+                  "  {2} -> {3}".format(cc_val, cc_val_command,
+                                        cxx_val, cxx_val_command))


Looks like cc_val_command and cxx_val_command are never defined so this result in a NameError exception and not our error message

PR #18052 will fix, thanks for pointing it out

@ronawho

Fix error in error handling for CC/CXX conflict To resolve the issue brought up by @ronawho in #17923 (review) Reviewed by @ronawho - thanks! - [x] full local testing

bradcray · 2021-07-17T07:07:23Z

Hi @mppf — It seems that something about this PR broke my fix for LLVM@11 on Mac Mojave (PR #17851). It took me from this afternoon until now to bisect it down to this PR, and I haven't looked into the PR itself to try to understand how it could've (and probably won't before vacation). Most likely guess would be that something about how util/config/gather-clang-sysroot-arguments is invoked or gets used to build up CFLAGS-like lists changed. Any guesses?

(I specifically see the problem when hwloc tries to configure itself, but I'm not sure whether it's anything specific to hwloc, or whether that's just the first attempt to link with clang that we get during the build).

Tagging @ronawho on this, as he was offering ideas to debug this today. I think this slipped by me until now simply because hwloc doesn't need to be rebuilt very often (?).

mppf · 2021-07-19T13:39:26Z

I don't know what's going wrong and would want to investigate further on a system showing the issue but I don't have one of those...

bradcray · 2021-08-07T01:00:42Z

In case others come across the last few comments, I've taken the conversation to https://github.com/Cray/chapel-private/issues/2069#issuecomment-894555342.

@mppf

Make CC include `configured-clang-sysroot-arguments` for CHPL_LLVM builds; re-fix clang for Mojave [reviewed by @mppf] PR #17923 had the impact of making CC store a full path to `clang`, where previously, it had just used `clang`. When using the homebrew version of `clang` on Mac systems, this had the impact of re-breaking Mac builds on Mojave systems—re-opening Cray/chapel-private#2069. (or "on my Mojave system" at least; in that prior to this, my system `clang` seemed to be used when building the runtime and third-party directories, due to being earlier in the path, and it worked out of the box with no special flags). Once the homebrew version was used for these directories, it started giving the `ld: unknown option: -platform_version` errors again due to the `./configure` steps in the third-party packages because the `-mlinker-version=450` flag wasn't being given to it in these contexts. This PR fixes this by having the `chpl_llvm.py` script always associate the contents of `configured-clang-sysroot-arguments` with `clang` when setting the `CC` variable. This causes all compilations using the homebrew version of `clang` to include `-mlinker-version=450` when appropriate / necessary on Mojave, using the previous logic that added that option into `configured-clang-sysroot-arguments` in #17851. It removes some now redundant logic from the Makefiles which was doing this for some, but not all directories. (This change also has the side effect of adding other options that are currently in `configured-clang-sysroot-arguments`—namely `-isysroot /Library/Developer/CommandLineTools/SDKs/MacOSX10.14.sdk -resource-dir /Library/Developer/CommandLineTools/usr/lib/clang/10.0.1` to the CC command line because they were also in the file. These do not seem to be strictly necessary, but also seem to do no harm apart from making the compiler's command line longer than necessary). Resolves Cray/chapel-private#2069

@daviditen

Improve compiler configuration This PR takes several steps to improve the configuration visible in `printchplenv`. It is intended to resolve the following issues: * #18450 * Cray/chapel-private#2520 * Cray/chapel-private#2529 * #18455 It takes the following steps: * Adjusts make/compiler/Makefile.clang to avoid running the compiler when `CHPL_TARGET_COMPILER=llvm` in order to avoid problems with `make clobber` with `CHPL_LLVM=bundled`. We can avoid running the compiler since we only build with LLVM 11+ and the associated clang defaults to C17 and C++14. In the future, it would be nice to move this logic to the Python scripts, but I did not take that on here. * Removes CPU feature tables since these have not been properly updated in a long time and have limited benefit. This reduces maintenance and the likelihood a user will run into problems when using an architecture not specifically tested. However, removing these feature tables means we can no longer give warnings/errors when the configuration requested appears to be something that could not run. * Uses a simple table to map PrgEnv CPU names to clang/llvm architecture names. This is a stopgap solution until we find a better way. * Changes CC/CXX support (added in PR #17923) to make setting these variables have less of a confusing impact (per discussion in #18450): * they do not prevent `CHPL_TARGET_COMPILER` defaulting to `llvm` when that makes sense * they are unused if other `CHPL_*_COMPILER` / `CHPL_*_CC` / `CHPL_*_CXX` are set * they apply only to the host configuration when working on a system using a PrgEnv compiler Reviewed by @daviditen - thanks! - [x] test on a Cray PrgEnv system - [x] full local testing

mppf force-pushed the compiler-cc branch 2 times, most recently from 03bd585 to 9aacc0a Compare June 16, 2021 21:26

mppf mentioned this pull request Jun 17, 2021

Documentation changes for CC and LLVM_CONFIG settings #17931

Merged

This comment has been minimized.

Sign in to view

mppf added 14 commits June 17, 2021 16:07

Update LLVM config python

9162be9

* fold LLVM_VERSION into chpl_llvm.py * remove find-llvm-config.sh and write it in chpl_llvm.py * clean up compiler selection --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Avoid problems with + in chpl-env-gen.h

056c014

e.g. CHPL_TARGET_COMPILER_COMMAND_CXX was clang++ Just removed _COMMAND_s from this file and additionally handled the + and consolidated the sed substitutions. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Allow setting CC/CXX

06fc1de

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

use table, improve errors/warnings

acf15d2

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

CHPL_TARGET_COMPILER_COMMAND_C -> CHPL_TARGET_CC etc

3b3e262

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Fix problems with CHPL_LLVM_CONFIG for LLVM=bundled

55a53d0

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Fix problem with CHPL_LLVM=system

535c470

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Add different category for the new printchplenv vars

6f7fb9a

don't want CHPL_LLVM_CONFIG / CHPL_HOST_CC etc to end up in build paths. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Add nopath set for printchplenv vars

1cb05d3

To include the new variables with --runtime / --compiler but not in those combine with --path. --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Remove Warning: from a fatal error

1caf67e

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Fix more problems with chpl-env-gen.h

20d2add

Follow-up to commit Avoid problems with + in chpl-env-gen.h Replace . with _ and don't make defines for CHPL_*_CC / CHPL_*_CXX --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Fix test/llvm/llvmDebug for removing LLVM_VERSION

8ef97cd

--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

Fix test/runtime/sungeun/chpl-env-gen.chpl

dbd642a

for not having CHPL_HOST_CC etc in the chpl-env-gen.h --- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

mppf force-pushed the compiler-cc branch from 8b73d85 to dbd642a Compare June 17, 2021 20:22

stonea reviewed Jun 23, 2021

View reviewed changes

daviditen approved these changes Jun 23, 2021

View reviewed changes

mppf merged commit 18519a0 into chapel-lang:main Jun 23, 2021

mppf deleted the compiler-cc branch June 23, 2021 19:11

mppf added a commit that referenced this pull request Jun 23, 2021

Merge pull request #17931 from mppf/docs-17923

7a31068

Documentation changes for CC and LLVM_CONFIG settings This PR contains the documentation changes to go along with PR #17923. Reviewed by @daviditen - thanks!

mppf mentioned this pull request Jun 24, 2021

Fix bundled LLVM builds #17979

Merged

1 task

mppf mentioned this pull request Jun 28, 2021

Relax - cannot set CHPL_LLVM_CONFIG with bundled #17993

Merged

1 task

ronawho reviewed Jul 10, 2021

View reviewed changes

mppf mentioned this pull request Jul 12, 2021

Fix error in error handling for CC/CXX conflict #18052

Merged

1 task

bradcray mentioned this pull request Aug 9, 2021

Make CC include configured-clang-sysroot-arguments for CHPL_LLVM builds #18187

Merged

This was referenced Sep 30, 2021

Set compiler name during build #12777

Closed

Should setting CC / CXX request the C backend? #18450

Closed

mppf mentioned this pull request Oct 8, 2021

Improve compiler configuration #18485

Merged

2 tasks

mppf mentioned this pull request Mar 28, 2023

Fix problem running 'make' with CC and CXX set #21985

Merged

2 tasks

bradcray mentioned this pull request May 14, 2024

Fix typo: 'pgicc' -> 'pgcc' #25042

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting CC, CXX, and other variables to indicate compiler path #17923

Allow setting CC, CXX, and other variables to indicate compiler path #17923

mppf commented Jun 16, 2021 •

edited

This comment has been minimized.

stonea left a comment

mppf commented Jun 23, 2021

daviditen left a comment

stonea commented Jun 24, 2021

ronawho Jul 10, 2021

mppf Jul 12, 2021 •

edited

bradcray commented Jul 17, 2021

mppf commented Jul 19, 2021

bradcray commented Aug 7, 2021

Allow setting CC, CXX, and other variables to indicate compiler path #17923

Allow setting CC, CXX, and other variables to indicate compiler path #17923

Conversation

mppf commented Jun 16, 2021 • edited

This comment has been minimized.

stonea left a comment

Choose a reason for hiding this comment

mppf commented Jun 23, 2021

daviditen left a comment

Choose a reason for hiding this comment

stonea commented Jun 24, 2021

ronawho Jul 10, 2021

Choose a reason for hiding this comment

mppf Jul 12, 2021 • edited

Choose a reason for hiding this comment

bradcray commented Jul 17, 2021

mppf commented Jul 19, 2021

bradcray commented Aug 7, 2021

mppf commented Jun 16, 2021 •

edited

mppf Jul 12, 2021 •

edited