This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Make the new device runtime the default
ClosedPublic

Authored by jhuber6 on Dec 1 2021, 10:18 AM.

Details

Summary

This patch changes the -fopenmp-target-new-runtime option which controls if
the new or old device runtime is used to be true by default. Disabling this to
use the old runtime now requires using -fno-openmp-target-new-runtime.

Diff Detail

Event Timeline

jhuber6 created this revision.Dec 1 2021, 10:18 AM
jhuber6 requested review of this revision.Dec 1 2021, 10:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2021, 10:18 AM

That's quite the change! I think it's about time.

tianshilei1992 accepted this revision.Dec 1 2021, 10:50 AM
This revision is now accepted and ready to land.Dec 1 2021, 10:50 AM
jhuber6 updated this revision to Diff 391079.Dec 1 2021, 10:53 AM

Fixing driver tests that change with this.

D114891 enables this for the amdgpu tests.

This patch will leave the nvptx tests running on the new runtime twice, and not on the old runtime at all, I think. lit.cfg should probably use old + new explicitly, instead of default + new

D114891 enables this for the amdgpu tests.

This patch will leave the nvptx tests running on the new runtime twice, and not on the old runtime at all, I think. lit.cfg should probably use old + new explicitly, instead of default + new

Do we still want to run tests for the old device runtime?

Do we still want to run tests for the old device runtime?

Maybe? We definitely don't want to run the tests for the new one twice

Do we still want to run tests for the old device runtime?

Maybe? We definitely don't want to run the tests for the new one twice

Oh that's definitely right.

I'm thinking we should also disable testing for the old device runtime.

We want amdgcn to remain on old deviceRTL till we have verified it . I made inline comments on how this could be done.

clang/lib/Driver/ToolChains/Clang.cpp
5905

Add a check for amdgcn something like this.

5907

This will keep AMDGCN on old runtime till we are ready to switch.

gregrodgers requested changes to this revision.Dec 1 2021, 5:53 PM

I forgot to add the "request changes" action.

This revision now requires changes to proceed.Dec 1 2021, 5:53 PM
ronlieb added a subscriber: ronlieb.Dec 1 2021, 6:06 PM

perhaps we can try this patch as is, and if it passes buildbot, let the new DeviceRTL be the default upstream for all targets.
if it fails the AMDGPU buildbot, then perhaps apply the above suggested change of leaving old runtime default for now for AMD.
or consider some other soultion.

in other words, how about we land it, and be ready to revert if bot goes red?

perhaps we can try this patch as is, and if it passes buildbot, let the new DeviceRTL be the default upstream for all targets.
if it fails the AMDGPU buildbot, then perhaps apply the above suggested change of leaving old runtime default for now for AMD.
or consider some other soultion.

in other words, how about we land it, and be ready to revert if bot goes red?

I could land this and make a follow-up patch doing what @gregrodgers suggests.

ronlieb accepted this revision.Dec 1 2021, 7:28 PM

works for me, i think Greg is ok with it too, we chatted internally an hour or so ago

works for me, i think Greg is ok with it too, we chatted internally an hour or so ago

Should I just land it now and sleep or wait until tomorrow? Whichever causes the least downtime for you.

works for me, i think Greg is ok with it too, we chatted internally an hour or so ago

Should I just land it now and sleep or wait until tomorrow? Whichever causes the least downtime for you.

like Jon said, you also have to disable the test for the "new" one because the default one now is the new one.

based on Shilei's last comment, do it in the morning ?

JonChesterfield added a comment.EditedDec 1 2021, 11:25 PM

This will definitely break amdgpu bot without D114865 landed first.

However that patch is currently blocked by Matt, so we may want to land this and disable the amdgpu buildbot until the backend is fixed.

edit: a cleanup to do with or after this is to remove all the lines with -newRTL in them from the libomptarget test files and drop the -newRTL test target from plugins/cuda. Depending on order of patches we might want to remove it from plugins/amdgpu as well, or it might already be missing.

gregrodgers accepted this revision.Dec 2 2021, 6:41 AM

this is ok as is

This revision is now accepted and ready to land.Dec 2 2021, 6:41 AM
JonChesterfield accepted this revision.Dec 2 2021, 8:10 AM

The consensus internally seems to be to land this and see what happens on the amdgpu buildbot. Go for it

This revision was landed with ongoing or failed builds.Dec 2 2021, 8:12 AM
This revision was automatically updated to reflect the committed changes.

thx for trying it out, please revert so we can look into it more on the AMDGPU target

thx for trying it out, please revert so we can look into it more on the AMDGPU target

I'm going to make a new revision to make AMDGPU use the old runtime by default, SG?

yes, thanks, sounds good