-
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
Create macro to identify openXL compiler #7480
base: master
Are you sure you want to change the base?
Conversation
# TODO we don't actually have a clang config | ||
# just use GNU config | ||
set(_OMR_TOOLCONFIG "gnu") | ||
elseif(CMAKE_C_COMPILER_ID MATCHES "^Clang$") |
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.
Z system will use OpenXL compiler too. Have you make sure either of the two below?
- Z system will not use Clang compiler ID for that expected switch; or
- this config setting is applicable to them as well.
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.
@r30shah please comment ...
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.
Yes, I believe the COMPILER_ID on Z will be IBMClang
and TOOLCONFIG will be set to openxl
. I would like @Deigue to confirm this.
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 didn't consider that the COMPILER_ID can be different for Z system.
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.
you had better get together with @Deigue
either, both can use the same ID, if we can;
or, AIX uses a more differentiating and meaningful ID, rather than this very generic "Clang"
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 can't find any existing toolconfig for "openxl" that's why I used "gnu" for 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.
I've made the compiler id related changes here: #7319 ,
also will be moving the "IBMClang$"
somewhere alongside the xlc clause so that all that compiler id variations can navigate to openxl toolchain easily from one spot based on Keiths feedback.
From what I remember, I was seeing zOS
as the compiler ID while running locally, but when trying a Open XL build on Jenkins for z/OS, I was seeing the compiler id come out as "IBMClang" ...
In any case, I did create my own openxl.cmake
toolconfig in the PR above.
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.
@Deigue Can this openxl.cmake be used for AIX as well?
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 would say yes, because there is a section in the file I left behind that says
if(OMR_OS_AIX)
, that leaves the default state of setup/configs, I suspect from xlc.cmake
.
So you would be able to use that section, but you may need to adapt the AIX configs to be whatever you need to make things work.
# OpenXL17 uses CMAKE_C_COMPILER_ID "Clang" | ||
set(_OMR_TOOLCONFIG "gnu") | ||
set(CMAKE_C_COMPILER_IS_OPENXL TRUE CACHE BOOL "OpenXL is the C compiler") |
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.
Lines should be indented with tabs (like the rest of this file).
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.
resolved
@@ -499,7 +499,7 @@ int32_t TR_PPCTableOfConstants::lookUp(TR::SymbolReference *symRef, TR::CodeGene | |||
int8_t local_buffer[1024]; | |||
int8_t *name = local_buffer; | |||
bool isAddr = false; | |||
intptr_t myTag; | |||
intptr_t myTag=0; |
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 add a space both sides of =
(like the lines above).
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.
resolved
This macro will specify if openXL17 clang is used. Signed-off-by: Ishita Ray <[email protected]>
Signed-off-by: Ishita Ray <[email protected]>
b133569
to
4fee25b
Compare
@@ -499,7 +499,7 @@ int32_t TR_PPCTableOfConstants::lookUp(TR::SymbolReference *symRef, TR::CodeGene | |||
int8_t local_buffer[1024]; | |||
int8_t *name = local_buffer; | |||
bool isAddr = false; | |||
intptr_t myTag; | |||
intptr_t myTag = 0; |
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 change seems unrelated to the title of this pull request; perhaps it should be proposed separately?
if(CMAKE_C_COMPILER_IS_OPENXL) | ||
set(OMR_ENV_OPENXL 1) | ||
set(ENV{OMR_ENV_OPENXL} ${OMR_ENV_OPENXL}) | ||
else() | ||
set(OMR_ENV_OPENXL 0) | ||
set(ENV{OMR_ENV_OPENXL} ${OMR_ENV_OPENXL}) | ||
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.
Why would we want this? Won't CMAKE_C_COMPILER_IS_OPENXL
be sufficient?
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.
That environment variable is for Makefiles to check if the compiler is openxl. I used that env variable instead of adding openxl in defaults.yml
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.
Which makefiles? With cmake, we don't write makefiles (cmake might even use another generator, e.g. ninja
).
I don't see any references to OMR_ENV_OPENXL
, neither in cmakefiles nor the environment; perhaps it would help if you provided a sample of how you imagine they would be used.
Added macro CMAKE_C_COMPILER_IS_OPENXL that will be used to add compiler options for openXL