Skip to content

Conversation

@mehrdadh
Copy link
Member

This PR changes RPC read buffer size to be adjustable from python size by registering init function for FastRPC.

@csullivan @kparzysz-quic

@mehrdadh mehrdadh requested a review from csullivan April 15, 2022 16:57
@mehrdadh mehrdadh force-pushed the hexagon/adjustable_buffer_size branch from 89b7fc1 to 68d0935 Compare April 15, 2022 17:02
Copy link
Contributor

@csullivan csullivan left a comment

Choose a reason for hiding this comment

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

🎉

return &g_hexagon_rpc_server;
static tvm::runtime::hexagon::HexagonRPCServer* g_hexagon_rpc_server;
tvm::runtime::hexagon::HexagonRPCServer* get_hexagon_rpc_server(
uint32_t rpc_receive_buff_size_bytes = TVM_HEXAGON_RPC_BUFF_DEFAULT_SIZE_BYTES) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about we set the default value to 0, delete the TVM_HEXAGON_RPC_BUFF_DEFAULT_SIZE_BYTES define, and then CHECK that rpc_receive_buff_size_bytes is non-zero when g_hexagon_rpc_server is null?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

.Default(none);
}

TVM_REGISTER_GLOBAL("tvm.contrib.hexagon.create_hexagon_session")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we include a test that we expect to fail when the stack size is too small, and expect to pass when the stack size is set larger?

Copy link
Contributor

Choose a reason for hiding this comment

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

The simulator ignores the stack size at the moment.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mehrdadh mehrdadh requested a review from csullivan April 18, 2022 15:51
Copy link
Contributor

@csullivan csullivan left a comment

Choose a reason for hiding this comment

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

LGTM!

@kparzysz-quic
Copy link
Contributor

kparzysz-quic commented Apr 18, 2022

Not a blocker, but is there a reason not to use std::stringbuf instead of the preallocated array? (the code for simulator does that)

@mehrdadh
Copy link
Member Author

@kparzysz-quic I don't see a reason. Would std::stringbuf be slower?

@kparzysz-quic
Copy link
Contributor

@kparzysz-quic I don't see a reason. Would std::stringbuf be slower?

Probably, but it wouldn't affect model performance, since the only time it would run would be during the actual RPC communication. If you want you can consider it in another PR.

@areusch areusch merged commit f617191 into apache:main Apr 19, 2022
@mehrdadh mehrdadh deleted the hexagon/adjustable_buffer_size branch April 19, 2022 00:33
Lucien0 pushed a commit to Lucien0/tvm that referenced this pull request Apr 19, 2022
altanh pushed a commit to altanh/tvm that referenced this pull request Apr 28, 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.

4 participants