This is an archive of the discontinued LLVM Phabricator instance.

[Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute
ClosedPublic

Authored by agozillon on Feb 27 2023, 5:11 AM.

Details

Summary

This patch adds the -fopenmp-is-device flag to Flang's FC1 and also
links it up to the application of an attribute to the mlir builtin.module
on lowering. Indiciating whether the module is for host or device.

This omp.is_device module attribute is a light weight adhoc attribute
with noconcrete ODS or class specification, similar to FIR and LLVM's
data_layout, defaultkind and kindmap's. Its intent is simply to carry
information for the moment, it affects transformations (or will in the
future) but does no transformations itself.

This patch currently does not add the fopenmp-targets flag,
e.g. amdgcn-amd-amdhsa=amdgcn-amd-amdhsa which in Clang
populates the cc1 commands with -fopenmp-is-device, so the only
way to utilise the -fopenmp-is-device flag is to directly use -fc1 with flang-new.

Diff Detail

Event Timeline

agozillon created this revision.Feb 27 2023, 5:11 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
agozillon requested review of this revision.Feb 27 2023, 5:11 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
agozillon updated this revision to Diff 500744.Feb 27 2023, 5:24 AM

Forgot to add an additional test in initial patch!

Could you update the discourse thread (https://discourse.llvm.org/t/rfc-omp-module-and-omp-function-vs-dialect-attributes-to-encode-openmp-properties/67998) with the chosen approach and reasons before proceeding?

Tests?

Happy to add more, I am just a little unsure what the best way to test this is other than application of the attribute to the module when the flag was used and verification that the flag was active in FC1, do you have any suggestions? I don't really think there is a way to test for the attribute in OpenMP/invalid.mlir (as no specified verifier) or in OpenMP/ops.mlir as it is not a predefined operation or attribute, but I can look at how fir.kindmap (which is applied similarly) is tested and see if they do anything particular.

agozillon updated this revision to Diff 501141.Feb 28 2023, 7:49 AM
  • [MLIR][OpenMP] Fix attribute helpers to apply to more than builtin.module
agozillon updated this revision to Diff 501645.Mar 1 2023, 12:57 PM
  • [Flang][mlir] Adding space at the end of the omp-is-device test file

After asking @jsjodin and @skatrak for some input on additional tests that could be added and having a look around the existing tests we couldn't really come up with any additional tests to add to the ones that are in the patch at the moment (inside of omp-is-device.f90 and driver-help.f90) unfortunately. Primarily as there isn't any use of this attribute yet, although there will be quite soon.

awarzynski added inline comments.Mar 2 2023, 3:34 AM
clang/lib/Driver/ToolChains/Flang.cpp
121–125

Can you add a test for this section?

flang/test/Lower/OpenMP/omp-is-device.f90
2

What happens if -fopenmp-is-device is used without -fopnemp?

flang/tools/bbc/bbc.cpp
127

This option is not tested

agozillon updated this revision to Diff 502134.Mar 3 2023, 8:01 AM
  • [MLIR][OpenMP] Fix attribute helpers to apply to more than builtin.module
  • [Flang][mlir] Adding space at the end of the omp-is-device test file
  • [Clang][Flang][Driver][Test] Add flang-omp.f90 to test fopenmp/fopenmp-is-device for flang driver-mode
  • [Flang][MLIR][OpenMP] Add additional tests to omp-is-device.f90 test
agozillon updated this revision to Diff 502139.Mar 3 2023, 8:19 AM
  • [Flang][MLIR][Test] Add newline at end of test file
agozillon added a comment.EditedMar 3 2023, 9:03 AM

The new update attempts to add the suggested new tests! Thank you very much @awarzynski for suggesting them that was a great help. Hopefully the new additions cover what you have requested, if not I am of course happy to change or add more tests as needed.

clang/lib/Driver/ToolChains/Flang.cpp
121–125

I have added a new test file flang-omp.f90 which aims to test that the appropriate flags are generated when openmp related flags are used it currently checks for -fopenmp/fopenmp-is-device

flang/test/Lower/OpenMP/omp-is-device.f90
2

I think I now cover this in the test with the *-DEVICE-FLAG-ONLY tests, currently it is silently ignored and nothing is applied to the module.

flang/tools/bbc/bbc.cpp
127

I believe I now test this in the omp-is-device.f90 test!

awarzynski accepted this revision.Mar 3 2023, 9:40 AM

Thank you very much @awarzynski for suggesting them that was a great help.

I'm happy that I could help :) The driver logic LGTM, but please wait for either @jdoerfert and/or @kiranchandramohan to also approve. Thanks for contributing!

clang/test/Driver/flang/flang-omp.f90
1

This test looks correct to me, but please note that:

! Check that flang -fc1 is invoked when in --driver-mode=flang

yet (%clang instead of %flang)

! RUN: %clang --driver-mode=flang -### -fopenmp %s 2>&1 | FileCheck --check-prefixes=CHECK-OPENMP %s

I'm not really sure whether we should be testing Flang-specific logic in Clang. Having said that, Flang does use clangDriver to implement its driver :)

You could consider using https://github.com/llvm/llvm-project/blob/main/flang/test/Driver/frontend-forwarding.f90 instead (or add an OpenMP specific file there).

Not a blocker.

This revision is now accepted and ready to land.Mar 3 2023, 9:40 AM

Thank you very much @awarzynski I will wait for further approval!

clang/test/Driver/flang/flang-omp.f90
1

Yes, I wasn't so sure either as it felt a little weird to test Flang components inside of Clang, but as you said it is a Clang toolchain (that this test is checking) and borrows from the clangDriver!

I borrowed this test from other similar tests in the same folder that test other flang specific driver logic in a similar manner, but I am more than happy to add an additional flang specific driver test as you mention!

agozillon added inline comments.Mar 3 2023, 11:57 AM
clang/test/Driver/flang/flang-omp.f90
1

On further looking into the frontend-forwarding test, I don't know if it is suitable for fopenmp-is-device as it is an fc1 option and won't be forwarded from the flang-new frontend down to fc1 at the moment!

I think this test will be more suitable when additional flags like the fopenmp-targets (or similar flags) that are used in this test are added to the Flang driver. As they spawn/propagate the openmp-is-device flag. However, perhaps I am incorrect.

awarzynski added inline comments.Mar 3 2023, 2:14 PM
clang/test/Driver/flang/flang-omp.f90
1

On further looking into the frontend-forwarding test, I don't know if it is suitable for fopenmp-is-device as it is an fc1 option and won't be forwarded from the flang-new frontend down to fc1 at the moment!

It should be - that's what the logic in Flang.cpp does, no? And if it doesn't, is it a deliberate design decision?

agozillon added inline comments.Mar 6 2023, 6:03 AM
clang/test/Driver/flang/flang-omp.f90
1

I believe the logic in Flang.h/Flang.cpp just invokes for -fc1 not the Frontend driver, at least that's what it looks like from the ConstructJob task that Clang invokes.

It's a deliberate design decision that -fopenmp-is-device is an fc1 option, it mimics the way Clang currently does it, which is a cc1 option, there is no way to directly specify it to clang without -cc1 I think. The hope is to keep the Flang OpenMP flags as similar to Clang's as possible for the time being I believe.

awarzynski added inline comments.Mar 6 2023, 10:10 AM
clang/test/Driver/flang/flang-omp.f90
1

I believe the logic in Flang.h/Flang.cpp just invokes for -fc1 not the Frontend driver.

flang-new -fc1 is the frontend driver ;-) So:

  • you've added the logic to forward -fopenmp-is-device that should work for both flang-new and clang --driver-mode=flang, but
  • you're only testing one of these.

There should have tests for both. In particular for flang-new given that this patch adds new functionality to LLVM Flang. If that doesn't work, let's try to figure it out together.

The hope is to keep the Flang OpenMP flags as similar to Clang's as possible for the time being I believe.

+1 We should try as much as we can to keep Clang's and Flang's behavior consistent :) (which you do 👍🏻 )

