oneDPL icon indicating copy to clipboard operation
oneDPL copied to clipboard

Change usage of _PSTL_UD[R/S]_PRESENT macros

Open adamfidel opened this issue 1 year ago • 5 comments

On some platforms, specifically Fedora 40 with GCC 14, the included pstl_config.h is such that the macros _PSTL_UDR_PRESENT and _PSTL_UDS_PRESENT are present but defined as an empty macro. This is causing compilation errors.

In this PR, we work around the empty macros by first testing _PSTL_UDR_PRESENT + 0, which evaluates to +0 (i.e., false) if the macro is undefined and the correct value if it is defined.

adamfidel avatar May 01 '24 15:05 adamfidel

Alternatively, perhaps we can instead "process" an empty but defined _PSTL_UDR_PRESENT into some local version that we then use so that we can have this workaround only in one place.

danhoeflinger avatar May 01 '24 16:05 danhoeflinger

@danhoeflinger: Should we instead align our _PSTL_UDR_PRESENT with how others treat it (defined on undefined) and then check defined(_PSTL_UDR_PRESENT)?

Are you suggesting that we change oneDPL's pstl_config.h to make this macro either undefined or empty defined, instead of always being defined 0 or 1 as it is currently? I'm not sure what the "correct" version is or how the upstream PSTL manages their pstl_config.h. I believe @rarutyun might have to weigh in on that.

@danhoeflinger: Alternatively, perhaps we can instead "process" an empty but defined _PSTL_UDR_PRESENT into some local version that we then use so that we can have this workaround only in one place.

That was my original thought, but I couldn't quite place where to put it.

It doesn't make sense to put it in pstl_config.h since that is where the issue arises (and there's the question of whether it will be included at all instead of the system's pstl_config.h). It also doesn't makes sense to put it into onedpl_config.h because there we define our own _ONEDPL_UDR_PRESENT which raises the question, why do we have two macros for the same thing? I don't have the historical context for that.

adamfidel avatar May 01 '24 16:05 adamfidel

We do not include pstl_config.h anywhere in our own code. I think it is here to easier sync with upstream, but someone with more knowledge of the sync process can correct me there. This wouldn't be the place to create our own version, but I also don't know the appropriate place.

I'm realizing that my original suggestion to align with their handling doesn't work, because we may encounter either way of defining _PSTL_UDR_PRESENT from an external source. Looking at the blame, https://github.com/gcc-mirror/gcc/commit/3162ca09dbdc2ea3ff1d4b752edf76955b6d94f9 is where they made the switch away from our current approach to this one.

It seems we will have the same problem with _PSTL_UDS_PRESENT as well. I didn't notice any others, but its worth a look.

One option is to make a macro: _ONEDPL_DEFINED_AND_NOT_ZERO(FLAG) which could be reused for something like this and have the workaround in one place. However, again I don't know where to put that.

danhoeflinger avatar May 01 '24 16:05 danhoeflinger

@danhoeflinger One option is to make a macro: _ONEDPL_DEFINED_AND_NOT_ZERO(FLAG) which could be reused for something like this and have the workaround in one place. However, again I don't know where to put that.

I changed this PR to create a new _ONEDPL_DEFINED_AND_NOT_ZERO macro and use that instead. I think it made it much simpler.

adamfidel avatar May 01 '24 16:05 adamfidel

I changed this PR to create a new _ONEDPL_DEFINED_AND_NOT_ZERO macro and use that instead. I think it made it much simpler.

I like it a lot better now after this switch. One real comment (above) to address from my perspective, but I also think this deserves a look from @rarutyun or someone else with better understanding of the upstream process before going forward.

danhoeflinger avatar May 01 '24 17:05 danhoeflinger