This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Make enabling the new driver more generic
ClosedPublic

Authored by jhuber6 on Apr 7 2022, 10:25 AM.

Details

Summary

In preparation for allowing other offloading kinds to use the new driver
a new opt-in flag -foffload-new-driver is added. This is distinct from
the existing -fopenmp-new-driver because OpenMP will soon use the new
driver by default while the others should not.

Diff Detail

Event Timeline

jhuber6 created this revision.Apr 7 2022, 10:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2022, 10:25 AM
jhuber6 requested review of this revision.Apr 7 2022, 10:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2022, 10:25 AM
jhuber6 updated this revision to Diff 421595.Apr 8 2022, 11:15 AM

Fix misplaced logic symbol that broke tests.

yaxunl added inline comments.Apr 18 2022, 9:09 AM
clang/lib/Driver/Driver.cpp
3979–3982

If OPT_fopenmp_new_driver is on by default, then OPT_foffload_new_driver will have no effect. Is this intended? Should OPT_fopenmp_new_driver be conditioned for OpenMP offloading only?

jhuber6 added inline comments.Apr 18 2022, 9:21 AM
clang/lib/Driver/Driver.cpp
3979–3982

Yeah, I need to rework the logic here once OpenMP moves to the new driver by default. I can probably require something like.

(C.isOffloadingHostKind(Action::OFK_OpenMP) && !Args.hasArg(options::OPT_fno_offload_new_driver) || Args.hasArg(options::OPT_fopenmp_new_driver);

Currently working on the patch to make this default for OpenMP so I'll be able to update this after that.

jhuber6 updated this revision to Diff 423459.Apr 18 2022, 1:13 PM

Rebase & update after making the new driver default for OpenMP.

jhuber6 updated this revision to Diff 423514.Apr 18 2022, 8:13 PM

Removing unnecessary change

should we have a test to show the effect of -foffload-new-driver on -ccc-print-phases for CUDA/HIP?

should we have a test to show the effect of -foffload-new-driver on -ccc-print-phases for CUDA/HIP?

We probably should, it's tested w/ OpenMP but we should check the bound architectures and such. I can add one.

should we have a test to show the effect of -foffload-new-driver on -ccc-print-phases for CUDA/HIP?

I decided to add the test in D120272 because this file only adds support for the option.

jhuber6 updated this revision to Diff 425785.Apr 28 2022, 8:09 AM

Ping and update.

yaxunl accepted this revision.Apr 28 2022, 8:51 AM

LGTM. Thanks.

This revision is now accepted and ready to land.Apr 28 2022, 8:51 AM
tra added inline comments.Apr 28 2022, 11:03 AM
clang/include/clang/Driver/Options.td
2535–2538

Nit: Can we combine that into a BoolFOption instead of using Flag ?

Also, considering that it's an option which controls the driver's behavior, not just tweaks code generation, perhaps it should be just --[no]offload-new-driver.

jhuber6 added inline comments.Apr 28 2022, 11:15 AM
clang/include/clang/Driver/Options.td
2535–2538

The -fopenmp variant was in LLVM-14 so that should stay at least. I could change it to be --offload (Although this does tweak code generation in a later patch to make the offloading entries). I most just used -f because all the other OpenMP options use -f. I could definitely use BoolFOption but I don't think it saves too much in this situation.

tra added inline comments.Apr 28 2022, 12:05 PM
clang/include/clang/Driver/Options.td
2535–2538

Naming is hard. :-) Real world things don't want to fit into neat categories.

I think --offload* would fit the general pattern of transitioning niche features (be it CUDA,HIP, OpenMP) into something more general (offload, GPU).

jhuber6 added inline comments.Apr 28 2022, 12:09 PM
clang/include/clang/Driver/Options.td
2535–2538

Sure, I'll change it to just be offload.

jhuber6 updated this revision to Diff 425879.Apr 28 2022, 12:22 PM

Changin -f[no-]offload-new-driver into --[no-]offload-new-driver

tra accepted this revision.Apr 28 2022, 3:15 PM
This revision was landed with ongoing or failed builds.Apr 29 2022, 6:15 AM
This revision was automatically updated to reflect the committed changes.