agozillon added inline comments.Mar 6 2023, 12:12 PM
clang/test/Driver/flang/flang-omp.f90
1

Ah I didn't realise that, thank you very much for clarifying that for me! :) I thought it was bypassing it with -fc1, and yjsy flang-new without it was the frontend! I still have a lot to learn, so I apologies for the misunderstanding!

In this case do you mean test with "flang-new -fc1 -fopenmp-is-device" or "flang-new -fopenmp-is-device", as the latter will not be accepted by flang-new in the current patch but I do test the -fc1 variation in the omp-is-device.f90 test which is more about testing the attribute admittedly as opposed to the driver command.

I don't know if testing it in the frontend-forwarding.f90 test is the right location in this case as it doesn't test with -fc1, it seems to test options are forwarded to -fc1 correctly, I could be incorrect though. In future passing an argument like -fopenmp-targets=amdgcn-amd-amdhsa to flang-new would likely expand into -fopenmp-is-device (and other arguments), like it does for Clang at the moment, so this is likely the option that would be tested inside of frontend-forwarding.f90 (and the subsequent check-case would make sure it expanded into -fopenmp-is-device and friends correctly).

However, I am more than happy to add a test for the forwarding behavior, I'm just not necessarily sure what that test would look like unfortunately, so I appologies!

