[OpenMP][Flang][MLIR] Lowering of requires directive from MLIR to LLVM IR

[skatrak]

Summary

Default atomic ordering information is processed in the OpenMP dialect
to LLVM IR lowering stage at every spot where an operation can be
affected by it. The rest of clauses are stored globally in the
OpenMPIRBuilderConfig object before starting that lowering stage, so
that the OMPIRBuilder can conditionally modify code generation
depending on these. At the end of the process, the omp.requires
attribute is itself lowered into a global constructor that passes these
clauses as flags to the OpenMP runtime.

Depends on D147217, D147218 and D158278.

[skatrak]

Fix unit test failure

[awarzynski]

flang/test/Driver/code-gen-aarch64.f90

21 ↗

(On Diff #512485)

Can you explain the rationale for this extra line? I'm guessing that ret is being generated somewhere? I'm mostly just curious. And also wondering whether there's any way to make this test less fragile :)

[skatrak]

flang/test/Driver/code-gen-aarch64.f90

21 ↗

(On Diff #512485)

This test started failing because the codegen changes resulted in an lretl instruction appear somewhere below (outside of the _QQmain region of code). The change I made was just to make sure that we're only checking that there's no "ret" within the region, which is probably not an ideal way to test either. But since I don't think this test is really related to the patch, I didn't want to make significant changes to it.

I'm not sure how we could make it less fragile, since that part of the test is basically interpreting the input as something it's not. So, if I understand it correctly, we're basically checking "garbage" data making sure that a pattern doesn't appear by chance. We can tighten the pattern to reduce the likelihood of false positives/negatives (which is what I did with that added line), but I suppose they are never going to be zero. A solution could be to use llvm-readelf -h instead of llvm-objdump, and then check for the "Machine" header.

[awarzynski]

flang/test/Driver/code-gen-aarch64.f90

21 ↗

(On Diff #512485)

Sorry about the delay and thanks for the explanation.

The intent of this test is to verify that this fails:

! RUN: %flang_fc1 -emit-obj -triple <some-triple> -o - | llvm-objdump  --triple <some-other-triple>

I couldn't find any other way to leverage this apart from checking for the "return" instruction in the output from llvm-objdump. Sadly, it looks like in your case the disassembler generates a "return" instruction anyway. Although it's under some other Asm label, it's still there. I appreciate your efforts to preserve the intended behavior of this test, but this patch demonstrates how fragile it is regardless. I couldn't find any other way to make it more robust and am leaning towards deleting it altogether (atm there are other tests to verify the --target flag).

Having said that, it is still very unclear to me why your changes affect the non-OpenMP code generation. This test is meant to be unrelated to OpenMP, so shouldn't be affected by what you are updating here, right? Any idea?

[kiranchandramohan]

For code in mlir/lib/Target/LLVMIR/ModuleTranslation.cpp, mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp we should have tests in mlir/test/Target/LLVMIR/.

[skatrak]

Update unit tests.

[skatrak]

Undo changes to unrelated unit test.

flang/test/Driver/code-gen-aarch64.f90

21 ↗

(On Diff #512485)

I couldn't find any other way to leverage this apart from checking for the "return" instruction in the output from llvm-objdump. Sadly, it looks like in your case the disassembler generates a "return" instruction anyway. Although it's under some other Asm label, it's still there. I appreciate your efforts to preserve the intended behavior of this test, but this patch demonstrates how fragile it is regardless. I couldn't find any other way to make it more robust and am leaning towards deleting it altogether (atm there are other tests to verify the --target flag).

I see, I'm not sure either whether this is a good test to make. If llvm-objdump was able to detect if a user is trying to disassemble using the wrong triple and emit an error, then we could check that. Since that seems not to be the case, we could maybe check that flang -c produces a binary for the intended architecture by running it through llvm-readelf -h. This would only check the headers and not the machine code, but maybe that's more useful than removing the test. If other tests already check this, then maybe removing this wouldn't really be an issue.

Having said that, it is still very unclear to me why your changes affect the non-OpenMP code generation. This test is meant to be unrelated to OpenMP, so shouldn't be affected by what you are updating here, right? Any idea?

Thank you for pointing this out, you are right. I spent some time looking into it, and it seems like the issue is that the registration function is being generated also when -fopenmp is not specified, which is a bug. It just turned out that this unit test picked it up by chance because the generated binary for aarch64 happened to contain a ret instruction if interpreted as x86_64. I'll revert changes to that unit test here and fix the bug in one of the patches this one depends on.

[awarzynski]

flang/test/Driver/code-gen-aarch64.f90

21 ↗

(On Diff #512485)

Since that seems not to be the case, we could maybe check that flang -c produces a binary for the intended architecture by running it through llvm-readelf -h. This would only check the headers and not the machine code, but maybe that's more useful than removing the test. If other tests already check this, then maybe removing this wouldn't really be an issue.

I really like this idea - the test could verify e_machine: https://en.wikipedia.org/wiki/Executable_and_Linkable_Format. Are interested in trying? I'd happily review (no spare cycles to do it myself ATM). No worries if not, this is completely unrelated to this patch!

[skatrak]

Rebase and update.

[skatrak]

flang/test/Driver/code-gen-aarch64.f90

21 ↗

(On Diff #512485)

Sure @awarzynski, I think I should be able to find some time for this next week if that's alright. Do you know if this the only test of this type or if there are others that could be changed at the same time?

[awarzynski]

flang/test/Driver/code-gen-aarch64.f90

21 ↗

(On Diff #512485)

Thanks!

There are 3 tests like this:

• flang/test/Driver/code-gen-x86.f90
• flang/test/Driver/code-gen-aarch64.f90
• flang/test/Driver/code-gen-rv64.f90

IIUC, the RISC-V one is already doing something similar to what we've discussed here.

[skatrak]

Fix formatting.

[skatrak]

Update with latest changes to parent patch.

[skatrak]

Update

[kiranchandramohan]

Could you rebase this patch?

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1201–1202

Should this be part of Semantics?

1938–1957

Can this be an operation in the OpenMP dialect?

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

1371

Could you move this to ModuleTranslation::getOpenMPBuilder?

[skatrak]

Thank you @kiranchandramohan for the feedback. Before working on the larger items, I'm requesting some clarification so that I've got a clearer idea of what the preferred approach is. Meanwhile, I'll start working on rebasing the complete stack of related patches and move the code in ModuleTranslation.cpp as suggested.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1201–1202

It would be possible to gather the atomic_default_mem_order clause present in any 'requires' directives in the compilation unit, and then apply it (or 'monotonic', if not present) to all atomic operations with no memory order clauses as a semantic pass. I suppose that would also come together with making the memory order attribute mandatory in MLIR for these operations, since we would be handling the defaults prior to that. Is this a preferable approach?

1938–1957

Do you mean removing the omp.requires attribute and instead having an omp.requires operation to be specified similarly to this?

module {
  omp.requires { clauses = #omp<clause_requires reverse_offload|unified_shared_memory> }
  ...
}

Doing this would allow us to lower the operation in OpenMPDialectLLVMIRTranslationInterface::convertOperation instead of OpenMPDialectLLVMIRTranslationInterface::amendOperation, but the clauses from this directive must be used to modify the lowering of some other operations, not just to produce the registration function.

The current approach allows accessing this information through the OffloadModuleInterface to be passed along to the OpenMPIRBuilder (in ModuleTranslation.cpp, currently), but if we had an operation instead of an attribute we would have to search for it in the module to extract this, which I'm not sure it would be a cleaner solution. Also there would be no guarantee that no more than a single occurrence of this operation would be present within the module at all times (e.g. if the input is handwritten MLIR).

Or is your suggestion to lower every instance of a 'requires' directive in Fortran to its own omp.requires operation at the spot where it appears? Something like this:

module {
  func.func @f() {
    omp.requires { clauses = #omp<clause_requires reverse_offload> }
    ...
  }
  ...
  func.func @g() {
    omp.requires { clauses = #omp<clause_requires unified_shared_memory> }
    ...
  }
}

The issue in that case becomes where to gather all of the clauses and lower to a single registration function in LLVM IR, since convertOperation would be called once per instance otherwise. The gathering of the clauses coming from different instances of 'requires' would currently be done in the bridge, so that the single module attribute is produced: D147218.

I suppose a solution to issues on both approaches would be to update the 'requires' flags in the OpenMPIRBuilder::Config structure in convertOperation and replace the OpenMPIRBuilder::createRegisterRequires function for a new OpenMPIRBuilder::createOrUpdateRegisterRequires, and make sure that only a single registration function is generated, eventually with the whole set of flags passed to the runtime (each subsequent call would update the 'flags' argument instead of creating the function and registering it as a constructor). I guess it wouldn't be an issue the fact that the config structure wouldn't have the complete set of clauses at all times, since they are supposed (and checked in semantics: D149337) to come before any operations that may be affected by them. But I'm also not sure about how clean this solution would be either.

Let me know what your thoughts are on whether any of these approaches to supporting 'requires' via an operation is preferable to an attribute, or if you have another idea, since any of these changes would result in multiple updates across all related pending patches so I want to make sure we agree on an approach before committing the time.

[skatrak]

Rebase and move OpenMPIRBuilder configuration to ModuleTranslation::getOpenMPBuilder

[kiranchandramohan]

Thanks for the updates. Please see comments inline.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1201–1202

Yes, that would be my preference. And it is fine for these instances of atomic operations to have the memory order attribute. Since these are anyway accepted as optional, there is no change in the operation definition.

I think in general, we should try to keep the translation as dumb and minimal as possible. So we can exclude this from here.

1938–1957

Thanks for the detailed reply and exploring the options.

My question was basically coming from the point of view that convertRequiresAttr is generating some llvm IR instructions. So, we could model these instructions as an operation in the OpenMP dialect. But you make a good point (point copied below).

but if we had an operation instead of an attribute we would have to search for it in the module to extract this, which I'm not sure it would be a cleaner solution. Also there would be no guarantee that no more than a single occurrence of this operation would be present within the module at all times (e.g. if the input is handwritten MLIR).

Can we move this to getOpenMPBuilder? This way if we decide to add an omp.module then we can just replace the use of the interface with this module operation.

There are a few concerns regarding the amendOperation flow,
-> Just like you said about having the operation multiple times, the attribute can also appear multiple times. This can be handled by checking that this attribute is only present in the module.
-> There is no equivalent to amendOperation in the the regular Dialect Conversion flow. If some one is using their own OpenMP libraries downstream and would like to use the Dialect Conversion mechanism, then there will be a problem.
-> The discardable nature of these attributes.

What do you think about this?

[kiranchandramohan]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1201–1202

The other option is to let the Openmpirbuilder handle this through the config. Why is this (the registration fn and the default ordering) done separately now?

[skatrak]

Thank you Kiran for your suggestions. I'd be happy to follow any approach that's considered best for this, so I tried to explain the best I could my thinking about the points you raised and hopefully we can decide on a solution that can inform some of the other related work that's dealing with the same issues, as well.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1201–1202

Yes, that would be my preference. And it is fine for these instances of atomic operations to have the memory order attribute. Since these are anyway accepted as optional, there is no change in the operation definition.

I think in general, we should try to keep the translation as dumb and minimal as possible. So we can exclude this from here.

Sure, it makes sense, I will start working on that. If there are no instances of requires atomic_default_mem_order, should the semantic pass leave atomic operations unchanged or should it explicitly add the memory order clause with the default value?

The other option is to let the Openmpirbuilder handle this through the config. Why is this (the registration fn and the default ordering) done separately now?

It made sense to split the 'requires' clauses into two attributes because there are different requirements for generating the registration function and for honoring the atomic_default_mem_order. The first should be produced if there are device constructs in the compilation unit, even if there are no 'requires' directives, whereas the second is honored as long as it appears in a 'requires' directive. By splitting these into two MLIR attributes it was possible to just add the ones that applied and avoid generating the registration function when it shouldn't be.

With regards to the way in which this patch currently picks up the value of the omp.atomic_default_mem_order, I agree that this could have been moved to the OpenMPIRBuilder and updated the createAtomic... functions to take that into account if an explicit memory order was not given. I only added it where I did because that's where the default was being picked up already, so it required less code changes. I can make this change instead of moving the logic to semantics, but I understand that the other approach is preferred.

1938–1957

Can we move this to getOpenMPBuilder? This way if we decide to add an omp.module then we can just replace the use of the interface with this module operation.

This could be done there, yes. However, it doesn't seem quite right to me lowering OpenMP-specific attributes inside of ModuleTranslation, even though this case would be a relatively small addition. In the case of adding an omp.module later I would assume that the proper spot to lower these attributes would be OpenMPDialectLLVMIRTranslationInterface::convertOperation, so it would have to be moved anyways. It makes sense to set up the OpenMPIRBuilder::Config in getOpenMPBuilder because it's where the OpenMP IR builder is created, so it's configured at the same time. But I'm not sure about doing any lowering there.

There are a few concerns regarding the amendOperation flow,
-> Just like you said about having the operation multiple times, the attribute can also appear multiple times. This can be handled by checking that this attribute is only present in the module.

That's a good point, if we keep this approach I'll add that check.

-> There is no equivalent to amendOperation in the the regular Dialect Conversion flow. If some one is using their own OpenMP libraries downstream and would like to use the Dialect Conversion mechanism, then there will be a problem.

I'm not quite familiar with the details of the regular dialect conversion flow, so I'd be happy if you could point out if I'm missing something here.

The OpenMP dialect currently only has implemented lowering to the LLVM IR dialect via the Dialect Conversion mechanism (mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp), which handles some but not all of the operations. This is currently triggered during flang compilation (flang/lib/Optimizer/CodeGen/CodeGen.cpp), resulting in an MLIR module with mostly LLVM IR dialect operations and possibly some remaining OpenMP dialect operations. This final MLIR representation is then translated to LLVM IR using the ModuleTranslation class, which in turn relies on all these dialect-specific translation interfaces that can define amendOperation.

If I understand it correctly, the Dialect Conversion mechanism on its own would not be enough to take advantage of all current support for OpenMP lowering regardless of whether we relied on dialect attributes or not, since some operations are translated directly to LLVM IR at the end of the MLIR flow. That last part is what's built on top of the OpenMPIRBuilder. So it feels to me like the more general issue really comes down to having the OpenMPIRBuilder, which translates OpenMP operations into LLVM IR instead of into LLVM IR dialect operations, rather than to using dialect attributes.

However, I do see how it would be possible to move logic from the OpenMPIRBuilder to the ConvertOpenMPToLLVMPass, but I'm not sure if we could do the same for OpenMP dialect attributes attached to operations from other dialects. The point I'm trying to make, I guess, is that we want to keep the OpenMPIRBuilder, so keeping OpenMP dialect-related information as attributes wouldn't make much of a difference as to the limitations of the Dialect Conversion mechanism. But again, I'm not that familiar with this, so if I'm wrong I'd be happy to be corrected and understand it better.

-> The discardable nature of these attributes.

According to the MLIR Language Reference, discardable attributes are just attributes that have semantics defined externally to the operation. I haven't been able to find any documentation related to the "discardability" of these attributes, but to me it should mean that the operation doesn't become invalid if it's taken away. So it could be the case that printing in text format some operation could ignore them, hence losing that information if that text is used to continue the compilation process. However, if we make sure that this is not the case for the builtin.module operation, there is no reason for any transformation/optimization/lowering passes outside of the OpenMP dialect to remove it, so I think it should not be a reason to decide against it.

I can see how these kinds of attributes could be lost if, during a pass, the operation they are attached to is replaced by something else without forwarding them or taking them into consideration to impact in some way the results of the process. And it wouldn't make sense to try to make every pass working on those operations aware of these attributes, so it makes sense that they would be discardable in this sense. However, in the case of the builtin.module I feel like this should not be a concern, since that kind of situation doesn't seem likely or even something that should be supported.

[kiranchandramohan]

Thanks @skatrak for the detailed reply. It will be good to discuss this a bit more and once we are all convinced with the approach then we can go fast.

My reply is inline.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1938–1957

Can we move this to getOpenMPBuilder? This way if we decide to add an omp.module then we can just replace the use of the interface with this module operation.

This could be done there, yes. However, it doesn't seem quite right to me lowering OpenMP-specific attributes inside of ModuleTranslation, even though this case would be a relatively small addition. In the case of adding an omp.module later I would assume that the proper spot to lower these attributes would be OpenMPDialectLLVMIRTranslationInterface::convertOperation, so it would have to be moved anyways. It makes sense to set up the OpenMPIRBuilder::Config in getOpenMPBuilder because it's where the OpenMP IR builder is created, so it's configured at the same time. But I'm not sure about doing any lowering there.

The concern here was that even though we are representing this as an attribute it seems to have an IR representation in LLVM IR. Also FWIU, it seems that we are generating a global constructor and not a metadata or a function attribute. The fact that it is a global constructor makes me feel that this should be provided as an option to the initializer/constructor in the OpenMPIRBuilder which can internally generate these.

There is no equivalent to amendOperation in the regular Dialect Conversion flow. If someone is using their own OpenMP libraries downstream and would like to use the Dialect Conversion mechanism, then there will be a problem.

I'm not quite familiar with the details of the regular dialect conversion flow, so I'd be happy if you could point out if I'm missing something here.

The OpenMP dialect currently only has implemented lowering to the LLVM IR dialect via the Dialect Conversion mechanism (mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp), which handles some but not all of the operations. This is currently triggered during flang compilation (flang/lib/Optimizer/CodeGen/CodeGen.cpp), resulting in an MLIR module with mostly LLVM IR dialect operations and possibly some remaining OpenMP dialect operations. This final MLIR representation is then translated to LLVM IR using the ModuleTranslation class, which in turn relies on all these dialect-specific translation interfaces that can define amendOperation.

If I understand it correctly, the Dialect Conversion mechanism on its own would not be enough to take advantage of all current support for OpenMP lowering regardless of whether we relied on dialect attributes or not, since some operations are translated directly to LLVM IR at the end of the MLIR flow. That last part is what's built on top of the OpenMPIRBuilder. So it feels to me like the more general issue really comes down to having the OpenMPIRBuilder, which translates OpenMP operations into LLVM IR instead of into LLVM IR dialect operations, rather than to using dialect attributes.

However, I do see how it would be possible to move logic from the OpenMPIRBuilder to the ConvertOpenMPToLLVMPass, but I'm not sure if we could do the same for OpenMP dialect attributes attached to operations from other dialects. The point I'm trying to make, I guess, is that we want to keep the OpenMPIRBuilder, so keeping OpenMP dialect-related information as attributes wouldn't make much of a difference as to the limitations of the Dialect Conversion mechanism. But again, I'm not that familiar with this, so if I'm wrong I'd be happy to be corrected and understand it better.

This is a general point that I am making here. The OpenMP Dialect sits in MLIR tree and it will have users both in upstream llvm-project as well as downstream. The current method we have chosen to lower the OpenMP dialect is through the OpenMPIRBuilder at the translation phase. This is done so as to share the code with Clang and have a single implementation of OpenMP in llvm-project as far as possible. I am not suggesting a change in this flow.

But what we have to keep in mind is that there will be others who might be lowering the OpenMP dialect differently for e.g. because they might have their own OpenMP runtime. So, in the best case, the design of the dialect should not be dependent on the OpenMPIRBuilder lowering, it should be general enough so that other users can also use it.

Now the problem here is that AFAIS, the Dialect conversions are usually defined for Operations and we do not have a counterpart for amendOperation that takes an Operation and an attribute. So how will a general user lower the OpenMP dialect? The best case here is again that there is an omp.module operation that when it undergoes DialectConversion will do all the required changes. With the current representation that we have chosen, they will have to add a custom Conversion Patter for the Builtin Module where the OpenMP attributes are interpreted. I don't know whether this is possible currently. Would be good to give this a try so that we know there is a way for DialectConversion users.

[skatrak]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1201–1202

This is now implemented in semantics in patch D149337. I'm currently working on updating the rest of patches to stop using the omp.atomic_default_mem_order attribute.

[skatrak]

Remove atomic_default_mem_order handling, as it's now done in semantics.

[skatrak]

Rebase.

[skatrak]

Fix comments.

[skatrak]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1938–1957

The concern here was that even though we are representing this as an attribute it seems to have an IR representation in LLVM IR. Also FWIU, it seems that we are generating a global constructor and not a metadata or a function attribute. The fact that it is a global constructor makes me feel that this should be provided as an option to the initializer/constructor in the OpenMPIRBuilder which can internally generate these.

Yes, in this case we are generating a new function in the IR that's registered as a global constructor, and not metadata/attributes. I see your point, and I think that the approach of moving this code generation to the OpenMPIRBuilder::initialize method makes sense as well. The only considerations for doing this I think would be to first set the IRBuilder configuration before calling the initializer (which makes sense, but it's done the other way around at the moment) and letting the initializer know that it should optionally generate this constructor. The flag to tell whether to generate the constructor would be necessary to ensure that it's only produced if there are device constructs in the compilation unit, but it can also serve temporarily to prevent clang from generating it twice, as it currently does not use the IRBuilder for this.

Now the problem here is that AFAIS, the Dialect conversions are usually defined for Operations and we do not have a counterpart for amendOperation that takes an Operation and an attribute. So how will a general user lower the OpenMP dialect? The best case here is again that there is an omp.module operation that when it undergoes DialectConversion will do all the required changes. With the current representation that we have chosen, they will have to add a custom Conversion Patter for the Builtin Module where the OpenMP attributes are interpreted. I don't know whether this is possible currently. Would be good to give this a try so that we know there is a way for DialectConversion users.

I can look into this, and see if I can make something like that work. But this requirement does make a good case for defining and using an omp.module to hold all these attributes we're working with. I think it's also a similar consideration with function attributes for declare target in D146063.

I think we're back to the first discussion here from a few months ago. We've now seen what using dialect attributes looks like, and what the problems related to that could be, and we have to decide whether to stick with them or to follow an alternative approach. The main downside that I see to defining omp.module and omp.func or similar is the difficulty around being able to rely on the existing infrastructure without breaking optimization/canonicalization/lowering passes. It's probably easy to inadvertently introduce bugs and the extent of the changes to replace builtin.module for omp.module and func.func for omp.func would likely be quite large.

So would you say that keeping the current approach of representing global OpenMP-specific information with discardable attributes is dependent on there being a clean way of lowering them in MLIR via DialectConversion? In your opinion, is this just an issue for this operation because it produces more than just LLVM IR metadata/attributes, or is it also applicable to other simpler flags (e.g. omp.is_device)?

@jsjodin, @agozillon, @domada, do you have any other thoughts about this?

[agozillon]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1938–1957

Not sure it's a good enough case, but that is just my opinion and I'm more than happy to defer the judgement to everyone else, here's my original version of trying to maintain both a builtin.module and an omp.module in Flang for an is_device which passed the test-suite at the time of making: https://github.com/ROCm-Developer-Tools/llvm-project/pull/193/files

The dozens of test changes aren't necessarily required (some are, but not quite at that level), however, all of the MLIR opt related components do not play well with other modules as there is some inherent expectation that it is an builtin.module that's being utilised and if it's not, generate one implicitly. So, you can ignore those (although if we want the most sensible user experience for opt passes I think those change are likely required).

While I'm very sure someone with more knowledge could do a much better job at shrinking the change list and making a more elegant solution, I think you still very likely are going to run into a lot of issues where bugs are created unintentionally by implementers through catering to only one module type and not considering both, as all an omp.module really is, is a superset of builtin.module, but there is no inheritance for operations in MLIR (although, I did try, but I can't remember the mileage I got out of it) as far as I am aware, so you have to make sure things stay appropriately aligned, if Flang only had one module type, it would likely not be a problem, as you could focus on catering to that sole module type without any regard for another (as is done today).

And there is as you said a lot of optimization passes that depend on builtin.modules, a lot of these can be generalized (and I do with a fair number in the pull request), but future passes have to take into consideration the chance of being used with two module types as well.

I'm not sure that's the same case for https://reviews.llvm.org/D146063 with function attributes at least as they're never actually lowered in any specialized way based on the attribute, just filtered out (and that might happen in the semantic phase at least partially based on where the current discussion on the patch goes), it might change if we ever want specialised lowering of device functions though. However, for data on GlobalOps, the attribute does act as an indicator that lowering changes are required, e.g. removal of original Global (certain cases), addition of metadata to the module or new globals in addition to the original.

So you may end up also having to have an omp.GlobalOp and omp.*all that can be made declare target*, this might be an exhaustive list based on the Fortran/C++ programming language + OpenMP spec, but it is perhaps not in terms of MLIR dialects as new operations for other dialects that support a mix of omp are made. Although, perhaps the MLIR way is just that they have to convert their operations to the omp operations that support declare target at some point in their pipeline or maybe we only need to care about what exists in terms of LLVM IR / Dialect.

We could have a seperate omp.declare_target operation, like I originally tried, but lowering functions if we ever decide to do so becomes a bit painful as any declare targets that resides inside of a function (which I think is the required placement in Fortran for a lot of constructs) has to have some kind of deferral mechanism when it's being lowered to LLVM-IR as convertOperation will process it when the function is being lowered (and so not completely lowered yet). This at least was how it was with my original dialect implementation, you could perhaps design omp.declare_target to act as a region container, similar to target.op, but that may create not so easy to read IR.

Anyway, I have droned on a little bit here, so I hope it's somewhat intelligible and useful with not too many silly assumptions on my part although I know there will be some. I don't mind which direction is chosen (I'm sure they'll all have their pros and cons, but I do have a lot of love for the simplicity of the attributes), just that we do chose one!

[jsjodin]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1938–1957

If we look at the message for the commit that added amendOperation it says:

Port the translation of five dialects that define LLVM IR intrinsics
(LLVMAVX512, LLVMArmNeon, LLVMArmSVE, NVVM, ROCDL) to the new dialect
interface-based mechanism. This allows us to remove individual translations
that were created for each of these dialects and just use one common
MLIR-to-LLVM-IR translation that potentially supports all dialects instead,
based on what is registered and including any combination of translatable
dialects. This removal was one of the main goals of the refactoring.

To support the addition of GPU-related metadata, the translation interface is
extended with the `amendOperation` function that allows the interface
implementation to post-process any translated operation with dialect attributes
from the dialect for which the interface is implemented regardless of the
operation's dialect. This is currently applied to "kernel" functions, but can
be used to construct other metadata in dialect-specific ways without
necessarily affecting operations.

If I understand the wording correctly, the second paragraph says that it is okay to add dialect attributes to operations of a different dialect, and process them using amendOperation, regardless
of the operation's dialect. Also it is okay to affect the operation (but not necessarily), so generating code (modifying the module operation) should be okay. So I don't really see a problem using amendOperation if we follow that logic.

One issue is that this directly accesses attributes and will not go through any interfaces that have been defined. FWIW, my personal opinion about interfaces is that they are best used to capture abstract or derived properties that are not tied to a specific dialect so that generic analyzes can be written without knowing the specifics of any dialect, and that dialect attributes are owned by a specific dialect and can be used directly. So using amendOperation should be okay imo.

[kiranchandramohan]

Thanks for the detailed discussion @skatrak @agozillon @jsjodin.

LGTM.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1938–1957

Not sure it's a good enough case, but that is just my opinion and I'm more than happy to defer the judgement to everyone else, here's my original version of trying to maintain both a builtin.module and an omp.module in Flang for an is_device which passed the test-suite at the time of making: https://github.com/ROCm-Developer-Tools/llvm-project/pull/193/files

Thanks for the pointer to this pull-request. As discussed in https://discourse.llvm.org/t/supporting-top-level-ops-other-than-builtin-module/65224, it seems having a non-builtin-module as a top-level Op is a welcome change. It might be worth seeing if some of those changes can make it into upstream MLIR when you get some time.

1938–1957

If we look at the message for the commit that added amendOperation it says:

Port the translation of five dialects that define LLVM IR intrinsics
(LLVMAVX512, LLVMArmNeon, LLVMArmSVE, NVVM, ROCDL) to the new dialect
interface-based mechanism. This allows us to remove individual translations
that were created for each of these dialects and just use one common
MLIR-to-LLVM-IR translation that potentially supports all dialects instead,
based on what is registered and including any combination of translatable
dialects. This removal was one of the main goals of the refactoring.

To support the addition of GPU-related metadata, the translation interface is
extended with the `amendOperation` function that allows the interface
implementation to post-process any translated operation with dialect attributes
from the dialect for which the interface is implemented regardless of the
operation's dialect. This is currently applied to "kernel" functions, but can
be used to construct other metadata in dialect-specific ways without
necessarily affecting operations.

If I understand the wording correctly, the second paragraph says that it is okay to add dialect attributes to operations of a different dialect, and process them using amendOperation, regardless
of the operation's dialect. Also it is okay to affect the operation (but not necessarily), so generating code (modifying the module operation) should be okay. So I don't really see a problem using amendOperation if we follow that logic.

One issue is that this directly accesses attributes and will not go through any interfaces that have been defined. FWIW, my personal opinion about interfaces is that they are best used to capture abstract or derived properties that are not tied to a specific dialect so that generic analyzes can be written without knowing the specifics of any dialect, and that dialect attributes are owned by a specific dialect and can be used directly. So using amendOperation should be okay imo.

The second paragraph, although it says that it can post-process any translated operation, mostly speaks about metadata. We are using it here to generate global variables.
The concern is also that there does not seem to be a similar functionality like amendOperation in DialectConversion unlike convertOperation.

The interface we are using is just a place-holder to capture all the changes required to enable device offloading/target operations handling. Ideally, finally, that should just dissolve into an omp.module operation.

1938–1957

The concern here was that even though we are representing this as an attribute it seems to have an IR representation in LLVM IR. Also FWIU, it seems that we are generating a global constructor and not a metadata or a function attribute. The fact that it is a global constructor makes me feel that this should be provided as an option to the initializer/constructor in the OpenMPIRBuilder which can internally generate these.

Yes, in this case we are generating a new function in the IR that's registered as a global constructor, and not metadata/attributes. I see your point, and I think that the approach of moving this code generation to the OpenMPIRBuilder::initialize method makes sense as well. The only considerations for doing this I think would be to first set the IRBuilder configuration before calling the initializer (which makes sense, but it's done the other way around at the moment) and letting the initializer know that it should optionally generate this constructor. The flag to tell whether to generate the constructor would be necessary to ensure that it's only produced if there are device constructs in the compilation unit, but it can also serve temporarily to prevent clang from generating it twice, as it currently does not use the IRBuilder for this.

Now the problem here is that AFAIS, the Dialect conversions are usually defined for Operations and we do not have a counterpart for amendOperation that takes an Operation and an attribute. So how will a general user lower the OpenMP dialect? The best case here is again that there is an omp.module operation that when it undergoes DialectConversion will do all the required changes. With the current representation that we have chosen, they will have to add a custom Conversion Patter for the Builtin Module where the OpenMP attributes are interpreted. I don't know whether this is possible currently. Would be good to give this a try so that we know there is a way for DialectConversion users.

I can look into this, and see if I can make something like that work. But this requirement does make a good case for defining and using an omp.module to hold all these attributes we're working with. I think it's also a similar consideration with function attributes for declare target in D146063.

I think we're back to the first discussion here from a few months ago. We've now seen what using dialect attributes looks like, and what the problems related to that could be, and we have to decide whether to stick with them or to follow an alternative approach. The main downside that I see to defining omp.module and omp.func or similar is the difficulty around being able to rely on the existing infrastructure without breaking optimization/canonicalization/lowering passes. It's probably easy to inadvertently introduce bugs and the extent of the changes to replace builtin.module for omp.module and func.func for omp.func would likely be quite large.

So would you say that keeping the current approach of representing global OpenMP-specific information with discardable attributes is dependent on there being a clean way of lowering them in MLIR via DialectConversion? In your opinion, is this just an issue for this operation because it produces more than just LLVM IR metadata/attributes, or is it also applicable to other simpler flags (e.g. omp.is_device)?

The concern is primarily that there does not seem to be something similar to amendOperation in DialectConversion. So we are getting into a lowering flow that might not work directly with DialectConversion.

Anyway, at the moment, I do not have an alternative suggestion. So we can go ahead with this.

[skatrak]

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

1938–1957

Hi Kiran, I spent some time trying to prototype something like what you're describing. A DialectConversion-based way to lower dialect attributes attached to builtin modules, or at least my understanding of what that would look like. I managed to implement something, although I suppose there are still many shortcomings to that approach: D150275. At least I think it shows it's doable.

Thank you for all the feedback. I will merge this after all parent patches are approved as well.

[skatrak]

Update and remove OpenMP dialect dependency from the generic LLVM IR translation.

Followed @kiranchandramohan's suggestion in the comments for D147172, since the
current approach prevents the use of OpenMP dialect-specific attributes for
initializing the OpenMPIRBuilder configuration. One of these is defined for
representing 'requires' clauses, which need to be stored in the
OpenMPIRBuilderConfig, so a different approach is necessary. The approach
implemented in this review is based on the amendOperation translation flow.

[skatrak]

Update patch.

[skatrak]

I think the change of approach to initialize the OpenMPIRBuilderConfig through the amendOperation() flow (rather than directly inside of ModuleTranslation::getOpenMPBuilder()) after @kiranchandramohan 's acceptance of the patch requires another review before landing. This patch still depends on a few pending patches so, in case significant changes have to be made to these, I think it's best to leave this review for last.

[kiranchandramohan]

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

1387–1388

This change has to go in another patch reviewed by the general MLIR/LLVM dialect community. Not sure about mentioning too much about OpenMPIRbuilder or the OpenMPDialect here.

[skatrak]

Update to split generic MLIR into its own patch.

[skatrak]

Update patch.

[kiranchandramohan]

LG.

[skatrak]

Rebase.

clang/lib/CodeGen/CGOpenMPRuntime.cpp

 CGOpenMPRuntime::CGOpenMPRuntime(CodeGenModule &CGM)
     : CGM(CGM), OMPBuilder(CGM.getModule()) {
   KmpCriticalNameTy = llvm::ArrayType::get(CGM.Int32Ty, /*NumElements*/ 8);
   llvm::OpenMPIRBuilderConfig Config(
       CGM.getLangOpts().OpenMPIsTargetDevice, isGPU(),
       CGM.getLangOpts().OpenMPOffloadMandatory,
       /*HasRequiresReverseOffload*/ false, /*HasRequiresUnifiedAddress*/ false,
       hasRequiresUnifiedSharedMemory(), /*HasRequiresDynamicAllocators*/ false);
-  OMPBuilder.initialize(CGM.getLangOpts().OpenMPIsTargetDevice
+  OMPBuilder.initialize();
+  OMPBuilder.loadOffloadInfoMetadata(CGM.getLangOpts().OpenMPIsTargetDevice
                                          ? CGM.getLangOpts().OMPHostIRFile
                                          : StringRef{});
   OMPBuilder.setConfig(Config);
 }
 
 void CGOpenMPRuntime::clear() {
   InternalVars.clear();
   // Clean non-target variable declarations possibly used only in debug info.
   for (const auto &Data : EmittedNonTargetVariables) {
     if (!Data.getValue().pointsToAliveValue())

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h

if (Separator.has_value())
       return *Separator;
     if (isGPU())
       return "$";
     return ".";
   }
 
   void setIsTargetDevice(bool Value) { IsTargetDevice = Value; }
   void setIsGPU(bool Value) { IsGPU = Value; }
+  void setOpenMPOffloadMandatory(bool Value) { OpenMPOffloadMandatory = Value; }
   void setFirstSeparator(StringRef FS) { FirstSeparator = FS; }
   void setSeparator(StringRef S) { Separator = S; }
 
   void setHasRequiresReverseOffload(bool Value);
   void setHasRequiresUnifiedAddress(bool Value);
   void setHasRequiresUnifiedSharedMemory(bool Value);
   void setHasRequiresDynamicAllocators(bool Value);
 
public:
   /// Create a new OpenMPIRBuilder operating on the given module \p M. This will
   /// not have an effect on \p M (see initialize)
   OpenMPIRBuilder(Module &M)
       : M(M), Builder(M.getContext()), OffloadInfoManager(this) {}
   ~OpenMPIRBuilder();
 
   /// Initialize the internal state, this will put structures types and
   /// potentially other helpers into the underlying module. Must be called
-  /// before any other method and only once! This internal state includes
-  /// Types used in the OpenMPIRBuilder generated from OMPKinds.def as well
-  /// as loading offload metadata for device from the OpenMP host IR file
-  /// passed in as the HostFilePath argument.
-  /// \param HostFilePath The path to the host IR file, used to load in
-  /// offload metadata for the device, allowing host and device to
-  /// maintain the same metadata mapping.
-  void initialize(StringRef HostFilePath = {});
+  /// before any other method and only once! This internal state includes types
+  /// used in the OpenMPIRBuilder generated from OMPKinds.def.
+  void initialize();
 
   void setConfig(OpenMPIRBuilderConfig C) { Config = C; }
 
   /// Finalize the underlying module, e.g., by outlining regions.
   /// \param Fn                    The function to be finalized. If not used,
   ///                              all functions are finalized.
   void finalize(Function *Fn = nullptr);
 
public:
   /// Loads all the offload entries information from the host IR
   /// metadata. This function is only meant to be used with device code
   /// generation.
   ///
   /// \param M         Module to load Metadata info from. Module passed maybe
   /// loaded from bitcode file, i.e, different from OpenMPIRBuilder::M module.
   void loadOffloadInfoMetadata(Module &M);
 
+  /// Loads all the offload entries information from the host IR
+  /// metadata read from the file passed in as the HostFilePath argument. This
+  /// function is only meant to be used with device code generation.
+  ///
+  /// \param HostFilePath The path to the host IR file,
+  /// used to load in offload metadata for the device, allowing host and device
+  /// to maintain the same metadata mapping.
+  void loadOffloadInfoMetadata(StringRef HostFilePath);
+
   /// Gets (if variable with the given name already exist) or creates
   /// internal global variable with the specified Name. The created variable has
   /// linkage CommonLinkage by default and is initialized by null value.
   /// \param Ty Type of the global variable. If it is exist already the type
   /// must be the same.
   /// \param Name Name of the variable.
   GlobalVariable *getOrCreateInternalVariable(Type *Ty, const StringRef &Name,
                                               unsigned AddressSpace = 0);

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp

 
 Function *OpenMPIRBuilder::getOrCreateRuntimeFunctionPtr(RuntimeFunction FnID) {
   FunctionCallee RTLFn = getOrCreateRuntimeFunction(M, FnID);
   auto *Fn = dyn_cast<llvm::Function>(RTLFn.getCallee());
   assert(Fn && "Failed to create OpenMP runtime function pointer");
   return Fn;
 }
 
-void OpenMPIRBuilder::initialize(StringRef HostFilePath) {
-  initializeTypes(M);
-
-  if (HostFilePath.empty())
-    return;
-
-  auto Buf = MemoryBuffer::getFile(HostFilePath);
-  if (std::error_code Err = Buf.getError()) {
-    report_fatal_error(("error opening host file from host file path inside of "
-                        "OpenMPIRBuilder: " +
-                        Err.message())
-                           .c_str());
-  }
-
-  LLVMContext Ctx;
-  auto M = expectedToErrorOrAndEmitErrors(
-      Ctx, parseBitcodeFile(Buf.get()->getMemBufferRef(), Ctx));
-  if (std::error_code Err = M.getError()) {
-    report_fatal_error(
-        ("error parsing host file inside of OpenMPIRBuilder: " + Err.message())
-            .c_str());
-  }
-
-  loadOffloadInfoMetadata(*M.get());
-}
+void OpenMPIRBuilder::initialize() { initializeTypes(M); }
 
 void OpenMPIRBuilder::finalize(Function *Fn) {
   SmallPtrSet<BasicBlock *, 32> ParallelRegionBlockSet;
   SmallVector<BasicBlock *, 32> Blocks;
   SmallVector<OutlineInfo, 16> DeferredOutlines;
   for (OutlineInfo &OI : OutlineInfos) {
     // Skip functions that have not finalized yet; may happen with nested
     // function generation.
case OffloadEntriesInfoManager::OffloadEntryInfo::
           static_cast<OffloadEntriesInfoManager::OMPTargetGlobalVarEntryKind>(
               /*Flags=*/GetMDInt(2)),
           /*Order=*/GetMDInt(3));
       break;
     }
   }
 }
 
+void OpenMPIRBuilder::loadOffloadInfoMetadata(StringRef HostFilePath) {
+  if (HostFilePath.empty())
+    return;
+
+  auto Buf = MemoryBuffer::getFile(HostFilePath);
+  if (std::error_code Err = Buf.getError()) {
+    report_fatal_error(("error opening host file from host file path inside of "
+                        "OpenMPIRBuilder: " +
+                        Err.message())
+                           .c_str());
+  }
+
+  LLVMContext Ctx;
+  auto M = expectedToErrorOrAndEmitErrors(
+      Ctx, parseBitcodeFile(Buf.get()->getMemBufferRef(), Ctx));
+  if (std::error_code Err = M.getError()) {
+    report_fatal_error(
+        ("error parsing host file inside of OpenMPIRBuilder: " + Err.message())
+            .c_str());
+  }
+
+  loadOffloadInfoMetadata(*M.get());
+}
+
 Function *OpenMPIRBuilder::createRegisterRequires(StringRef Name) {
   // Skip the creation of the registration function if this is device codegen
   if (Config.isTargetDevice())
     return nullptr;
 
   Builder.ClearInsertionPoint();
 
   // Create registration function prototype

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp

 //===----------------------------------------------------------------------===//
 #include "mlir/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.h"
 #include "mlir/Dialect/LLVMIR/LLVMDialect.h"
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
 #include "mlir/Dialect/OpenMP/OpenMPInterfaces.h"
 #include "mlir/IR/IRMapping.h"
 #include "mlir/IR/Operation.h"
 #include "mlir/Support/LLVM.h"
+#include "mlir/Support/LogicalResult.h"
 #include "mlir/Target/LLVMIR/Dialect/OpenMPCommon.h"
 #include "mlir/Target/LLVMIR/ModuleTranslation.h"
 #include "mlir/Transforms/RegionUtils.h"
 
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/TypeSwitch.h"
 #include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include "llvm/Frontend/OpenMP/OMPIRBuilder.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/IRBuilder.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Transforms/Utils/ModuleUtils.h"
+#include <optional>
 
 using namespace mlir;
 
 namespace {
 static llvm::omp::ScheduleKind
 convertToScheduleKind(std::optional<omp::ClauseScheduleKind> schedKind) {
   if (!schedKind.has_value())
     return llvm::omp::OMP_SCHEDULE_Default;
ompBuilder->applySimd(
                        : nullptr,
       llvm::omp::OrderKind::OMP_ORDER_unknown, simdlen, safelen);
 
   builder.restoreIP(afterIP);
   return success();
 }
 
 /// Convert an Atomic Ordering attribute to llvm::AtomicOrdering.
-llvm::AtomicOrdering
+static llvm::AtomicOrdering
 convertAtomicOrdering(std::optional<omp::ClauseMemoryOrderKind> ao) {
   if (!ao)
     return llvm::AtomicOrdering::Monotonic; // Default Memory Ordering
 
   switch (*ao) {
   case omp::ClauseMemoryOrderKind::Seq_cst:
     return llvm::AtomicOrdering::SequentiallyConsistent;
   case omp::ClauseMemoryOrderKind::Acq_rel:

 convertOmpAtomicRead(Operation &opInst, llvm::IRBuilderBase &builder,
                      LLVM::ModuleTranslation &moduleTranslation) {
 
   auto readOp = cast<omp::AtomicReadOp>(opInst);
   llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
 
   llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
 
   llvm::AtomicOrdering AO = convertAtomicOrdering(readOp.getMemoryOrderVal());
   llvm::Value *x = moduleTranslation.lookupValue(readOp.getX());
   llvm::Value *v = moduleTranslation.lookupValue(readOp.getV());
 
   llvm::Type *elementType =
       moduleTranslation.convertType(readOp.getElementType());
 
   llvm::OpenMPIRBuilder::AtomicOpValue V = {v, elementType, false, false};
   llvm::OpenMPIRBuilder::AtomicOpValue X = {x, elementType, false, false};
   builder.restoreIP(ompBuilder->createAtomicRead(ompLoc, X, V, AO));
if (auto offloadMod = dyn_cast<omp::OffloadModuleInterface>(
         llvmFunc->dropAllReferences();
         llvmFunc->eraseFromParent();
       }
     }
   }
   return success();
 }
 
+/// Converts the module-level set of OpenMP requires clauses into LLVM IR using
+/// OpenMPIRBuilder.
+static LogicalResult
+convertRequiresAttr(Operation &op, omp::ClauseRequiresAttr requiresAttr,
+                    LLVM::ModuleTranslation &moduleTranslation) {
+  auto *ompBuilder = moduleTranslation.getOpenMPBuilder();
+
+  // No need to read requiresAttr here, because it has already been done in
+  // translateModuleToLLVMIR(). There, flags are stored in the
+  // OpenMPIRBuilderConfig object, available to the OpenMPIRBuilder.
+  auto *regFn =
+      ompBuilder->createRegisterRequires(ompBuilder->createPlatformSpecificName(
+          {"omp_offloading", "requires_reg"}));
+
+  // Add registration function as global constructor
+  if (regFn)
+    llvm::appendToGlobalCtors(ompBuilder->M, regFn, /* Priority = */ 0);
+
+  return success();
+}
+
 namespace {
 
 /// Implementation of the dialect interface that converts operations belonging
 /// to the OpenMP dialect to LLVM IR.
 class OpenMPDialectLLVMIRTranslationInterface
     : public LLVMTranslationDialectInterface {
 public:
   using LLVMTranslationDialectInterface::LLVMTranslationDialectInterface;
 
   /// Translates the given operation to LLVM IR using the provided IR builder
   /// and saving the state in `moduleTranslation`.
   LogicalResult
   convertOperation(Operation *op, llvm::IRBuilderBase &builder,
                    LLVM::ModuleTranslation &moduleTranslation) const final;
 
+  /// Given an OpenMP MLIR attribute, create the corresponding LLVM-IR, runtime
+  /// calls, or operation amendments
   LogicalResult
   amendOperation(Operation *op, NamedAttribute attribute,
                  LLVM::ModuleTranslation &moduleTranslation) const final;
 };
 
 } // namespace
 
-/// Given an OpenMP MLIR attribute, create the corresponding LLVM-IR, runtime
-/// calls, or operation amendments
 LogicalResult OpenMPDialectLLVMIRTranslationInterface::amendOperation(
     Operation *op, NamedAttribute attribute,
     LLVM::ModuleTranslation &moduleTranslation) const {
-  return llvm::TypeSwitch<Attribute, LogicalResult>(attribute.getValue())
-      .Case([&](mlir::omp::FlagsAttr rtlAttr) {
+  return llvm::StringSwitch<llvm::function_ref<LogicalResult(Attribute)>>(
+             attribute.getName())
+      .Case("omp.is_target_device",
+            [&](Attribute attr) {
+              if (auto deviceAttr = attr.dyn_cast<BoolAttr>()) {
+                llvm::OpenMPIRBuilderConfig &config =
+                    moduleTranslation.getOpenMPBuilder()->Config;
+                config.setIsTargetDevice(deviceAttr.getValue());
+                return success();
+              }
+              return failure();
+            })
+      .Case("omp.is_gpu",
+            [&](Attribute attr) {
+              if (auto gpuAttr = attr.dyn_cast<BoolAttr>()) {
+                llvm::OpenMPIRBuilderConfig &config =
+                    moduleTranslation.getOpenMPBuilder()->Config;
+                config.setIsGPU(gpuAttr.getValue());
+                return success();
+              }
+              return failure();
+            })
+      .Case("omp.host_ir_filepath",
+            [&](Attribute attr) {
+              if (auto filepathAttr = attr.dyn_cast<StringAttr>()) {
+                llvm::OpenMPIRBuilder *ompBuilder =
+                    moduleTranslation.getOpenMPBuilder();
+                ompBuilder->loadOffloadInfoMetadata(filepathAttr.getValue());
+                return success();
+              }
+              return failure();
+            })
+      .Case("omp.flags",
+            [&](Attribute attr) {
+              if (auto rtlAttr = attr.dyn_cast<omp::FlagsAttr>())
                 return convertFlagsAttr(op, rtlAttr, moduleTranslation);
+              return failure();
             })
-      .Case([&](mlir::omp::VersionAttr versionAttr) {
+      .Case("omp.version",
+            [&](Attribute attr) {
+              if (auto versionAttr = attr.dyn_cast<omp::VersionAttr>()) {
                 llvm::OpenMPIRBuilder *ompBuilder =
                     moduleTranslation.getOpenMPBuilder();
                 ompBuilder->M.addModuleFlag(llvm::Module::Max, "openmp",
                                             versionAttr.getVersion());
                 return success();
+              }
+              return failure();
             })
-      .Case([&](mlir::omp::DeclareTargetAttr declareTargetAttr) {
+      .Case("omp.declare_target",
+            [&](Attribute attr) {
+              if (auto declareTargetAttr =
+                      attr.dyn_cast<omp::DeclareTargetAttr>())
                 return convertDeclareTargetAttr(op, declareTargetAttr,
                                                 moduleTranslation);
+              return failure();
             })
-      .Default([&](Attribute attr) {
-        // fall through for omp attributes that do not require lowering and/or
-        // have no concrete definition and thus no type to define a case on
+      .Case(
+          "omp.requires",
+          [&](Attribute attr) {
+            if (auto requiresAttr = attr.dyn_cast<omp::ClauseRequiresAttr>()) {
+              using Requires = omp::ClauseRequires;
+              Requires flags = requiresAttr.getValue();
+              llvm::OpenMPIRBuilderConfig &config =
+                  moduleTranslation.getOpenMPBuilder()->Config;
+              config.setHasRequiresReverseOffload(
+                  bitEnumContainsAll(flags, Requires::reverse_offload));
+              config.setHasRequiresUnifiedAddress(
+                  bitEnumContainsAll(flags, Requires::unified_address));
+              config.setHasRequiresUnifiedSharedMemory(
+                  bitEnumContainsAll(flags, Requires::unified_shared_memory));
+              config.setHasRequiresDynamicAllocators(
+                  bitEnumContainsAll(flags, Requires::dynamic_allocators));
+              return convertRequiresAttr(*op, requiresAttr, moduleTranslation);
+            }
+            return failure();
+          })
+      .Default([](Attribute) {
+        // Fall through for omp attributes that do not require lowering.
         return success();
-      });
+      })(attribute.getValue());
 
   return failure();
 }
 
 /// Given an OpenMP MLIR operation, create the corresponding LLVM IR
 /// (including OpenMP runtime calls).
 LogicalResult OpenMPDialectLLVMIRTranslationInterface::convertOperation(
     Operation *op, llvm::IRBuilderBase &builder,

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp

SmallVector<llvm::Value *> ModuleTranslation::lookupValues(ValueRange values) {
   for (Value v : values)
     remapped.push_back(lookupValue(v));
   return remapped;
 }
 
 llvm::OpenMPIRBuilder *ModuleTranslation::getOpenMPBuilder() {
   if (!ompBuilder) {
     ompBuilder = std::make_unique<llvm::OpenMPIRBuilder>(*llvmModule);
+    ompBuilder->initialize();
 
-    bool isTargetDevice = false, isGPU = false;
-    llvm::StringRef hostIRFilePath = "";
-
-    if (auto deviceAttr =
-            mlirModule->getAttrOfType<mlir::BoolAttr>("omp.is_target_device"))
-      isTargetDevice = deviceAttr.getValue();
-
-    if (auto gpuAttr = mlirModule->getAttrOfType<mlir::BoolAttr>("omp.is_gpu"))
-      isGPU = gpuAttr.getValue();
-
-    if (auto filepathAttr =
-            mlirModule->getAttrOfType<mlir::StringAttr>("omp.host_ir_filepath"))
-      hostIRFilePath = filepathAttr.getValue();
-
-    ompBuilder->initialize(hostIRFilePath);
-
-    // TODO: set the flags when available
-    llvm::OpenMPIRBuilderConfig config(
-        isTargetDevice, isGPU,
+    // Flags represented as top-level OpenMP dialect attributes are set in
+    // `OpenMPDialectLLVMIRTranslationInterface::amendOperation()`. Here we set
+    // the default configuration.
+    ompBuilder->setConfig(llvm::OpenMPIRBuilderConfig(
+        /* IsTargetDevice = */ false, /* IsGPU = */ false,
         /* OpenMPOffloadMandatory = */ false,
         /* HasRequiresReverseOffload = */ false,
         /* HasRequiresUnifiedAddress = */ false,
         /* HasRequiresUnifiedSharedMemory = */ false,
-        /* HasRequiresDynamicAllocators = */ false);
-    ompBuilder->setConfig(config);
+        /* HasRequiresDynamicAllocators = */ false));
   }
   return ompBuilder.get();
 }
 
 llvm::DILocation *ModuleTranslation::translateLoc(Location loc,
                                                   llvm::DILocalScope *scope) {
   return debugTranslation->translateLoc(loc, scope);
 }
mlir::translateModuleToLLVMIR(Operation *module, llvm::LLVMContext &llvmContext,
   if (!llvmModule)
     return nullptr;
 
   LLVM::ensureDistinctSuccessors(module);
 
   ModuleTranslation translator(module, std::move(llvmModule));
   llvm::IRBuilder<> llvmBuilder(llvmContext);
 
   // Convert module before functions and operations inside, so dialect
   // attributes can be used to change dialect-specific global configurations via
   // `amendOperation()`. These configurations can then influence the translation
   // of operations afterwards.
   if (failed(translator.convertOperation(*module, llvmBuilder)))
     return nullptr;
 
   if (failed(translator.convertComdats()))
     return nullptr;
   if (failed(translator.convertFunctionSignatures()))
     return nullptr;
   if (failed(translator.convertGlobals()))
     return nullptr;
   if (failed(translator.createTBAAMetadata()))
     return nullptr;
 
   // Convert other top-level operations if possible.
   for (Operation &o : getModuleBody(module).getOperations()) {
     if (!isa<LLVM::LLVMFuncOp, LLVM::GlobalOp, LLVM::GlobalCtorsOp,
              LLVM::GlobalDtorsOp, LLVM::ComdatOp>(&o) &&
         !o.hasTrait<OpTrait::IsTerminator>() &&
         failed(translator.convertOperation(o, llvmBuilder))) {
       return nullptr;
     }
   }
 

mlir/test/Target/LLVMIR/openmp-llvm.mlir

 // CHECK:         call void @__kmpc_omp_task_complete_if0(ptr @{{.+}}, i32 %[[omp_global_thread_num]], ptr %[[task_data]])
 // CHECK:         br label %[[if_else_exit]]
 // CHECK:       [[if_else_exit]]:
 // CHECK:         br label %[[task_exit:[^,]+]]
 // CHECK:       [[task_exit]]:
 // CHECK:         call void @foo_after()
 // CHECK:         ret void
 
+// -----
+
+// Check that OpenMP requires flags are registered by a global constructor.
+// CHECK: @llvm.global_ctors = appending global [1 x { i32, ptr, ptr }]
+// CHECK-SAME: [{ i32, ptr, ptr } { i32 0, ptr @[[REG_FN:.*]], ptr null }]
+// CHECK: define {{.*}} @[[REG_FN]]({{.*}})
+// CHECK-NOT: }
+// CHECK:   call void @__tgt_register_requires(i64 10)
+module attributes {omp.requires = #omp<clause_requires reverse_offload|unified_shared_memory>} {}