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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
