TIFitis marked an inline comment as done.
TIFitis 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();
----------------
kiranchandramohan wrote:
> 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.
>
>
>
>
I've created a discourse topic for this here.
https://discourse.llvm.org/t/rfc-adding-new-util-file-to-mlir-dialect-translation/68221?u=tifitis
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142914/new/
https://reviews.llvm.org/D142914
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