awarzynski added inline comments.Mar 7 2023, 2:12 AM
clang/test/Driver/flang/flang-omp.f90
1

I still have a lot to learn

Don't we all? :)

I apologies for the misunderstanding!

No worries! This could be helpful: https://github.com/llvm/llvm-project/blob/main/flang/docs/FlangDriver.md

I don't know if testing it in the frontend-forwarding.f90

I agree that probably not. In this case, a "compiler driver" flag ( -fopenmp-targets) implies a certain "frontend driver" flag ("-fopenmp-is-device). That's not really forwarding.

In future passing an argument like -fopenmp-targets=amdgcn-amd-amdhsa to flang-new

Ah, that's something I've missed and that's my ignorance kicking in, sorry. Correct me if I am wrong:

  1. this patch adds -fopnemp-is-device to Flang's frontend driver (that's the logic in CompilerInvation.cpp),
  2. the logic in Flang.cpp means that despite flang-new not supporting -fopenmp-targets= just yet, it is possible to use clang --driver-mode=flang -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa instead of flang-new -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa.

I've completely missed the fact that flang-new is missing -fopenmp-targets (I would appreciate that being mentioned in the summary).

Do we need to have 2. at this point? Ideally, we would add -fopenmp-targets= to flang-new first and then test with:

  • flang-new -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa (this could happen in a subsequent patch, the logic for the frontend driver is still fine).

While testing with clang is also correct, this:

  • %clang --driver-mode=flang -### -fopenmp -fopenmp-targets=amdgcn-amd-amdhsa %s

feels rather counter-intuitive and indeed very uncommon.

Just from this patch alone I feel that 2. is not yet needed and you could safely:

  • remove the changes in Flang.cpp, and
  • delete flang-omp.f90).

You could revisit these once -fopenmp-targets is ready. It's totally fine to add "frontend driver" (flang-new -fc1) and "compiler driver" (flang-new) logic separately (just make it clear in the summary). WDYT?

agozillon edited the summary of this revision. (Show Details)Mar 7 2023, 3:58 AM
agozillon added inline comments.Mar 7 2023, 4:43 AM
clang/test/Driver/flang/flang-omp.f90
1

