[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

Fri Mar 31 06:11:44 PDT 2023

agozillon added a comment.

In D145264#4236173 <https://reviews.llvm.org/D145264#4236173>, @kiranchandramohan wrote:

> Please split this patch into three:
>
> 1. Code changes and testing for the driver and the FIR+OpenMP dialect generated.
> 2. Code changes and test for FIR+OpenMP to LLVM+OpenMP dialect.
> 3. Code changes and testing for the translation from LLVM + OpenMP dialect to LLVM IR. Code in OpenMPToLLVMIRTranslation.cpp should be tested with `mlir-translate`.

More than happy to do all of the above, however, there is no code for step 2 in this case as it undergoes no rewriting or transformation when going from FIR+OpenMP -> LLVM+OpenMP. However, I am happy to make a test to assure it is retained during the conversion, but I believe it would just be a test in the patch, would it be okay to merge patch 2 and 3 from your comment? Meaning there would be two patches, the driver additions and the attribute generation from the flags. And then the lowering of the attribute to LLVM-IR (with additional mlir-translate tests for LLVM+OpenMP -> LLVM-IR and FIR+OpenMP -> LLVM+OpenMP).

> In D145264#4233358 <https://reviews.llvm.org/D145264#4233358>, @agozillon wrote:
>
>> Unfortunately I do not believe an mlir-translate test that tests if the OffloadModuleInterface is accessible when directly utilizing mlir-translate is possible for this patch... I forgot I removed the is device check as it is already done at the initial creation of the attribute. However, I do have a future patch that it will be utilised in and when @jsjodin's initial TargetOp work is in I can likely create a test around that functionality.
>
> You may delay the translation code till then.

Thank you.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145264/new/

https://reviews.llvm.org/D145264