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

Attempt to fix #7703 #7706

Merged
merged 9 commits into from
Aug 1, 2023
Merged

Attempt to fix #7703 #7706

merged 9 commits into from
Aug 1, 2023

Conversation

steven-johnson
Copy link
Contributor

This fixes #7703, in the sense that we no longer play dubious games with unique_ptr. As a bonus, the failure in #7699 is now reproducible without using ASAN (it becomes an assertion failure). Not sure if it addresses #7606 or not.

Not sure if we should land this as-is or wait for a full fix -- landing this will inject reliable failures on the buildbots.

@steven-johnson
Copy link
Contributor Author

@aekul -- if you don't have the bandwidth to look at this, if there are any pointers you can give to get me started on fixing it, I'd appreciate it; I have some co-workers who want to try this out, but I don't want to give it to them until we can work out the crashy bits :-)

@aekul
Copy link
Contributor

aekul commented Jul 28, 2023

@steven-johnson Just started looking into this. Using a shared_ptr is probably the way to go but I'll investigate a bit more in case I'm overlooking something.

@aekul
Copy link
Contributor

aekul commented Jul 31, 2023

Using a shared_ptr makes sense here. The ThreadInfo is intended to be created once (the first time compute_features is called on the thread loop) and then shared across multiple GPULoopInfo objects that are created in the recursive calls to compute_features.

There's an assumption in the code that when accessing the ThreadInfo object, we should be at a loop nest inside a thread loop, so the object should be non-null. If that's not the case, then it indicates a bug not in the ThreadInfo creation code but in how that particular Func is handled e.g. in #7699 the issue is that scalars are not handled properly.

@steven-johnson were these changes finalized or were there other things you planned to do? If they're final, I'll approve. This will probably create merge conflicts with #7726 though.

@steven-johnson
Copy link
Contributor Author

Let's get #7726 landed and then revisit this with that in mind -- I approved #7726 but want to let the buildbots do at least some spot-builds before landing it, hopefully later today

@steven-johnson
Copy link
Contributor Author

PTAL -- look at the TODO comment for get_thread_info() too.

Copy link
Contributor

@aekul aekul 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 for fixing this

src/autoschedulers/anderson2021/GPULoopInfo.h Show resolved Hide resolved
src/autoschedulers/anderson2021/LoopNest.cpp Outdated Show resolved Hide resolved
@steven-johnson
Copy link
Contributor Author

Failures are unrelated, landing

@steven-johnson steven-johnson merged commit 0839270 into main Aug 1, 2023
3 checks passed
ardier pushed a commit to ardier/Halide-mutation that referenced this pull request Mar 3, 2024
* Attempt to fix halide#7703

* fixes

* Update LoopNest.cpp

* Update GPULoopInfo.h

* Fixes.

* clang-tidy
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.

Anderson2021 autoscheduler plays dubious games with unique_ptr<>
2 participants