Skip to content

Conversation

@csullivan
Copy link
Contributor

@csullivan csullivan commented Jun 8, 2022

Only include Hexagon runtime sources when building for hexagon rpc on hardware and do not include them
for x86 (host, simulator) or android builds.

Duplicate of #11595.

cc @mehrdadh

csullivan and others added 2 commits June 8, 2022 16:01
are included in the runtime build. Specifically only include them
when building for hexagon rpc on hardware and do not include them
for x86 (host, simulator) or android builds.
@csullivan csullivan marked this pull request as ready for review June 8, 2022 23:02
endfunction()

if(BUILD_FOR_HEXAGON OR USE_HEXAGON_RPC)
if(BUILD_FOR_HEXAGON OR ("${USE_HEXAGON_RPC}" STREQUAL "HEXAGON"))
Copy link
Contributor

Choose a reason for hiding this comment

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

When would BUILD_FOR_HEXAGON be false, but USE_HEXAGON_RPC be "HEXAGON"? It seems like the latter implies the former...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BUILD_FOR_HEXAGON is false for the runtime android and x86 build targeted by the hexagon_api, and USE_HEXAGON_RPC is true for these cases. Thus I want to tighten the requirement so that hexagon sources are not brought in to the tvm_runtime builds (initiated by the hexagon_api cmake) when we aren't building for hexagon.

Copy link
Contributor

@kparzysz-quic kparzysz-quic Jun 9, 2022

Choose a reason for hiding this comment

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

What I mean is that it seems that (${USE_HEXAGON_RPC} == "HEXAGON") => BUILD_FOR_HEXAGON. Unless I'm missing something, you can just remove USE_HEXAGON_RPC from x86/android builds (and also from this check).

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested with if(BUILD_FOR_HEXAGON) and it appears to work at least for PR #11653 which depends on this PR to pass CI.

Copy link
Contributor

@adstraw adstraw Jun 10, 2022

Choose a reason for hiding this comment

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

We may be able to "remove USE_HEXAGON_RPC from x86/android builds" in hexagon_api/CMakeLists.txt as @kparzysz-quic suggests but I am getting some strange results when doing so. I reverted the changes to that file in this PR.

Changing to if(BUILD_FOR_HEXAGON) and removing the API check works and accomplishes the original intent of the PR.

Copy link
Contributor

@adstraw adstraw left a comment

Choose a reason for hiding this comment

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

LGTM; thanks @csullivan

@github-actions github-actions bot requested a review from mehrdadh June 10, 2022 23:06
Copy link
Contributor

@kparzysz-quic kparzysz-quic left a comment

Choose a reason for hiding this comment

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

Let's unblock dependent PRs. We can refine this later if necessary.

@kparzysz-quic kparzysz-quic merged commit 2df4524 into apache:main Jun 13, 2022
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