[PATCH] D119507: [MLGO] Use TFLite in 'development mode'

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/CMakeLists.txt:897
 set(TENSORFLOW_C_LIB_PATH "" CACHE PATH "Path to TensorFlow C library install")
-if (TENSORFLOW_C_LIB_PATH)
+set(LLVM_HAVE_TFLITE "" CACHE BOOL "Use tflite")
+if (LLVM_HAVE_TFLITE)
----------------
ebrevdo wrote:
> Should this be LLVM_USE_TFLITE?
We used the "have" pattern before though. The idea is that if the dep is present we enable the code. I think "use" is used to say "enable this" (deps not an issue).


================
Comment at: llvm/CMakeLists.txt:904
+  include_directories(${LLVM_PROTOBUF_OUT_DIR})
+elseif (TENSORFLOW_C_LIB_PATH)
   find_library(tensorflow_c_api tensorflow PATHS ${TENSORFLOW_C_LIB_PATH}/lib NO_DEFAULT_PATH REQUIRED)
----------------
ebrevdo wrote:
> do you plan to remove this in future?
Yes. Keeping it for now until we sort out downstream (so a few days after this lands). After that, this and the old TFUtils.cpp get deleted.


================
Comment at: llvm/include/llvm/Config/llvm-config.h.cmake:102
 /* Define if LLVM was built with a dependency to the libtensorflow dynamic library */
 #cmakedefine LLVM_HAVE_TF_API
 
----------------
ebrevdo wrote:
> plan to remove this in future?
yes.


================
Comment at: llvm/lib/Analysis/TFLiteUtils.cpp:103
+    : Input(InputSpecs.size()), Output(OutputSpecsSize) {
+  tflite::StderrReporter ErrorReporter;
+  SmallVector<char, 128> TFLitePathBuff;
----------------
ebrevdo wrote:
> Suggest subclassing tflite::StatefulErrorReporter like [here](https://github.com/ochi96/mediapipe_simplified/blob/2b6070b69d907298bf3c87f486c74689765bc9a3/mediapipe/util/tflite/error_reporter.cc) so that you can check error status by looking at reporter.message() after the initialization and after running inference.  you can even add a bool like has_error which will be cheaper (no copy).
adding a FIXME to do that - want to get the core functionality up and going with the minimal change (and this is large-ish already).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119507