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

Add compiler options needed by openXL #7447

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ishitaR88
Copy link

No description provided.

Copy link

@github-actions github-actions bot left a 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.

@zl-wang
Copy link
Contributor

zl-wang commented Aug 26, 2024

please provide a proper description of what you are trying to do in this PR

@zl-wang
Copy link
Contributor

zl-wang commented Sep 11, 2024

as for compiler-id, could you make it consistent with Z side (they are switching to OpenXL too, on Z of course)? also, Clang 17.x.x is ambiguous: that 17 refers to OpenXL17, or refers to community Clang version 17?

Detecting OpenXL as XLClang ... this is ambiguous too: we are currently using both xlC and xlclang (i.e. xlC16's xlclang front-end) already. I am not sure if it eventually worked out with this ambiguity, i prefer to staying away from the ambiguity. At least, provide abundant comments for future readers to understand it.

@ishitaR88
Copy link
Author

Commit for openXL as Xclang is redundant it's an old change that we don't need anymore so I have removed it.
For the Openxl clang version it is referring to OpenXL17. I have changed the wordings of the comment.

@ishitaR88 ishitaR88 changed the title Initialize variable in PPCTableOfConstants WIP: Add compiler options needed by openXL Oct 3, 2024
@ishitaR88 ishitaR88 changed the title WIP: Add compiler options needed by openXL Add compiler options needed by openXL Oct 18, 2024
@@ -1515,7 +1515,7 @@ bool TR_OSRCompilationData::TR_ScratchBufferInfo::operator==(const TR_ScratchBuf

#if (defined(TR_HOST_POWER) && defined(TR_TARGET_POWER) \
|| defined(TR_HOST_S390) && defined(TR_TARGET_S390)) \
&& defined(__IBMCPP__)
&& (defined(__IBMCPP__) || defined(__open_xl__) && defined(__cplusplus))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as raised in the other PR, is defined(__cplusplus) definitely needed here (and more places below)? isn't it automatically defined for c++ files to begin with?

@@ -144,7 +144,7 @@ typedef struct TR_InlinedCallSite
#include <builtins.h>
#endif /* __cplusplus */
#define FLUSH_MEMORY(smp) if( smp ) __lwsync();
#elif defined(LINUX)
#elif defined(LINUX) || defined(__open_xl__)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks out of place ... shouldn't it be defined in the same clause for AIX? strongly recommend to do that just for readability.

@@ -64,7 +64,7 @@
#define OMR_NORETURN _declspec(noreturn)
#elif defined(__GNUC__)
#define OMR_NORETURN __attribute__ ((noreturn))
#elif defined(__IBMCPP__)
#elif defined(__IBMCPP__) || defined(__open_xl__) && defined(__cplusplus)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@@ -22,7 +22,7 @@
#include "runtime/OMRRuntimeAssumptions.hpp"
#include "env/jittypes.h"

#if defined(__IBMCPP__) && !defined(AIXPPC) && !defined(LINUXPPC)
#if (defined(__IBMCPP__) || defined(__open_xl__) && defined(__cplusplus)) && !defined(AIXPPC) && !defined(LINUXPPC)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto re c++

@@ -23,7 +23,7 @@

TR_RuntimeHelperTable runtimeHelpers;

#if (defined(__IBMCPP__) || defined(__IBMC__) && !defined(MVS)) && !defined(LINUXPPC64)
#if (defined(__IBMCPP__) || defined(__IBMC__) && !defined(MVS)) && !defined(LINUXPPC64) || defined(__open_xl__)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the other comment re MVS applies here too. && and || are used in an unreadable way ... did you really take their precedences into accounts?

CC="$ac_save_CC $ac_arg"
if ac_fn_c_try_compile "$LINENO"; then :
ac_cv_prog_cc_c89=$ac_arg
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indent properly to align with if

#elif defined(__xlC__) /* defined(__GNUC__) */
#if ((__xlC__ > 0x0d01) || ((__xlC__ == 0x0d01) && (__xlC_ver__ >= 0x00000300))) /* XLC >= 13.1.3 */
#elif (defined(__xlC__)) /* defined(__GNUC__) */
#if ((__xlC__ > 0x0d01) || ((__xlC__ == 0x0d01) && (__xlC_ver__ >= 0x00000300))) /* XLC >= 13.1.3 OR OpenXL */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a better way to do the same thing? instead of these cryptic numbers

@@ -237,7 +237,7 @@ typedef double SYS_FLOAT;
* xlC11 C++ compiler reportedly supports attributes before function names, but we've only tested xlC12.
* The C compiler doesn't support it.
*/
#if defined(__cplusplus) && (__xlC__ >= 0xc00)
#if defined(__cplusplus) && (__xlC__ >= 0xc00)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this relevant to and applies to openxl case?

Trc_PRT_signal_omrsignal_sig_protect_Exit_long_jumped_back_to_omrsig_protect(fn, fn_arg, handler, handler_arg, flags);
return OMRPORT_SIG_EXCEPTION_OCCURRED;
}
#if !defined(__open_xl__)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this block of code isn't needed for OpenXL build anymore? does it still work as xlC build?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants