On Fri, 24 Jul 2020 16:18:34 +0200 Thomas Schwinge <tho...@codesourcery.com> wrote:
> Hi Julian! > > On 2020-06-16T15:39:45-0700, Julian Brown <jul...@codesourcery.com> > wrote: > > This patch fixes a set of XFAILs > > The overall goal of couse is not to resolve XFAILs, but to implement > the expected behavior. :-) Eh, details :-). > > in some recently-added patches by > > skipping a detach operation on "no-op" exit data operations for > > blocks with zero dynamic refcount. > > So this is one aspect of <https://gcc.gnu.org/PR95203> "OpenACC 2.6 > deep copy: attach/detach API routines: no-op behavior". Not quite, I don't think -- that's about pointers (or pointed-to blocks) that are not mapped on the target. With this patch, we have a detach operation with a block with a zero dynamic refcount, but a non-zero structured refcount -- i.e. it's still mapped. So I think the patch is necessary, but not sufficient for the other cases you mention. > > This takes advantage of the ordering of > > detach clauses with respect to associated data-movement clauses: > > i.e., they are grouped together adjacently. > > I'm not convinced that it's sufficient to just special-case these > cases. Instead, per the OpenACC "Data Clause Actions" etc., shouldn't > basically all 'gomp_fatal's that we have on 'attach'/'detach' code > paths turn into no-ops ("no action is taken")? > > > And I'd then again like to bring forward my idea from another review: > > | we may (rather easily?) add a flag variable (ICV; > | initialized from an environment variable) to guard this checking > | behavior? > > Here: to *keep* the 'gomp_fatal's. > > | I suppose we may now have a few libgomp testcases that > | actually do [check things via expected 'gomp_fatal's], > | which wouldn't work any longer [...]. > > Such testcases could then 'dg-set-target-env-var "GOMP_ATTACH_FATAL" > "1"' (better name is desirable), and have one variant with and one > variant without that enabled. > > > Before you start re-working the patch, let's please first get > agreement on what exactly we intend to achieve. Hm, you are probably right about the no-op behaviour for attach operations (but slightly ugh in terms of usability), but I don't think that's really the problem the patch addresses. As for the user-tweakable checking -- yeah maybe it could work, but I don't think I'm going to have time to work on that at the moment. Sorry! HTH, Julian
