[PATCH] D119657: [OpenMP][mlir] Lowering for omp.atomic.update

[Shraiysh via Phabricator via llvm-commits]

shraiysh requested review of this revision.
shraiysh added a comment.

Requesting review again, because added translation for multiple operations in update region.



================
Comment at: mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:992
+                       LLVM::ModuleTranslation &moduleTranslation) {
+  // Boilerplate.
+  llvm::OpenMPIRBuilder *ompBuilder = moduleTranslation.getOpenMPBuilder();
----------------
ftynse wrote:
> shraiysh wrote:
> > ftynse wrote:
> > > This comment is not helpful, we can see it is boilerplate. I would rather have most of it factored out into a function like `getOpenMPLoc(IRBuilderBase &)`.
> > For the function `getOpenMPLoc`, we would need `opInst` and `moduleTranslation` too. Plus, `llvm::OpenMPIRBuilder::LocationDescription` doesn't have a copy constructor, so that would also have to be passed. Also, the `ompBuilder` is used later in this function, so that cannot be pushed into the function. At this point, a function like the following doesn't feel like it is worth it. I have removed the comment as suggested. Let me know if you have any suggestions for an alternative.
> > ```getOpenMPLoc(Operation &opInst, llvm::IRBuilderBase &builder, LLVM::ModuleTranslation moduleTranslation, llvm::OpenMPIRBuilder::LocationDescription &ompLoc)``` 
> I don't have a problem with multi-argument signatures. `ModuleTranslation` needs to be passed by reference as well. Can't we just make LocationDescription copy-constructible (or movable)?
Now the constructor directly accepts builder, so this is not needed. Thanks for the discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119657