[Openmp-commits] [PATCH] D50522: [OpenMP][libomptarget] Bringing up to spec with respect to OMP_TARGET_OFFLOAD env var

[Jonas Hahnfeld via Phabricator via Openmp-commits]

Hahnfeld added inline comments.


================
Comment at: libomptarget/src/interface.cpp:28-61
+// mutex
+std::mutex LibomptargetPrintMtx;
+
+
+////////////////////////////////////////////////////////////////////////////////
+/// support for print and assert
+
----------------
AlexEichenberger wrote:
> Hahnfeld wrote:
> > AlexEichenberger wrote:
> > > Hahnfeld wrote:
> > > > This seems overly complex where we now only need a single `fprintf` (which is thread-safe) inside an `if`. Do you have upcoming patches that will use these functions?
> > > Yes, I intend this to be used for all output feedback to users. Ideally, I would find a way to share the infrastructure in kmpc, but for the moment this should do it. Note that the lock is only acquired prior to aborting, so overheads are not an issue.
> > In that case I think the functions should be moved to `omptarget.cpp` (or a completely new file?) because they'll be used by more than just the interface?
> > 
> > Inverting `cond` seems weird, could you explain that choice? If that's to model `assert` I think the function should be renamed to `AssertOrFatalMassage`. To me the current function name implies `if (cond) FatalMessage(...)`, but that may be just me.
> I agree.. maybe private.cpp?
> 
> I named them AssertWithFatalMessage, but then you mentioned that asserts are turned off in the release mode, so I though that would be confusing too.
> 
> But if you prefer Assert, that was my first choice too
Another option would be to remove `FatalMessageWithCond` and put in the `if` statement directly. That would a) be clear, b) avoid choosing a name and whether to invert `cond` or not, and c) deduplicate code and make `FatalMessage` used (which is not at the moment).

For one single function I don't think we need to start a new file right now. IMO that could go to `omptarget.cpp`.


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D50522