Adding libs for OpenXL #20045

ishitaR88 · 2024-08-22T18:56:01Z

No description provided.

zl-wang · 2024-08-26T13:39:50Z

Two high-level comments:

provide a proper description of what you are trying to do in this PR;
you cannot modify the code as you did, since it breaks the builds using xlC compiler. keep in mind that the codebase feeds into java8/11/17/21/newer builds. we only intended to switch to OpenXL for 'newer' builds like java23. if you change the code as you did in this PR, java8-21 cannot be built anymore (with xlC). you have to change the code (and makefile etc) conditionally to be compatible with both compilers.

zl-wang · 2024-09-11T11:24:22Z

please indicate if it cleared the criteria that, with these changes in place, we can build with both xlC/xlclang and OpenXL (also staying or better the performance). that is when it is ready to be reviewed.

zl-wang · 2024-09-19T18:42:22Z

did I miss your changes to makefiles, to set CC and CXX? at least, i haven't seen them yet.

pshipton · 2024-09-20T21:03:16Z

I tried a build with the OMR (eclipse/omr#7447) and these OpenJ9 changes.
https://hyc-runtimes-jenkins.swg-devops.com/job/Build_JDK23_ppc64_aix_Personal/37
I added the compiler path (only works on paix822) and defined CC and CXX https://github.ibm.com/Peter-Shipton/tooling/tree/openxl
I had to fix (hack) openssl.gmk as well as revert the change to use xlc https://github.com/pshipton/openj9-openjdk-jdk23/tree/openxl

It got this far:

16:49:15  [ 41%] Running preprocessing p/runtime/ebb.spp to create /home/jenkins/workspace/Build_JDK23_ppc64_aix_Personal/build/aix-ppc64-server-release/vm/runtime/compiler/p/runtime/ebb.ipp
16:49:15  error: invalid value 'extc99' in '-std=extc99'

pshipton · 2024-09-20T21:06:21Z

did I miss your changes to makefiles, to set CC and CXX?

defaults.yml should be updated similarly to https://github.ibm.com/Peter-Shipton/tooling/commit/eebd2986b8a3b

It won't work on any OpenJ9 open machine until we update them to have the 17.1.2 compiler.

ishitaR88 · 2024-09-23T13:16:38Z

I tried a build with the OMR (eclipse/omr#7447) and these OpenJ9 changes. https://hyc-runtimes-jenkins.swg-devops.com/job/Build_JDK23_ppc64_aix_Personal/37 I added the compiler path (only works on paix822) and defined CC and CXX https://github.ibm.com/Peter-Shipton/tooling/tree/openxl I had to fix (hack) openssl.gmk as well as revert the change to use xlc https://github.com/pshipton/openj9-openjdk-jdk23/tree/openxl

It got this far:
16:49:15  [ 41%] Running preprocessing p/runtime/ebb.spp to create /home/jenkins/workspace/Build_JDK23_ppc64_aix_Personal/build/aix-ppc64-server-release/vm/runtime/compiler/p/runtime/ebb.ipp
16:49:15  error: invalid value 'extc99' in '-std=extc99'

Yes there was an issue. I have fixed that.will push the modification shortly.

pshipton · 2024-09-23T15:35:53Z

runtime/tests/redirector/jep178/module.xml

@@ -34,7 +34,7 @@ SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0-only WITH Classpath-ex
 			<makefilestub data="CFLAGS += -Wc,DLL,EXPORTALL">
 				<include-if condition="spec.zos_390.*"/>
 			</makefilestub>
-			<makefilestub data="UMA_CC_MODE += -qpic -brtl -bexpall">
+			<makefilestub data="UMA_CC_MODE += -fpic -brtl -bexpall">


This can't be changed unconditionally.
FYI module.xml (UMA) is mostly used for IBM Java 8 builds, but it's still (was) possible use UMA to build OpenJ9 versions, It's something we should check, if UMA is still working for Java 23 with OpenXL. By default we use cmake, but UMA can be used if adding options for debugging purposes.

keithc-ca · 2024-10-02T21:49:07Z

runtime/cmake/platform/os/aix.cmake

+    set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,-G,-brtl,-bernotok,-bnoentry,-bnolibpath -latomic -lperfstat -liconv")
+    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,-brtl -latomic -lperfstat -liconv")
+else()
+    set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,-G,-brtl,-bernotok,-bnoentry,-bnolibpath")
+    set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} -Wl,-brtl")


Please indent with tabs like other cmake files.

keithc-ca · 2024-10-02T21:50:31Z

runtime/cmake/platform/toolcfg/gnu.cmake

@@ -35,7 +35,7 @@ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-exceptions -fno-rtti")

 # Raise an error if a shared library has any unresolved symbols.
 # This flag isn't supported on OSX, but it has this behaviour by default


Please expand the comment to match the change (including proper punctuation).

keithc-ca · 2024-10-02T21:53:25Z

runtime/compiler/CMakeLists.txt

+		if(OMR_OS_AIX AND CMAKE_C_COMPILER_IS_OPENXL)
+			list(APPEND J9_SHAREDFLAGS -m64)
+		else()
+			list(APPEND J9_SHAREDFLAGS -q64)
+		endif()


The AIX part of this test is redundant (see line 278).

keithc-ca · 2024-10-02T21:56:40Z

runtime/compiler/env/J9KnownObjectTable.cpp

+            #if defined(__open_xl__)
+               TR_VMFieldsInfo **fieldsInfoByIndex = (TR_VMFieldsInfo**)__builtin_alloca(endIndex * sizeof(TR_VMFieldsInfo*));
+            #else
+               TR_VMFieldsInfo **fieldsInfoByIndex = (TR_VMFieldsInfo**)alloca(endIndex * sizeof(TR_VMFieldsInfo*));
+            #endif


Please don't indent preprocessor directives, but do add comments on #else and #endif:

#if defined(__open_xl__) TR_VMFieldsInfo **fieldsInfoByIndex = (TR_VMFieldsInfo**)__builtin_alloca(endIndex * sizeof(TR_VMFieldsInfo*)); #else /* defined(__open_xl__) */ TR_VMFieldsInfo **fieldsInfoByIndex = (TR_VMFieldsInfo**)alloca(endIndex * sizeof(TR_VMFieldsInfo*)); #endif /* defined(__open_xl__) */

This pattern appears often enough that we might instead want to put this in a common header file:

#if defined(__open_xl__) #define alloca(size) __builtin_alloca(size) #endif /* defined(__open_xl__) */

keithc-ca · 2024-10-02T21:58:35Z

runtime/compiler/runtime/RelocationRecord.cpp

@@ -64,7 +64,7 @@
 #endif /* defined(J9VM_OPT_JITSERVER) */

 // TODO: move this someplace common for RuntimeAssumptions.cpp and here
-#if defined(__IBMCPP__) && !defined(AIXPPC) && !defined(LINUXPPC)
+#if (defined(__IBMCPP__) || defined(__open_xl__) && defined(__cplusplus)) && !defined(AIXPPC) && !defined(LINUXPPC)


The subexpression defined(__cplusplus) seems redundant in this C++ source file.

keithc-ca · 2024-10-02T22:06:13Z

runtime/makelib/targets.mk.aix.inc.ftl

+ifneq ($(OMR_ENV_OPENXL),1)
+  CFLAGS += -qlanglvl=extended -qarch=ppc -qalias=noansi -qxflag=LTOL:LTOL0 -qsuppress=1506-1108 -qstackprotect
+else
+  #openxl options
+  CFLAGS += -std=c89 -qarch=ppc -fno-strict-aliasing -fstack-protector
+endif


Please invert this to avoid the negation:

ifeq ($(OMR_ENV_OPENXL),1) # openxl options CFLAGS += -std=c89 -qarch=ppc -fno-strict-aliasing -fstack-protector else CFLAGS += -qlanglvl=extended -qarch=ppc -qalias=noansi -qxflag=LTOL:LTOL0 -qsuppress=1506-1108 -qstackprotect endif

Same comment near lines 128 & 139.

keithc-ca · 2024-10-02T22:08:39Z

runtime/makelib/targets.mk.aix.inc.ftl

@@ -178,7 +203,9 @@ $(patsubst %.s,%.o,$(filter %.s,$(UMA_FILES_TO_PREPROCESS))) : %$(UMA_DOT_O) : %

 ifdef UMA_TREAT_WARNINGS_AS_ERRORS
  ifndef UMA_SUPPRESS_WARNINGS_AS_ERRORS
-    CFLAGS += -qhalt=w
-    CXXFLAGS += -qhalt=w
+    ifneq ($(OMR_ENV_OPENXL),1)


Please invert and add equivalent options for OpenXL.

keithc-ca · 2024-10-02T22:11:45Z

runtime/oti/j9nonbuilder.h

@@ -1579,9 +1579,10 @@ typedef struct J9ExceptionHandler {
 	U_32 exceptionClassIndex;
 } J9ExceptionHandler;

-#if defined(__xlC__) || defined(J9ZOS390)  /* Covers: Z/OS, AIX, Linux PPC*/
+//JACKIE: not sure where defined(__open_xl__) condition should go


C++ style comments cannot be used in a C header file (not that this one should stay anyway).

keithc-ca · 2024-10-02T22:12:27Z

runtime/oti/j9nonbuilder.h

 #pragma pack(1)
-#elif defined(__ibmxl__) || defined(__GNUC__) || defined(_MSC_VER) /* Covers: Linux PPC LE, Windows, Linux x86 */
+#elif defined(__ibmxl__) || defined(__open_xl__) || defined(__GNUC__) || defined(_MSC_VER) /* Covers: Linux PPC LE, Windows, Linux x86 */


I think defined(__open_xl__) belongs on line 1583 which make this one dead.

keithc-ca · 2024-10-02T22:13:52Z

runtime/oti/j9nonbuilder.h

@@ -1579,9 +1579,10 @@ typedef struct J9ExceptionHandler {
 	U_32 exceptionClassIndex;
 } J9ExceptionHandler;

-#if defined(__xlC__) || defined(J9ZOS390)  /* Covers: Z/OS, AIX, Linux PPC*/
+//JACKIE: not sure where defined(__open_xl__) condition should go
+#if defined(__xlC__) || defined(J9ZOS390) || defined(__open_xl__)  /* Covers: Z/OS, AIX, Linux PPC*/


Please finish each sentence with a period, spell z/OS correctly and sort platforms:

#if defined(__xlC__) || defined(J9ZOS390) || defined(__open_xl__) /* Covers: AIX, Linux PPC and z/OS. */

pshipton · 2024-10-03T18:32:21Z

Tried a new build https://hyc-runtimes-jenkins.swg-devops.com/job/Build_JDK23_ppc64_aix_Personal/52
There are a number of errors like

14:27:22  /home/jenkins/workspace/Build_JDK23_ppc64_aix_Personal/openj9/runtime/compiler/optimizer/InterpreterEmulator.cpp:1594:105: error: too many arguments to function call, expected 0, have 1
14:27:22   1594 |                                                       (int32_t) _currentCallMethod->virtualCallSelector(cpIndex), cpIndex, _currentCallMethod,
14:27:22        |                                                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^~~~~~~

This commit adds necessary libraries nedded by linker commands for openxl compiler on AIX architechture. Signed-off-by: Ishita Ray <ishita.ray@ibm.com>

Signed-off-by: midronij <jackie.midroni@ibm.com>

Signed-off-by: Ishita Ray <ishita.ray@ibm.com>

Add CMAKE_C_COMPILER_IS_OPENXL macro to add the compiler flags needed by openXL17 Signed-off-by: Ishita Ray <ishita.ray@ibm.com>

ishitaR88 force-pushed the openxl branch from ba9f6f8 to 68fd820 Compare August 22, 2024 19:08

ishitaR88 requested a review from dsouzai as a code owner August 22, 2024 19:16

ishitaR88 force-pushed the openxl branch from 3fdd87b to a1ca6df Compare September 11, 2024 05:40

pshipton mentioned this pull request Sep 20, 2024

OpenSSL compilation on AIX doesn't "just work" with OpenXL 7.1.2 #20207

Closed

pshipton reviewed Sep 23, 2024

View reviewed changes

ishitaR88 force-pushed the openxl branch 3 times, most recently from e426cf2 to 121bd27 Compare September 26, 2024 20:17

keithc-ca requested changes Oct 2, 2024

View reviewed changes

ishitaR88 force-pushed the openxl branch from 1d1d4e2 to fd4fd50 Compare October 3, 2024 14:20

ishitaR88 changed the title ~~Adding libs for OpenXL~~ WIP:Adding libs for OpenXL Oct 3, 2024

keithc-ca marked this pull request as draft October 3, 2024 18:34

keithc-ca changed the title ~~WIP:Adding libs for OpenXL~~ Adding libs for OpenXL Oct 3, 2024

ishitaR88 and others added 6 commits October 3, 2024 14:57

Add libs for OpenXL compiler on AIX

9ac3c7e

This commit adds necessary libraries nedded by linker commands for openxl compiler on AIX architechture. Signed-off-by: Ishita Ray <ishita.ray@ibm.com>

Add necessary libraries to linker commands

fd75693

Signed-off-by: midronij <jackie.midroni@ibm.com>

Add __open_xl__ macro ifdefs

c6373bb

Signed-off-by: midronij <jackie.midroni@ibm.com>

Add __open_xl__ macros to __IBMC__/__IBMCPP__ conditions

3ecd5dc

Signed-off-by: midronij <jackie.midroni@ibm.com>

Add __open_xl__ macro for __builtin_alloca

f444c93

Signed-off-by: Ishita Ray <ishita.ray@ibm.com>

Add CMAKE_C_COMPILER_IS_OPENXL macro for related flags

c9a7ed2

Add CMAKE_C_COMPILER_IS_OPENXL macro to add the compiler flags needed by openXL17 Signed-off-by: Ishita Ray <ishita.ray@ibm.com>

ishitaR88 force-pushed the openxl branch from fd4fd50 to c9a7ed2 Compare October 3, 2024 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding libs for OpenXL #20045

Adding libs for OpenXL #20045

ishitaR88 commented Aug 22, 2024

zl-wang commented Aug 26, 2024

zl-wang commented Sep 11, 2024

zl-wang commented Sep 19, 2024

pshipton commented Sep 20, 2024 •

edited

Loading

pshipton commented Sep 20, 2024

ishitaR88 commented Sep 23, 2024

pshipton Sep 23, 2024

keithc-ca Oct 2, 2024

keithc-ca Oct 2, 2024

keithc-ca Oct 2, 2024

keithc-ca Oct 2, 2024

keithc-ca Oct 2, 2024

keithc-ca Oct 2, 2024

keithc-ca Oct 2, 2024

keithc-ca Oct 2, 2024

keithc-ca Oct 2, 2024

keithc-ca Oct 2, 2024

pshipton commented Oct 3, 2024

		@@ -35,7 +35,7 @@ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fno-exceptions -fno-rtti")

		# Raise an error if a shared library has any unresolved symbols.
		# This flag isn't supported on OSX, but it has this behaviour by default

Adding libs for OpenXL #20045

Are you sure you want to change the base?

Adding libs for OpenXL #20045

Conversation

ishitaR88 commented Aug 22, 2024

zl-wang commented Aug 26, 2024

zl-wang commented Sep 11, 2024

zl-wang commented Sep 19, 2024

pshipton commented Sep 20, 2024 • edited Loading

pshipton commented Sep 20, 2024

ishitaR88 commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pshipton commented Oct 3, 2024

pshipton commented Sep 20, 2024 •

edited

Loading