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:
> > > > 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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142914/new/
https://reviews.llvm.org/D142914
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
