[PATCH] D117752: [MLGO] Improved support for AOT cross-targeting scenarios

[Petr Hosek via Phabricator via llvm-commits]

phosek accepted this revision.
phosek added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/CMakeLists.txt:896-897
+    set(LLVM_INLINER_MODEL_PATH "none")
+  endif()
   if (NOT DEFINED LLVM_INLINER_MODEL_PATH
       OR "${LLVM_INLINER_MODEL_PATH}" STREQUAL ""
----------------
I think this could be `elif`?


================
Comment at: llvm/CMakeLists.txt:905-906
+    set(LLVM_RAEVICT_MODEL_PATH "none")
+  endif()
   if (NOT DEFINED LLVM_RAEVICT_MODEL_PATH
       OR "${LLVM_RAEVICT_MODEL_PATH}" STREQUAL ""
----------------
The same here?


================
Comment at: llvm/cmake/modules/TensorFlowCompile.cmake:47
 # called ${cpp_class} - which may be a namespace-qualified class name.
-function(tfcompile model tag_set signature_def_key fname cpp_class)
-  set(prefix ${CMAKE_CURRENT_BINARY_DIR}/${fname})
-  set(obj_file ${prefix}.o)
-  set(hdr_file ${prefix}.h)
-  string(TOUPPER ${fname} fname_allcaps)
-  set(override_header ${LLVM_OVERRIDE_MODEL_HEADER_${fname_allcaps}})
-  set(override_object ${LLVM_OVERRIDE_MODEL_OBJECT_${fname_allcaps}})
-  if (EXISTS "${override_header}" AND EXISTS "${override_object}")
-    configure_file(${override_header} ${hdr_file} COPYONLY)
-    configure_file(${override_object} ${obj_file} COPYONLY)
-    message("Using provided header "
-      ${hdr_file} " and object "   ${obj_file}
-      " files for model " ${model})
-  else()
-    tf_get_absolute_path(${model} ${CMAKE_CURRENT_BINARY_DIR} LLVM_ML_MODELS_ABSOLUTE)
-    message("Using model at " ${LLVM_ML_MODELS_ABSOLUTE})
-    add_custom_command(OUTPUT ${obj_file} ${hdr_file}
-      COMMAND ${TENSORFLOW_AOT_COMPILER} aot_compile_cpu
-            --multithreading false
-            --dir ${LLVM_ML_MODELS_ABSOLUTE}
-            --tag_set ${tag_set}
-            --signature_def_key ${signature_def_key}
-            --output_prefix ${prefix}
-            --cpp_class ${cpp_class}
-            --target_triple ${LLVM_HOST_TRIPLE}
-    )
-  endif()
+function(tfcompile model tag_set signature_def_key fname cpp_class hdr_file obj_file)
+  tf_get_absolute_path(${model} ${CMAKE_CURRENT_BINARY_DIR} LLVM_ML_MODELS_ABSOLUTE)
----------------
This is unrelated to this change, but since you're already modifying this function, I'd also consider renaming it to `tf_compile` to match other `tf_*` functions.


================
Comment at: llvm/cmake/modules/TensorFlowCompile.cmake:85
+    configure_file(${override_object} ${obj_file} COPYONLY)
+    message("Using provided header " ${hdr_file} " and object " ${obj_file} "
+      files for model " ${fname})  
----------------
I'd probably use `STATUS` level for this.


================
Comment at: llvm/cmake/modules/TensorFlowCompile.cmake:90
+  elseif("${model}" STREQUAL "none")
+    message("Will skip enabling mlgo for ${fname}")
+    return()
----------------
I'd probably use `STATUS` level for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117752