Thank you very much for the link! Very useful and comprehensive documentation :)

  1. That is correct, and it works like -fopenmp-is-device in Clang
  2. That is correct unfortunately, but we do fully intend to support fopenmp-targets in the very near future (another team member is working on this patch), I just thought it would be nice to have all the fopenmp-is-device components in a single patch!

I have added it to the summary, sorry for missing it previously!

It's not needed within this patch no, but it does keep it self contained within one patch and other patches can build off of it a little easier, but I am happy to separate the logic into two different patches if that is preferred!

agozillon updated this revision to Diff 503008.Mar 7 2023, 5:42 AM
  • [Flang][ToolChain][OpenMP][Driver] Reduce changes to only add -fc1 support

The recent update removed the added offload related code from Flang.h/.cpp and removed the related test.

Would it be possible to have an extra sign-off as requested by @awarzynski (or more review points to correct/discuss if we aren't happy with the state/direction of the patch) from either @kiranchandramohan or @jdoerfert my apologies for the pings!

And then provided @awarzynski is happy with the current state of the patch after this recent commit and no further comments I can commit and close the patch :)

Thank you very much for all the excellent comments in this review, and the attention given to it, I appreciate it greatly.

kiranchandramohan accepted this revision.Mar 7 2023, 7:13 AM

LG. See one minor comment in the tests.

I would prefer having an Interface for Target Modules if that could be made to work. I guess this can be taken up separately after https://reviews.llvm.org/D144883.

flang/test/Lower/OpenMP/omp-is-device.f90
9–18

Any reason to have separate checks for essentially the same (e.g: DEVICE vs BBC-DEVICE)?

LG. See one minor comment in the tests.

I would prefer having an Interface for Target Modules if that could be made to work. I guess this can be taken up separately after https://reviews.llvm.org/D144883.

Thank you Kiran, by interface for Target Modules do you mean a new omp.module or this type of external interface: https://mlir.llvm.org/docs/Interfaces/#external-models-for-attribute-operation-and-type-interfaces

And quite possibly, @skatrak is currently looking into the review comments in the referenced patch (as he found similar use for the patch), so we shall see where the results lead to. Never opposed to improving on the infrastructure and this is something we will iterate on as we progress with offloading! :)

flang/test/Lower/OpenMP/omp-is-device.f90
9–18

In this case to test the addition of the flag to bbc and also to the Flang driver, and testing the flag has the same behavior in both. There is perhaps a way to make the test simpler that I'm unaware of though? e.g. a way to merge DEVICE/BBC-DEVICE into one check

LG. See one minor comment in the tests.

I would prefer having an Interface for Target Modules if that could be made to work. I guess this can be taken up separately after https://reviews.llvm.org/D144883.

Thank you Kiran, by interface for Target Modules do you mean a new omp.module or this type of external interface: https://mlir.llvm.org/docs/Interfaces/#external-models-for-attribute-operation-and-type-interfaces

And quite possibly, @skatrak is currently looking into the review comments in the referenced patch (as he found similar use for the patch), so we shall see where the results lead to. Never opposed to improving on the infrastructure and this is something we will iterate on as we progress with offloading! :)

The latter one (external interface).

flang/test/Lower/OpenMP/omp-is-device.f90
9–18

If DEVICE and BBC-DEVICE are checking the same things, then you only need to have one of them (say DEVICE) and use it in the check-prefix of both.

awarzynski accepted this revision.Mar 7 2023, 8:16 AM

And then provided @awarzynski is happy with the current state of the patch

LGTM, thanks for the updates and for working on this!

agozillon updated this revision to Diff 503070.Mar 7 2023, 8:47 AM
  • [Flang][Driver] Tidy up omp-is-device.f90 test

Cleaned up the test, thank you both for teaching me more about the compiler and test infrastructure! If you're happy with the test changes @kiranchandramohan I'll commit the changes upstream to close out the review?

This revision was landed with ongoing or failed builds.Mar 7 2023, 10:58 AM
This revision was automatically updated to reflect the committed changes.