Page MenuHomePhabricator

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

Authored by kernhanda on Jan 7 2020, 8:15 PM.



This change fixes the build on Windows, so that cblas_interface.dll exports functions correctly and an implib is created and installed correctly.

Currently, LLVM cannot be consumed on Windows after it has been installed in a location because cblas_interface.lib is not created/installed, thus failing the import check in LLVMExports.cmake.

Diff Detail

Event Timeline

kernhanda created this revision.Jan 7 2020, 8:15 PM

I don't have commit access, so someone else will have to submit this.

ftynse requested changes to this revision.Jan 8 2020, 5:10 AM
ftynse added inline comments.

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


This is a third occurrence of a similar macro definition. Would it be possible to generalize it somehow and have only one?


Please add a comment that indicates which #if this is closing

This revision now requires changes to proceed.Jan 8 2020, 5:10 AM
kernhanda updated this revision to Diff 236871.Jan 8 2020, 11:26 AM
kernhanda marked 2 inline comments as done.

Added comments on #endif statements to indicate block they're closing.

kernhanda marked an inline comment as done.Jan 8 2020, 11:26 AM
kernhanda added inline comments.

Yes, I would prefer to make that as a separate change, after consensus has been reached on a new name.


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.

ftynse accepted this revision.Jan 9 2020, 1:44 AM
ftynse marked an inline comment as done.
ftynse added inline comments.

Works for me. This seems to be the last "library" file for the runner, so the number of macros should not grow uncontrollably.

This revision is now accepted and ready to land.Jan 9 2020, 1:44 AM

Not sure if we're waiting for more approvals, but in case we're not, just a reminder that someone else will have to commit this on my behalf. :) Thanks!

This revision was automatically updated to reflect the committed changes.