On Sat, Jan 16, 2021 at 07:19:51PM +0000, Kwok Cheung Yeung wrote:
> > I think you don't need this loop, instead you could just check
> > if (bitmap_bit_p (&generic_head, DECL_UID (detach_decl))
> > || bitmap_bit_p (&firstprivate_head, DECL_UID (detach_decl))
> > || bitmap_bit_p (&lastprivate_head, DECL_UID (detach_decl)))
> >
>
> I think the main problem with this is that you cannot then point to the
> location of the offending data-sharing clause. Given a task construct with
> 'detach(x) shared(x)', I would tend to think of the 'shared(x)' as being the
> incorrect part here, and so would want the error to point to it. Unless you
> have any objections, I am inclined to keep this as it is?
Ok. As detach clause is at most one, the loop is acceptable, but we
certainly would want to avoid O(n^2) complexities in number of clauses.
> I've tried this diff:
>
> case OMP_CLAUSE_DETACH:
> - decl = OMP_CLAUSE_DECL (c);
> - goto do_notice;
> + flags = GOVD_FIRSTPRIVATE | GOVD_SEEN;
> + goto do_add;
>
> and just asserted that a suitable firstprivate clause is found in
> finish_taskreg_scan, and it seems to work fine :-).
Yeah, that should DTRT.
>
> > > + #pragma omp task detach (x) detach (y) /* { dg-error "there can be at
> > > most one 'detach' clause in a task construct" } */
> >
> > It would be on a task construct rather than in a task construct, but the
> > common wording for this diagnostics is
> > "too many %qs clauses", "detach"
> > Please use that wording.
>
> Done, though I don't see the point of using a %qs format code with a
> constant string here...
Helping translators.
They already have the "too many %qs clauses" string to translate (and many
have translated it already), the detach word shouldn't be translated, and we
don't want them to translate
"too many %<detach%> clauses"
"too many %<if%> clauses"
"too many %<schedule%> clauses"
...
>
> I have applied your other suggestions, and have retested the gomp.exp and
> libgomp tests. The full testrun started yesterday showed no regressions. If
> you have no further issues then I will commit this later tonight ahead of
> stage4.
LGTM, thanks.
Jakub
