-
Notifications
You must be signed in to change notification settings - Fork 396
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
Add Open XL toolchain and config changes for z/OS #7319
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for supporting the project, and congratulations on your first contribution! A project committer will shortly review your contribution. In the mean time, if you haven't had a chance please skim over the contribution guidelines which all pull requests must adhere to. If the ECA pull request check fails, have a look at the instructions for signing the ECA in the legal considerations section.
If you run into any problems our community will be happy to assist you in any way we can. There are a number of recommended ways to interact with the community. We encourage you to ask questions, or drop by to say hello.
3f0b73e
to
c39496d
Compare
3ccf780
to
e34c258
Compare
jenkins build all |
b9fb8d3
to
f7c2ff6
Compare
ff73ec3
to
c1479dd
Compare
jenkins build all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High level feedback:
- Squash the two commits into a single commit.
- Revise and improve comments.
- Remove dead code.
-m64 | ||
) | ||
else() | ||
# -qarch should be there for 32 and 64 C/CXX flags but the C compiler is used for the assembler and it has trouble with some assembly files if it is specified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Spread comments longer than 100 chars across multiple lines.
- End comments with a full stop / period.
# -qarch should be there for 32 and 64 C/CXX flags but the C compiler is used for the assembler and it has trouble with some assembly files if it is specified | |
# -qarch should be there for 32 and 64 C/CXX flags but the C compiler is used for | |
# the assembler and it has trouble with some assembly files if it is specified. |
#"\"-Wc,xplink\"" # link with xplink calling convention | ||
#"\"-Wc,rostring\"" # place string literals in read only storage | ||
#"\"-Wc,FLOAT(IEEE,FOLD,AFP)\"" # Use IEEE (instead of IBM Hex Format) style floats | ||
#"\"-Wc,enum(4)\"" # Specifies how many bytes of storage enums occupy | ||
#"\"-Wa,goff\"" # Assemble into GOFF object files | ||
#"\"-Wc,NOANSIALIAS\"" # Do not generate ALIAS binder control statements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove dead code instead of commenting it.
string(APPEND CMAKE_ASM_FLAGS " \"-Wa,-mgoff\"") | ||
string(APPEND CMAKE_ASM_FLAGS " \"-Wa,-mSYSPARM(BIT64)\"") | ||
|
||
# commenting out options progressively as they are not needed with Open XL, can remove in cleanup later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is off, but this comment should go away since dead code will be removed.
endfunction() | ||
else() | ||
function(_omr_toolchain_process_exports TARGET_NAME) | ||
# we only need to do something if we are dealing with a shared library |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First person should be avoided since it is informal and takes attention away from the key point.
@keithc-ca Can you please review this PR? |
set(CMAKE_C_COMPILER_IS_OPENXL ON CACHE BOOL "ibm-clang is the C compiler") | ||
set(_OMR_TOOLCONFIG "openxl") | ||
else() | ||
message(STATUS "NO MATCH") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should be fatal (like line 219)?
Actually, I think line 219 should use CMAKE_C_COMPILER_ID
instead of CMAKE_CXX_COMPILER_ID
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Keith, I was verifying all my cleanup changes with different builds. After making the above-mentioned changes, while Open XL passes with no problems, XLC build gives a fatal error, because I guess it is entering this z/OS section, but not matching with the other xlclang compilers and in place of the "NO MATCH" its just throwing the error.
Perhaps it makes more sense to update this to
message(STATUS "NO XLClang match. Using xlc toolchain.")
with the above, both XLC / OpenXL are now compiling successfully.
Does this sound good?
|
||
# Copyright (c) 2017, 2024 IBM Corp. and others | ||
# | ||
# This program and the accompanying materials are made available under | ||
# the terms of the Eclipse Public License 2.0 which accompanies this | ||
# distribution and is available at http://eclipse.org/legal/epl-2.0 | ||
# or the Apache License, Version 2.0 which accompanies this distribution | ||
# and is available at https://www.apache.org/licenses/LICENSE-2.0. | ||
# | ||
# This Source Code may also be made available under the following Secondary | ||
# Licenses when the conditions for such availability set forth in the | ||
# Eclipse Public License, v. 2.0 are satisfied: GNU General Public License, | ||
# version 2 with the GNU Classpath Exception [1] and GNU General Public | ||
# License, version 2 with the OpenJDK Assembly Exception [2]. | ||
# | ||
# [1] https://www.gnu.org/software/classpath/license.html | ||
# [2] http://openjdk.java.net/legal/assembly-exception.html | ||
# | ||
# SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 OR LicenseRef-GPL-2.0 WITH Assembly-exception | ||
############################################################################### |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the right copyright notice. The year should be 2024 (since this file is new); we don't need blank line 2; and the URLs and SPDX are wrong. This should be copied from an existing file.
# Testarossa build variables. Longer term the distinction between TR and the rest | ||
# of the OMR code should be heavily reduced. In the mean time, we keep | ||
# the distinction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Punctuation.
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one blank line, please.
set(OMR_C_WARNINGS_AS_ERROR_FLAG -qhalt=w) | ||
set(OMR_CXX_WARNINGS_AS_ERROR_FLAG -qhalt=w) | ||
|
||
# There is no enhanced warning for XLC right now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should not be used for xlc: why is that compiler even mentioned here?
ddr/Makefile
Outdated
@@ -26,7 +26,7 @@ include $(top_srcdir)/omrmakefiles/configure.mk | |||
|
|||
OMR_TOOLCHAIN ?= gcc | |||
|
|||
ifneq (,$(filter gcc xlc,$(OMR_TOOLCHAIN))) | |||
ifneq (,$(filter gcc xlc openxl,$(OMR_TOOLCHAIN))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we care about non-cmake builds with OpenXL? If not, the remaining changes should be removed.
5ce9dec
to
a47dfb7
Compare
The commit message and the discussion here should refer to "z/OS", not "zos". |
57cc58e
to
69a94df
Compare
Thanks for the feedback/cleanup suggestions. I've addressed the above comments and verified builds are passing. |
omr_remove_flags(CMAKE_ASM_FLAGS -qhalt=e) | ||
omr_remove_flags(CMAKE_CXX_FLAGS -qhalt=s) | ||
omr_remove_flags(CMAKE_C_FLAGS -qhalt=e) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest languages should be addressed in a consistent order; see lines 34-35 and elsewhere, where C is handled before C++.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reordered to put C_FLAGS first as seen in other places.
# -march should be there for 32 and 64 C/CXX flags but the C compiler is used for | ||
# the assembler and it has trouble with some assembly files if it is specified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this comment reference 64-bit compiles here in the 32-bit branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed along with the rest of "ppc" section, as POWER support will be contributed by another developer as mentioned below.
endmacro(omr_toolconfig_global_setup) | ||
endif() | ||
|
||
if(OMR_HOST_ARCH STREQUAL "ppc") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has any of this been tested on POWER? I see several -q
options which appear to be in the xlc style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another dev (Jackie) is working on support for POWER, as such I will be removing this untested section from here. Believe she has commits that are adding support separately so this section should be not here.
set(CMAKE_C_ARCHIVE_CREATE "<CMAKE_AR> -X64 cr <TARGET> <LINK_FLAGS> <OBJECTS>") | ||
set(CMAKE_C_ARCHIVE_FINISH "<CMAKE_RANLIB> -X64 <TARGET>") | ||
endif() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please omit this blank line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed extra line.
|
||
set(CMAKE_ASM_FLAGS "-fno-integrated-as") | ||
string(APPEND CMAKE_ASM_FLAGS " \"-Wa,-mgoff\"") | ||
string(APPEND CMAKE_ASM_FLAGS " \"-Wa,-mSYSPARM(BIT64)\"") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't BIT64
be conditional on OMR_ENV_DATA64
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the missing conditional
configure
Outdated
@@ -1727,7 +1727,8 @@ Some influential environment variables: | |||
Specifies whether the package will run in 32- or 64-bit mode. | |||
One of: 31,32,64 | |||
OMR_TOOLCHAIN | |||
The toolchain used to build the package. One of: gcc,xlc,msvc | |||
The toolchain used to build the package. One of: | |||
gcc,xlc,msvc,openxl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should not be modified: it's not used in cmake builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
configure.ac
Outdated
@@ -47,7 +47,7 @@ AC_ARG_VAR([exe_output_dir], [The output directory for executables. (Default: $t | |||
AC_ARG_VAR([OMR_HOST_OS], [The operating system where the package will run. One of: aix,linux,osx,win,zos]) | |||
AC_ARG_VAR([OMR_HOST_ARCH], [The architecture of the CPU where the package will run. One of: aarch64,arm,ppc,riscv,s390,x86]) | |||
AC_ARG_VAR([OMR_TARGET_DATASIZE], [Specifies whether the package will run in 32- or 64-bit mode. One of: 31,32,64]) | |||
AC_ARG_VAR([OMR_TOOLCHAIN], [The toolchain used to build the package. One of: gcc,xlc,msvc]) | |||
AC_ARG_VAR([OMR_TOOLCHAIN], [The toolchain used to build the package. One of: gcc,xlc,msvc,openxl]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should not be modified: it's not used in cmake builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
This change will require changes from eclipse#7319 Signed-off-by: Gaurav Chaudhari <[email protected]>
8e4853d
to
bcdb1e7
Compare
Should have addressed all the comments above, also changed to use |
Adds the initial set of changes to support compilation with Open XL on z/OS. This will define the alternative flags and configuration that needs to be used by the wyvern compiler while running on z/OS. Signed-off-by: Gaurav Chaudhari <[email protected]>
Added conditional |
@keithc-ca Addressed above comments and ready for review. Awaiting approval on this from your side, then will reach out to Babneet for reviewing again and merging. |
if(CMAKE_C_COMPILER_IS_XLCLANG) | ||
macro(omr_toolconfig_global_setup) | ||
# For XLClang, remove any usages of -qhalt=e or -qhalt=s provided by default | ||
# in the CMAKE CXX/C/ASM FLAGS, since xlclang/xlclang++ are not compatible | ||
# with the e or s options. | ||
omr_remove_flags(CMAKE_ASM_FLAGS -qhalt=e) | ||
omr_remove_flags(CMAKE_C_FLAGS -qhalt=e) | ||
omr_remove_flags(CMAKE_CXX_FLAGS -qhalt=s) | ||
endmacro(omr_toolconfig_global_setup) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should not be used when CMAKE_C_COMPILER_IS_XLCLANG
(in which case OMR_TOOLCONFIG
is xlc
). All the resulting dead code should be removed from this file.
Even after such changes this would still use -q
options, which I believe are for xlc, not OpenXL.
@@ -198,19 +198,28 @@ macro(omr_detect_system_information) | |||
# just use GNU config | |||
set(_OMR_TOOLCONFIG "gnu") | |||
endif() | |||
elseif(CMAKE_C_COMPILER_ID MATCHES "IBMClang$") | |||
set(_OMR_TOOLCONFIG "openxl") | |||
set(CMAKE_C_COMPILER_IS_OPENXL ON CACHE BOOL "ibm-clang is the C compiler") | |||
elseif(CMAKE_C_COMPILER_ID MATCHES "^XL(Clang)?$" OR CMAKE_C_COMPILER_ID STREQUAL "zOS") | |||
# In CMake 3.14 and prior, XLClang uses CMAKE_C_COMPILER_ID "XL" | |||
# In CMake 3.15 and beyond, XLClang uses CMAKE_C_COMPILER_ID "XLClang" | |||
set(_OMR_TOOLCONFIG "xlc") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This belongs within the if
block for xlc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, does this mean the check would be updated as such:
elseif(CMAKE_C_COMPILER_ID MATCHES "^XL(Clang)?$" OR CMAKE_C_COMPILER_ID MATCHES "IBMClang$" OR CMAKE_C_COMPILER_ID STREQUAL "zOS")
I only had it as a separate clause because it could not enter the elseif
mentioned above, and the COMPILER_ID was ending up to be IBMClang
when running with Open XL via a jenkins build.
Adds the initial set of changes to support compilation with Open XL on z/OS. This will define the alternative flags and configuration that needs to be used by the wyvern compiler while running on z/OS.
(This is one part of the multiple changes added for supporting Open XL compilation on OMR)