[Openmp-commits] [PATCH] D86804: [OpenMP] Consolidate error handling and debug messages in Libomptarget

[Ye Luo via Phabricator via Openmp-commits]

ye-luo added a comment.

In D86804#2248214 <https://reviews.llvm.org/D86804#2248214>, @jdoerfert wrote:

> In D86804#2248201 <https://reviews.llvm.org/D86804#2248201>, @ye-luo wrote:
>
>> In D86804#2247785 <https://reviews.llvm.org/D86804#2247785>, @jdoerfert wrote:
>>
>>> In D86804#2247697 <https://reviews.llvm.org/D86804#2247697>, @ye-luo wrote:
>>>
>>>> It seems that functions are marked static so they should be OK. However, including the whole Debug.h in a plugin cpp makes it feel OK to use any function/macro from the header file. But actually only part of the macros are for the plugin. some  are only for the libomptarget.
>>>
>>> I'm not sure we want to make a distinction, the point is to move to a unified debug/message model. You can choose not only the level of information but also the kind of output (text, json, ...). The messages will then be tied to the webpage via enums, that allows all plugins to emit the same message for the same thing with the same link to more information. There will certainly things that are only used in libomptarget or the plugins, but I don't see how that is any worse than duplicating the parts that are used by both.
>>
>> I didn't mean to duplicate anything. Instead you need multiple header files. One for common parts, one for libomptarget and one for plugins. The latter two both include the first one. Later you expand OFFLOAD_XXX signals, they can be added to the common file. The return signal is generated by the plugins and captured by the libomptarget. Some users may want to see only the messages captured universally by libomptarget. Some users still want to see the native error message. So the libomptarget and plugin side error handling still needs to be separated.
>
> I fail to see why this machinery is necessary to emit only messages from one place and not the other. I am not against a hierarchy of headers per se, but right now, and maybe also later, there seems to be little point.
> I mean, we need to introduce a new env variable, actually two, that allow separate control. Once we have that we can argue about separation. 
> Alternatively, I would have suggested to define the "location" prior to including the debug header, e.g.,:
>
>   #define DEBUG_LOCATION "PLUGIN"
>   #include "Debug.h"
>
> which we verify like:
>
>   #if DEBUG_LOCATION != "PLUGIN" and DEBUG_LOCATION != "OMPTARGET"
>   #error ...
>   #endif
>
> At the end of the day I want to simplify things. A single location for all our debug needs sounds simpler than `1 + #plugins` to me, even if we don't use all functionality at each location. If separation does not allow anything we cannot reasonably do in a single location, I doubt it provides a benefit.

OK. it seems that macros are unified by DEBUG_PREFIX. So it is good for the purpose of this patch.
Long term, I think the plugin side info should be controlled separately from libomptarget side.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86804