[PATCH] D99024: [OpenMP][OMPIRBuilder] Adding support for `omp atomic`

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX added subscribers: AMDChirag, SouraVX.
SouraVX added a comment.

Thanks! @fghanim for the patch :)

I noticed following piece of code getting used in `createAtomicRead`,  `createAtomicWrite`, `createAtomicCapture`, `createAtomicUpdate`.

  if (!updateToLocation(Loc))
      return Loc.IP;
  Type *XTy = X.Var->getType();
    assert(XTy->isPointerTy() && "OMP Atomic expects a pointer to target memory");
    Type *XElemTy = XTy->getPointerElementType();
    assert((XElemTy->isFloatingPointTy() || XElemTy->isIntegerTy() ||
            XElemTy->isPointerTy()) &&
           "OMP atomic read expected a scalar type");

Was wondering if this could be simplified or hoisted out in a tiny innocuous helper function ?

Also right now we have these `4` public interfaces with external front-end's. Can we simplify this into `2`  public interfaces as `createAtomicReadOrWrite` and `createAtomicCaptureOrUpdate` ?
The rationale behind this proposition is that, these functions share a lot common code(at expense of 1 or 2 extra argument needed to handle the other case).
Can we do this ? Or doing such simplification will render the code more unreadable/complicated ?
Please Let me know WDYT ?
+ @AMDChirag



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:831
+  /// \param AllocIP	  Instruction to create AllocaInst before.
+  /// \param X			    The target atomic pointer to be updated
+  /// \param Expr		    The value to update X with.
----------------
Can we use a better/descriptive name here ? Maybe `TargetAtomicPtr` ?
Other references should also benefit from descriptive name.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:934
+  /// \param X			    The target atomic pointer to be updated
+  /// \param V			    Memory address where to store captured value
+  /// \param Expr		    The value to update X with.
----------------
This could also benefit from descriptive naming ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99024