Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an option to build with -pthread #3671

Merged
merged 1 commit into from
Aug 3, 2019
Merged

Conversation

abergeron
Copy link
Contributor

I've noticed that the libraries (libtvm.so and friends) are currently linked with pthread, but are not built with support for threading.

I've made this patch to build the libraries with proper thread support. It is configurable if it needs to be disabled. It should also work properly on all platforms.

@abergeron
Copy link
Contributor Author

The test that failed was for batch norm layer. I think it has nothing to do with my changes, but I'm not 100% sure.

@tqchen tqchen merged commit 3e4ed6d into apache:master Aug 3, 2019
@tqchen
Copy link
Member

tqchen commented Aug 3, 2019

Thanks @abergeron . In the next time when flaky testcase failed, please retrigger the ci by another commit and send a flaky test issue

@kparzysz-quic
Copy link
Contributor

What is the intended meaning (and scope) of this flag? The name of the flag suggests that if USE_THREADS if FALSE, then the TVM binaries will not use threads at all. On Hexagon we want to be able to use threads even if cmake's standard scripts cannot detect the proper options, but this flag makes it difficult.

@abergeron
Copy link
Contributor Author

@kparzysz-quic This is mostly for binary compatibility with other C++ libraries. Building with or without -pthreads affect some of the private C++ classes that end up embedded inside the library and may conflict through the loader.

I have encountered this problem more than once, while loading tvm and pytorch for example. The python process would crash or hang depending on the load order and gdb indicates incompatible mixed symbols as the most probable cause.

@kparzysz-quic
Copy link
Contributor

@abergeron I'm not disputing that adding -pthread may be necessary in certain cases. What I'm asking is what this option is meant to do. If it's to add -pthread to compile/link options, it should be named accordingly, and there should be a way to opt out of it without disabling the use of (p)threads altogether.

wweic pushed a commit to wweic/tvm that referenced this pull request Aug 9, 2019
@abergeron
Copy link
Contributor Author

Since I'm not touching much, it should still link with pthread as before if you set USE_PTHREADS=OFF.

I'm not sure I would recommend doing that though.

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.

3 participants