This is an archive of the discontinued LLVM Phabricator instance.

[mlir] NFC - Split out RunnerUtils that don't require a C++ runtime
ClosedPublic

Authored by nicolasvasilache on Feb 27 2020, 7:35 AM.

Details

Summary

This revision split out a new CRunnerUtils library that supports
MLIR execution on targets without a C++ runtime.

Diff Detail

Event Timeline

ftynse added inline comments.Feb 27 2020, 7:47 AM
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
41

I'm not sure templates align with the comment above saying "entities must be kept C compatible".

rriddle added inline comments.Feb 27 2020, 8:41 AM
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
2

Please fix this.

Addressing review.

nicolasvasilache marked 3 inline comments as done.Feb 27 2020, 9:44 AM
nicolasvasilache added inline comments.
mlir/include/mlir/ExecutionEngine/CRunnerUtils.h
41

True, thanks, rephrased.

nicolasvasilache marked an inline comment as done.

Update comment.

Harbormaster completed remote builds in B47458: Diff 246998.
ftynse accepted this revision.Feb 27 2020, 10:42 AM
This revision is now accepted and ready to land.Feb 27 2020, 10:42 AM
This revision was automatically updated to reflect the committed changes.

This change broke the MLIR build on Windows.

As far as I can tell, the intention here is to produce two separate shared libraries: mlir_c_runner_utils and mlir_runner_utils with the second one (mlir_runner_utils) including the first one (mlir_c_runner_utils). Since:

add_llvm_library(mlir_c_runner_utils SHARED CRunnerUtils.cpp)

produces *only* a dll on windows, the linking of mlir_runner_utils fails because target_link_libraries is looking for a .lib file as opposed to a .dll file. I think this may be a case where either we need to use LINK_LIBS or explicitly build a static lib as well, but I haven't tried either yet.

@stella.stamenova @NathanielMcVicar bcee8982a2931960ca29a9e95e3d10355e7393ab adds an extra static library for linking for Windows.
As I do not have a Windows configuration handy, would you please confirm it fixes the issue you see?
Thanks!