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.

Sign up for GitHub

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

Jump to bottom

[Hexagon] Profiling changes for abadams/per_instance_profiling #8187

Open
wants to merge 5 commits into
base: abadams/per_instance_profiling
Choose a base branch
from
Open

[Hexagon] Profiling changes for abadams/per_instance_profiling #8187

wants to merge 5 commits into from

Conversation

aankit-quic
Copy link
Contributor

I still see a crash on device when offloading, the culprit being IR like this:

...
let profiler_shared_sampling_token = (void *)alloca(4)
halide_profiler_init_sampling_token(profiler_shared_sampling_token, 0)
...
<Hexagon> for (output) {
    halide_profiler_acquire_sampling_token(profiler_shared_sampling_token, profiler_local_sampling_token) // profiler_shared_sampling_token is not a valid read address here
}

@aankit-quic aankit-quic mentioned this pull request Apr 11, 2024
Open
mcourteaux
mcourteaux reviewed Apr 11, 2024
View reviewed changes
src/runtime/hexagon_host.cpp
if (remote_profiler_set_current_func) {
remote_profiler_set_current_func(instance->current_func);
if(instance) {
if (instance->next) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If both instance and instance->next are non-null, then the bug has already occurred, right? Only one instance should ever be in the linked list? Or am I misunderstanding something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't that why there is an error thrown inside the if (instance->next) block?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that the error should be generated upon attempting to assign to instance->next. @abadams Implemented this bit of logic actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check on this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this piece of code from abadams in halide_profiler_instance_start should already take care of it.

           if (s->get_remote_profiler_state) {
                error(user_context) << "Cannot profile pipeline " << pipeline_name
                                    << " while pipeline " << s->instances->pipeline_stats->name
                                    << " is running, because it is running on a device.";
                return halide_error_code_cannot_profile_pipeline;
            }

I think it will make sure that only one instance exists for hexagon. So we can let this code be as is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcourteaux Can you confirm if I should do anything to address this comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I was hoping that @abadams could read what I wrote in the first comment, and judge. I have never worked with HVX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abadams Any thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

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

The code in halide_profiler_instance_start prevents profiling anything if there's currently an on-device profiling instance. The code above prevents profiling an on-device instance if there's currently anything being profiled. These are not quite the same thing. If you have an on-device profiling instance and an off-device one, you hit a different check depending on the order they start in.

src/runtime/hexagon_remote/qurt/sim_remote.cpp
halide_profiler_instance_state hvx_profiler_instance;
int *profiler_current_func_addr = &hvx_profiler_instance.current_func;

halide_profiler_instance_state *halide_hexagon_remote_profiler_get_global_instance() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you on purpose create two separate instances of halide_profiler_state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing. Fixed it.

src/runtime/hexagon_remote/qurt/sim_remote.cpp Outdated
halide_profiler_instance_state *halide_profiler_get_state() {
return (halide_profiler_instance_state *)(&profiler_state);
halide_profiler_state *halide_profiler_get_state() {
static halide_profiler_state profiler_state;
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you on purpose create two separate instances of halide_profiler_state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing. Fixed it.

@abadams
Copy link
Member

abadams commented Apr 11, 2024

Oh yeah, that sampling token didn't account for crossing a host-device boundaries inside a parallel loop. Hrm.

@steven-johnson
Copy link
Contributor

What's the status on this PR?

@abadams
Copy link
Member

abadams commented Apr 30, 2024

I believe it's stalled on me

@abadams abadams added the buildbot_test_hvx All Hexagon-related tests should be built and run for this PR. label May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abadams abadams abadams left review comments

@mcourteaux mcourteaux mcourteaux left review comments

@halidebuildbots halidebuildbots Awaiting requested review from halidebuildbots

Assignees
No one assigned
Labels
buildbot_test_hvx All Hexagon-related tests should be built and run for this PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aankit-quic @abadams @steven-johnson @mcourteaux