This is an archive of the discontinued LLVM Phabricator instance.

[clang][OpenMP] Add interop support for multiple depend clauses
ClosedPublic

Authored by mhalk on Jul 19 2023, 3:38 AM.

Details

Summary

This patch removes the constraint of the interop directive where only a single
depend clause was allowed.

Diff Detail

Event Timeline

mhalk created this revision.Jul 19 2023, 3:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 3:38 AM
mhalk requested review of this revision.Jul 19 2023, 3:38 AM
Herald added a project: Restricted Project. · View Herald Transcript
mhalk updated this revision to Diff 541942.Jul 19 2023, 3:43 AM

Fix accidental remove of assert(...).

mhalk added inline comments.Jul 19 2023, 3:54 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6985

Not sure if this is useful here.
Saw this RunCleanupsScope in conjunction with the emission of dependences + calls.

But, at least in the generated IR it won't remove duplicate depend clauses.

If anybody can comment on this, that would be great.
Otherwise, I'd tend to remove it.

ABataev added inline comments.Jul 19 2023, 5:36 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6985

Not required here

mhalk added inline comments.Jul 19 2023, 6:09 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
6985

Thank you! Will be removed.

mhalk updated this revision to Diff 542032.Jul 19 2023, 8:05 AM

Rebase + removed RunCleanupsScope for preparation w.r.t. D137607

We don't need to update the interface function to indicate how many deps we have?

mhalk added a comment.Jul 19 2023, 9:57 AM

We don't need to update the interface function to indicate how many deps we have?

Which interface function exactly? __tgt_interop_[init|use|destroy]?

We don't need to update the interface function to indicate how many deps we have?

Which interface function exactly? __tgt_interop_[init|use|destroy]?

Yup.

mhalk added a comment.EditedJul 19 2023, 11:21 AM

As far as I can tell, no.
This *should* already be covered by Ndeps + DepList from __tgt_interop_init's signature:

__tgt_interop_init(ident_t *LocRef, kmp_int32 Gtid,
                   [...],
                   kmp_int32 Ndeps, kmp_depend_info_t *DepList,
                   kmp_int32 HaveNowait)

Which is very similar to the signature of __kmpc_omp_taskwait_deps_51 -- which also uses the result of buildDependences:

__kmpc_omp_taskwait_deps_51(ident_t *loc_ref, kmp_int32 gtid,
                            kmp_int32 ndeps, kmp_depend_info_t *dep_list,
                            [...], kmp_int32 has_no_wait);

I guess the naming should be improved, changed that to DependenceList.
And I did not set the HasNowaitClause correctly, will update immediately.

mhalk updated this revision to Diff 542130.Jul 19 2023, 11:26 AM

Fixed HasNowaitClause always set to zero
Renamed corresponding variable to DependenceList, to better reflect its meaning.

This revision is now accepted and ready to land.Jul 19 2023, 11:28 AM