[PATCH] D69189: [Builtins] Fix bug where powerpc builtins specializations didn't remove generic implementations.

[Dan Liew via Phabricator via llvm-commits]

delcypher created this revision.
delcypher added reviewers: phosek, beanz, compnerd, shiva0217, amyk, rupprecht, kongyi, mstorsjo, t.p.northover, weimingz, jroelofs, joerg, sidneym.
Herald added subscribers: Sanitizers, steven.zhang, shchenz, jsji, kristof.beyls, mgorny, nemanjai.
Herald added projects: LLVM, Sanitizers.

Previously the CMake code looked for filepaths of the form
`<arch>/<filename>` as an indication that `<arch>/<filename>` provided a
specialization of a top-level file `<filename>`. For powerpc there was a
bug because the powerpc specialized implementations lived in `ppc/` but
the architectures were `powerpc64` and `powerpc64le` which meant that
CMake was looking for files at `powerpc64/<filename>` and
`powerpc64le/<filename>`.

Although we could just add similar code to what there is for arm (i.e.
compute `${_arch}`) this extremely error prone (until r375150 no error
was raised). Instead this patch takes a different approach that removes
looking for the architecture name entirely. Instead this patch uses the
convention that a source file in a sub-directory might be a
specialization of a generic implementation and if a source file of the
same name (ignoring extension) exists at the top-level then it is the
corresponding generic implementation. This approach is much simpler
because it doesn't require keeping track of different architecture
names.

This convention already existed in repository but previously it was
implicit.  This change makes it explicit.

This patch is motivated by wanting to revert r375162 which worked around
the powerpc bug found when r375150 landed.

Once it lands we should revert r375162.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D69189

Files:
  lib/builtins/CMakeLists.txt


Index: lib/builtins/CMakeLists.txt
===================================================================
--- lib/builtins/CMakeLists.txt
+++ lib/builtins/CMakeLists.txt
@@ -586,15 +586,6 @@
 
   foreach (arch ${BUILTIN_SUPPORTED_ARCH})
     if (CAN_TARGET_${arch})
-      # NOTE: some architectures (e.g. i386) have multiple names.  Ensure that
-      # we catch them all.
-      set(_arch ${arch})
-      if("${arch}" STREQUAL "armv6m")
-        set(_arch "arm|armv6m")
-      elseif("${arch}" MATCHES "^(armhf|armv7|armv7s|armv7k|armv7m|armv7em)$")
-        set(_arch "arm")
-      endif()
-
       # For ARM archs, exclude any VFP builtins if VFP is not supported
       if (${arch} MATCHES "^(arm|armhf|armv7|armv7s|armv7k|armv7m|armv7em)$")
         string(REPLACE ";" " " _TARGET_${arch}_CFLAGS "${TARGET_${arch}_CFLAGS}")
@@ -608,10 +599,18 @@
       # architecture specific manner.  This prevents multiple definitions of the
       # same symbols, making the symbol selection non-deterministic.
       foreach (_file ${${arch}_SOURCES})
-        if (${_file} MATCHES ${_arch}/*)
+        get_filename_component(_file_dir "${_file}" DIRECTORY)
+        if (NOT "${_file_dir}" STREQUAL "")
+          # Architecture specific file. We follow the convention that a source
+          # file that exists in a sub-directory (e.g. `ppc/divtc3.c`) is
+          # architecture specific and that if a generic implementation exists
+          # it will be a top-level source file (e.g. `divtc3.c`).
           get_filename_component(_name ${_file} NAME)
           string(REPLACE ".S" ".c" _cname "${_name}")
-          list(REMOVE_ITEM ${arch}_SOURCES ${_cname})
+          if (EXISTS "${CMAKE_CURRENT_SOURCE_DIR}/${_cname}")
+            message(STATUS "For ${arch} builtins preferring ${_file} to ${_cname}")
+            list(REMOVE_ITEM ${arch}_SOURCES ${_cname})
+          endif()
         endif ()
       endforeach ()
 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D69189.225675.patch
Type: text/x-patch
Size: 1924 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191018/caa8be03/attachment.bin>