This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Avoid costly shadow map traversals whenever possible
ClosedPublic

Authored by jhuber6 on Nov 3 2021, 10:02 AM.

Details

Summary

In the OpenMC app we saw omp target update spending an awful lot of
time in the shadow map traversal without ever doing any update there.
There are two cases that allow us to avoid the traversal completely.
The simplest thing is that small updates cannot (reasonably) contain
an attached pointer part. The other case requires to track in the
mapping table if an entry might contain an attached pointer as part.
Given that we have a single location shadow map entries are created,
the latter is actually fairly easy as well.

Diff Detail

Event Timeline

jdoerfert created this revision.Nov 3 2021, 10:02 AM
jdoerfert requested review of this revision.Nov 3 2021, 10:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2021, 10:02 AM
Herald added a subscriber: sstefan1. · View Herald Transcript
jdoerfert updated this revision to Diff 384534.Nov 3 2021, 11:26 AM

Update api.cpp

grokos accepted this revision.Nov 10 2021, 1:36 PM

LGTM with two minor nits.

openmp/libomptarget/src/device.cpp
343

Can you add a comment explaining that IsNew is always going to be false because getTgtPtrBegin is called from targetDataEnd, targetDataContiguous and processDataBefore, i.e. for data which is expected to be mapped already. And maybe it's more elegant to hardcode false in the return statement directly, thus avoiding defining a variable which is never used in the body of this function.

openmp/libomptarget/src/omptarget.cpp
619

an be --> can be

This revision is now accepted and ready to land.Nov 10 2021, 1:36 PM
grokos added a comment.Dec 6 2021, 5:17 PM

Ping. I'm preparing some more ShadowPtrMap optimizations which I'd like to base on top of this patch. Is there any planned change or can we land the patch?

jhuber6 commandeered this revision.Dec 10 2021, 9:52 AM
jhuber6 edited reviewers, added: jdoerfert; removed: jhuber6.

Commandeering to land for @jdoerfert.

jhuber6 updated this revision to Diff 393527.Dec 10 2021, 9:53 AM

Addressing comments and fixing errors in debug build.

i see the same failures in my local build, and when i revert this commit, and rebuild/test, failures go away

This seems to have broken some x64 openmp tests, e.g. https://lab.llvm.org/buildbot/#/builders/193/builds/2940

i see the same failures in my local build, and when i revert this commit, and rebuild/test, failures go away

Yeah, I can see those failures now. I can revert until it's figured out.

jhuber6 reopened this revision.Dec 10 2021, 12:58 PM
This revision is now accepted and ready to land.Dec 10 2021, 12:58 PM
jdoerfert updated this revision to Diff 401292.Jan 19 2022, 9:45 AM

Fix errors, uninitialized iterator and wrong TPR (obj was used, ptr is needed).

This revision was landed with ongoing or failed builds.Jan 19 2022, 8:15 PM
This revision was automatically updated to reflect the committed changes.