-
Notifications
You must be signed in to change notification settings - Fork 397
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
Modifications to ensure compatibility with OpenXL and xlC on AIX #7561
Conversation
@zl-wang here is the new OpenXL PR for OMR. Marked as a WIP since I'm still addressing the remaining review comments from Ishita's PRs |
5340b8e
to
7615492
Compare
I've just finished addressing the review comments from the previous version of this PR. @keithc-ca @zl-wang when you have a moment could you take a look to ensure that I didn't miss anything/provide any further feedback? |
configure
Outdated
CC="$ac_save_CC $ac_arg" | ||
if ac_fn_c_try_compile "$LINENO"; then : | ||
ac_cv_prog_cc_c89=$ac_arg | ||
if [ "$OMR_ENV_OPENXL" == "1" ]; then |
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 be derived from configure.ac
(which is not modified by this change).
How would OMR_ENV_OPENXL
be defined? (Hint: it's not from OmrDetectSystemInformation.cmake
).
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.
I'm not sure if this is the solution you have in mind, but I opted to replace the check on OMR_ENV_OPENXL
with the following condition:
if [[ "$CC" =~ 'ibm-clang' ]] && [[ "$CXX" =~ 'ibm-clang' ]]; then
since CC
and CXX
are both used in configure.ac
, and when OpenXL being used, they should be set to the following:
* C Compiler: Version 17.0.6 (at /opt/IBM/openxlC/17.1.2/bin/ibm-clang_r)
* C++ Compiler: Version 17.0.6 (at /opt/IBM/openxlC/17.1.2/bin/ibm-clang++_r)
Is this a viable alternative?
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.
watch out for possible conflict with zOS using wyvern compiler:
- do they have the same names?
- if yes, can they use whatever under that condition?
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.
The zOS installation of OpenXL does indeed use the same name. And I don't think there's anything under that condition that's incompatible with zOS, but I will test that out before we proceed just to make sure.
c1a8ebd
to
d3634f0
Compare
d3634f0
to
d5cd947
Compare
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.
Overall looks fine to me; just had some questions and minor suggestions.
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.
LGTM, could you squash the commits?
To ensure that OMR can be built on AIX with both OpenXL and xlC, make modifications to compiler flags, macros, and linked libraries. Signed-off-by: midronij <[email protected]>
Initialize nlen, tindex, and myTag in TR_PPCTableOfConstants::lookUp() to prevent unexpected behavior when building with OpenXL on AIX. Signed-off-by: midronij <[email protected]>
9e8ec74
to
62621c9
Compare
jenkins build all |
Windows and z/OS builds failed due to a lack of machines. RV ran into some groovy error Jenkins build riscv |
The RV problem happened again, but it looks like it's been happening in other PR builds as well |
Modify compiler flags, macros, and linked libraries to ensure that OMR can be built with both OpenXL and xlC on AIX.
Updated version of #7447 and #7480