[flang-commits] [PATCH] D154373: [llvm-test-suite][Fortran/gfortran] Refactor build system

[Tarun Prabhu via Phabricator via flang-commits]

tarunprabhu marked 2 inline comments as done.
tarunprabhu added inline comments.


================
Comment at: Fortran/gfortran/CMakeLists.txt:129
+  -mfpmath=387
+  -mfpmath=sse
   # -Os might eventually be supported, so this might also need to be removed
----------------
luporl wrote:
> I get an error on AArch64 when `-mtune=amdfam10` is used. It seems to be an X86-specific flag.
I intend to work on enabling the target directives after I get the remaining "compile" tests operational. Handling those directives properly should reduce the likelihood of these sorts of problems. 


================
Comment at: Fortran/gfortran/CMakeLists.txt:240
+# called from each subdirectory containing tests. It is expected that the
+# subdirectory will contain a fail named "DisabledFiles.cmake" which will
+# provide the list of files that are disabled in that subdirectory for various
----------------
luporl wrote:
> nit: s/fail/file/
I would argue that this is a bug, not merely a nit :-)


================
Comment at: Fortran/gfortran/CMakeLists.txt:536-546
+      elseif (line MATCHES "dg-additional-sources[ ]*[\"]?(.+)[\"]?[ ]*[}]")
+        separate_arguments(others UNIX_COMMAND ${CMAKE_MATCH_1})
+        list(TRANSFORM others STRIP)
+      elseif (line MATCHES "dg-(additional-)?options [{]?[ ]*\"([^\"]*)\"[ ]*[}]?(.*)")
+        # TODO: We completely ignore any target-specific options that may be
+        # present. These are usually in the form of target directives. For now,
+        # it seems to be ok, but as these tests are enabled on more platforms,
----------------
luporl wrote:
> It seems there is some code for parsing DejaGNU's directives that is common between compile and execute tests.
> As it's currently only a few lines, it doesn't seem worth to move it to a new function now, but it's something to consider in the future, if more common directives appear.
Yes, I agree, 

The target directives are currently ignored and need to be handled (I intend to do so once I enable the compile-tests - if not before if it turns out to cause major problems). That would be a good time to address this.


Repository:
  rT test-suite

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

https://reviews.llvm.org/D154373