[PATCH] D72384: [mlir] mlir-cpu-runner test's cblas_interface should export functions on Windows

[Kern Handa via Phabricator via llvm-commits]

kernhanda marked an inline comment as done.
kernhanda added inline comments.


================
Comment at: mlir/test/mlir-cpu-runner/include/cblas_interface.h:1
+//===- cblas_interface.h - Simple Blas subset interface -------------------===//
+//
----------------
ftynse wrote:
> I'm not certain `cblas_interface` is the right name here, it doesn't seem to contain anything specific to BLAS. Maybe `linalg_interface` would have been a better name. May be a separate clean up commit after syncing with @nicolasvasilache 
Yes, I would prefer to make that as a separate change, after consensus has been reached on a new name.


================
Comment at: mlir/test/mlir-cpu-runner/include/cblas_interface.h:13-25
+#ifdef _WIN32
+#ifndef MLIR_CBLAS_INTERFACE_EXPORT
+#ifdef cblas_interface_EXPORTS
+/* We are building this library */
+#define MLIR_CBLAS_INTERFACE_EXPORT __declspec(dllexport)
+#else
+/* We are using this library */
----------------
ftynse wrote:
> This is a third occurrence of a similar macro definition. Would it be possible to generalize it somehow and have only one? 
AFAICT, no. Three separate shared modules are created, each with their own exports. Furthermore, the three headers are all used within `cblas_interface`, so the macro can't be shared, because defining it would be telling the Windows linker that all the functions should be exported, instead of just the ones that are defined in `cblas_interface`.

The only other way is to have a shared preprocessor symbol between the three modules, but set it to different numerical values, which would be the way the different usages are identified.

Thoughts? I would suggest leaving it as is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72384