Skip to content

Conversation

henrytwo
Copy link
Member

Addresses #1271, which will allow LTC to work from the prebuild wheels without import issues.

Previously, we were missing some cmake macros to include $ORIGIN to the list of directories searched for shared libraries.

As part of this, the reference LTC backend + base LTC backend were moved to _mlir_libs to be consistent with our other libraries (and also because the both of the libs need to be in the same directory for linking).

cc: @ke1337 @antoniojkim

@henrytwo henrytwo requested a review from silvasean August 25, 2022 19:26
@henrytwo henrytwo self-assigned this Aug 25, 2022
@henrytwo
Copy link
Member Author

@silvasean In the future should we add some test that uses the built wheel to ensure we don't hit a similar issue in the future?

It looks like this issue stemmed from the fact that when the lib was used, it wasn't in the same location where it was originally built, which is it why it failed to find the shared library. Piping the binary thru strings, I could see some hardcoded paths.

With this PR, $ORIGIN was added to the list of paths searched for shared libs, which should solve the problem.

Copy link
Contributor

@silvasean silvasean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

@silvasean
Copy link
Contributor

@silvasean In the future should we add some test that uses the built wheel to ensure we don't hit a similar issue in the future?

I think we attempt to do that here:

- name: Build Python wheels and smoke test.

Maybe we can extend the "smoke test" that is done there to do a basic LTC test?

@henrytwo henrytwo merged commit e869e68 into main Aug 25, 2022
@henrytwo henrytwo deleted the henrytu/ld_ltc_wheel branch August 25, 2022 22:25
qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
qedawkins pushed a commit to nod-ai/torch-mlir that referenced this pull request Oct 3, 2022
The modification to use the builder based interface to generate Krnl loop is completed (llvm#1250, llvm#1283, llvm#1285, llvm#1292, llvm#1293, llvm#1303, llvm#1308, llvm#1314, llvm#1316, llvm#1317, llvm#1326, llvm#1403), and BuildKrnlLoop is no longer needed.

Signed-off-by: Whitney Tsang [email protected]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants