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

[Fady Ghanim via Phabricator via llvm-commits]

fghanim marked 5 inline comments as done.
fghanim added a comment.

In D99024#2667320 <https://reviews.llvm.org/D99024#2667320>, @SouraVX wrote:

> Thanks! @fghanim for the patch :)

Sure! :)

> 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 ?

The `return` returns from `createOMPAtomicXXX` calls, and each of `XTy` and `XElemTy` are used inside the function. Also, We are checking different kinds of errors between different atomic clauses, and issuing different error assert messages based on the error, .

> 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 ?

I don't think we are simplifying things by doing that, because it will cause unnecessary confusion. two parts:

- Regarding atomic read and atomic write, there is nothing in common between the two (other than the initial checks). While at first glance, they may look similar, they are not.
- Regarding atomic update and atomic capture, I already implemented the parts that are common between the two separately into `emitAtomicUpdate`

In general, it is better for someone handling an atomic read, to just call `createOMPAtomicRead`, than call something that is also doing writes. more intuitive.

> + @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.
----------------
SouraVX wrote:
> Can we use a better/descriptive name here ? Maybe `TargetAtomicPtr` ?
> Other references should also benefit from descriptive name.
These names are picked to match the naming used in the OMP standard documents for atomic:
https://www.openmp.org/spec-html/5.0/openmpsu95.html


================
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.
----------------
SouraVX wrote:
> This could also benefit from descriptive naming ?
These names are picked to match the naming used in the OMP standard documents for atomic:
https://www.openmp.org/spec-html/5.0/openmpsu95.html


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2161-2164
+    if (Builder.GetInsertPoint() == ContBB->end())
+      llvm::errs() << "end block\n";
+    else
+      ContBB->dump();
----------------
Meinersbur wrote:
> Debugging leftover?
Yes. Thanks for catching that. removed


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1922
 
+bool OpenMPIRBuilder::CheckAndEmitFlushAfterAtomic(
+    const LocationDescription &Loc, llvm::AtomicOrdering AO, AtomicKind AK) {
----------------
Meinersbur wrote:
> Consider making such helpers static functions instead of priviate methods, so they don't need to appear in the public header file.
I call emitFlush() which is private


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2082-2088
+  case AtomicRMWInst::Xchg:
+  case AtomicRMWInst::FAdd:
+  case AtomicRMWInst::FSub:
+  case AtomicRMWInst::BAD_BINOP:
+  case AtomicRMWInst::Max:
+  case AtomicRMWInst::Min:
+  default:
----------------
Meinersbur wrote:
> fghanim wrote:
> > Meinersbur wrote:
> > > With a default case, no need to enumerate all cases.
> > 
> > I intentionally listed all cases. If you want, i'll remove `default` which I added to future proof the code.
> With a default case, the compiler does not warn about unhandled cases anymore. If you want both, you could do:
> 
> ```
>   [...]
>   case AtomicRMWInst::Min:
>     llvm_unreachable("Unsupported atomic update operation");
>   }
>   llvm_unreacheable("Unhandled atomic update operation");
> ```
> Personally I think it just adds clutter lines. In case of some parts of clang's OpenMP implementation it fills entire screens of unsupported cases and there is no chance I miss handling the case during testing due to the llvm_unreachable.
> 
> Currently, `return nullptr;` is unreachable code.
The issue here is that, these cases are either not supported in omp (min,max,..), or should be handled by the cmpExch (fadd, fsub,exch). I worry if i don't explicitly list them as unreachable somebody might try to implement them. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99024