[PATCH] D102155: [AIX][compiler-rt] Add cmake module to build libatomic.a

[Jinsong Ji via Phabricator via llvm-commits]

jsji added a comment.

Thanks @lkail ! In general, I think we can try to make the Modules platform dependent.



================
Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:3
+
+set(AIX_LIBATOMIC_EXPORTED_ROUTINES
+  __atomic_compare_exchange
----------------
Can we make *ALL* the code in this file platform independent?  Although there are NO other users in other platform, we can make it reusable in case there are similar requests from other platform.

So, please remove `AIX_` prefix here. 


================
Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:47
+
+function(add_aix_atomic_dynlib bitmode out_dir export_list)
+  cmake_parse_arguments(LIB
----------------
Same, remove `_aix_` here, also pass down compile_flag and link flag. 




================
Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:60
+                        SOVERSION 1
+                        POSITION_INDEPENDENT_CODE OFF
+                        COMPILE_FLAGS -m${bitmode} -I -qmakedep=gcc
----------------
Why this has to be `OFF`?  Isn't `SHARED` needs PIC? Also AIX defaults to PIC.


================
Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:61
+                        POSITION_INDEPENDENT_CODE OFF
+                        COMPILE_FLAGS -m${bitmode} -I -qmakedep=gcc
+                        LINK_FLAGS "-Wl,-b${bitmode} -Wl,-H512 -Wl,-D0 \
----------------
Can we pass the flag into this functions instead of hardcoding them here.


================
Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:62
+                        COMPILE_FLAGS -m${bitmode} -I -qmakedep=gcc
+                        LINK_FLAGS "-Wl,-b${bitmode} -Wl,-H512 -Wl,-D0 \
+                                    -Wl,-T512 -Wl,-bhalt:4 -Wl,-bernotok \
----------------
Same as above, these flags should be passed down from `CMakeLists.txt` , not hardcoding them here.


================
Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:74
+
+function(add_aix_libatomic)
+  cmake_parse_arguments(LIB
----------------
Remove `_aix_`, also add options to control building 32 bit only, 64 bit only, or both.


================
Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:80
+    ${ARGN})
+  # Write exported routines to export list file prior the build.
+  set(LIBATOMIC_EXPORT_LIST ${CMAKE_CURRENT_BINARY_DIR}/export_list.atomic)
----------------
Can we make this another function like `generate_export_list`?


================
Comment at: compiler-rt/cmake/Modules/CompilerRTAIXUtils.cmake:99
+  add_custom_command(OUTPUT ${AIX_LIBATOMIC_ARCHIVE}
+                     COMMAND ${CMAKE_AR} -r -X32_64
+                             -o ${AIX_LIBATOMIC_ARCHIVE}
----------------
ar flags should be passed down, or can have default, and can be overwritten by args.


================
Comment at: compiler-rt/lib/builtins/CMakeLists.txt:753
+  include(CompilerRTAIXUtils)
+  add_aix_libatomic(
+    PARENT_TARGET builtins
----------------
Set the compile flags, linker flags, ar flags here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102155/new/

https://reviews.llvm.org/D102155