[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

Mon Feb 6 05:00:44 PST 2023

kiranchandramohan added a comment.

Please add more description to the summary regarding the scope of this patch. If it is only able to lower specific llvm-dialect types, please call out that. Please also explain the split in work between mlir::Translation and OpenMPIRbuilder. You can also consider splitting this into two patches. One that just adds the changes to OpenMPIRBuilder and the child version of the patch adding the translation in MLIR.

================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder &builder,
+                               StringRef name, uint32_t &strLen) {
+  if (auto fileLoc = loc.dyn_cast<FileLineColLoc>()) {
+    StringRef fileName = fileLoc.getFilename();
+    unsigned lineNo = fileLoc.getLine();
----------------
TIFitis wrote:
> clementval wrote:
> > TIFitis wrote:
> > > clementval wrote:
> > > > TIFitis wrote:
> > > > > clementval wrote:
> > > > > > TIFitis wrote:
> > > > > > > clementval wrote:
> > > > > > > > TIFitis wrote:
> > > > > > > > > clementval wrote:
> > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > clementval wrote:
> > > > > > > > > > > > Instead of copy pasting this from `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` can you extract it and put it in a common shared file so bith translation can use the same code without duplication?
> > > > > > > > > > > @raghavendhra put up a patch some time back and he faced some issues. It might be good to check with him or may be he can comment here.
> > > > > > > > > > > https://reviews.llvm.org/D127037
> > > > > > > > > > > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> > > > > > > > > > Just moving the three functions should be trivial. I'm not talking about the processMapOperand.
> > > > > > > > > I've moved `getSizeInBytes`.
> > > > > > > > > 
> > > > > > > > > The other two functions make use of `mlir::Location` and thus can't be moved trivially.
> > > > > > > > > 
> > > > > > > > > I can still try to move them by individually passing the elements of `mlir::Location` but that might not be ideal. Is that what you'd like?
> > > > > > > > What about a new header file in `mlir/include/mlir/Target/LLVMIR/Dialect/**` shared by `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` and `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`. That should be doable. 
> > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` and `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp` already have access to the common `mlir::Location` type.
> > > > > > > 
> > > > > > > Problem is that `OMPIRBuilder.cpp` is the only common file between them  where I can move the two functions to. Currently there are no `mlir/**` include files in `OMPIRBuilder.cpp` and it seems to me like a strict design choice to have it that way.
> > > > > > The functions can be header only. Why do you need to put them in the OMPIRBuilder.cpp? I think it is better than duplicate the exact same code over. 
> > > > > Sorry, I misunderstood you earlier.
> > > > > I've added a new header file `mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h`, this is my first attempt at adding a new header file, please let me know if you find any issues.
> > > > Thanks! That's what I had in mind. We might want to check with MLIR folks if `mlir::utils` is suited for that. I don't mind if it is `mlir::omp::builder` or smth similar since it is related to the OMPIRBuilder.
> > > Since the utils file is common to all the dialects I kept it as `mlir::utils`.
> > > 
> > > How do I get the opinion from people working in MLIR on this, can you suggest some reviewers whom I can add?
> > It's only valid for translation to the `llvmir` dialect so that why `mlir::utils` seems to generic to me. 
> > 
> > Maybe @ftynse has some thoughts on this. 
> I agree with you on that, would perhaps renaming it to something like `mlir::dialect-utils` be a better option?
You can post in MLIR discourse MLIR section (https://discourse.llvm.org/c/mlir/31) to get an opinion.

open-directive-utils , ompbuilder-utils are other options. Simialr names could be considered for the file name as well.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914