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

[Valentin Clement via Phabricator via llvm-commits]

clementval added inline comments.


================
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:
> > > > > > > > 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914