[RFC] Adding new Util file to MLIR Dialect Translation

MLIR
1

Hi,

In this patch here: ⚙ D142914 [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives. I am trying to add OMPIRBuilder support for the Target Data directives for lowering to LLVM IR from OpenMP Dialect.

In doing so, we have encountered a couple functions that are exactly the same between OpenACC and OpenMP. To prevent code duplication, the code reviewers in the patch suggested to add a new common header file where we can keep these functions.

We are looking for comments from the community regarding what would be a good approach for this.
Currently a new file Utils.h has been added to the directory mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h and in that file a new namespace mlir::utils has been introduced to interface to these functions.

Looking for suggestions on the directory-placement of the file, name of the file as well as the name of the new namespace.

Some suggestions from the reviewers for the namespace are:

  • mlir::open-directive-utils
  • mlir::ompbuilder-utils
  • mlir::dialect-utils

Cheers,
Akash

2 Likes
2

I will vote for namespace mlir::ompbuilder-utils

3

The proposed location is fine as long as it doesn’t make other translations to depend on the OpenMP Frontend in LLVM. Otherwise, I would suggest something like mlir/Target/LLVMIR/Dialect/OpenMPCommon that clearly indicate that it is related to OpenMP. We already have a precedent with conversions.

There’s no need to introduce a namespace for just two functions. Leaving them in mlir or mlir::LLVM would be just fine. If you really want, put them in mlir::LLVM::utils. Also, I’m quite strongly against namespace names with dashes.