[Openmp-commits] [PATCH] D98838: [OpenMP] Fixed a crash in hidden helper thread

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

In D98838#2635384 <https://reviews.llvm.org/D98838#2635384>, @jdoerfert wrote:

> Let me be direct for a second so we don't end up here again in a few months:
> The patch was on phab for ~1 year, nobody cared, this is a very common phenomena.
> It also has been merged for weeks. I get the fact that we want a stable release
> but showing up last minute just saying we need to pull stuff is *not* helpful
> from an overall perspective. I say this especially because the number of people/
> organizations that develop and upstream complex features is very limited. If you
> want to benefit from such efforts you should be prepared to help, IMHO. That does
> mean to do some testing and reviewing *before* the last release candidate is due.
> Not to say this was not tested, but the capabilities are arguably different here.

Well, sort of. We reported segfaults on it pretty much when it landed,
https://reviews.llvm.org/D77609  has a comment from Jan 18 this year. It landed after:

> The test still doesn't work ideally on amdgpu, but it no longer crashes, and 
> some of the print statements within the target region are seen.

which given it's host only code was a fairly clear sign that things weren't right.

Granted Ron & I didn't review or read the change (as far as I know), but then we're
mostly fighting to get amdgpu working and didn't anticipate a host-only change
breaking gpu offloading.

In the spirit of direct, I didn't care about this change until it landed and broke stuff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98838/new/

https://reviews.llvm.org/D98838