[PATCH 3/9] eal: use barrier intrinsics when compiling with msvc

[Konstantin Ananyev]

05/04/2023 16:38, Tyler Retzlaff пишет:
> On Wed, Apr 05, 2023 at 02:35:47PM +0200, Morten Brørup wrote:
>>> From: Konstantin Ananyev [mailto:konstantin.ananyev at huawei.com]
>>> Sent: Wednesday, 5 April 2023 12.57
>>>
>>>>>>> Another ore generic comment - do we really need to pollute all that code
>>> with RTE_TOOLCHAIN_MSVC ifdefs?
>>>>>>> Right now we have ability to have subdir per arch (x86/arm/etc.).
>>>>>>> Can we treat x86+windows+msvc as a special arch?
>>>>>>
>>>>>> i asked this question previously and confirmed in the technical board
>>>>>> meeting. the answer i received was that the community did not want new
>>>>>> directory/headers introduced for compiler support matrix and i should
>>>>>> use #ifdef in the existing headers.
>>>>>
>>>>> Ok, can I then ask at least to minimize number of ifdefs to absolute
>>>>> minimum?
>>>>
>>>> in principal no objection at all, one question though is what to do with
>>>> comment based documentation attached to macros? e.g.
>>>>
>>>> #ifdef SOME_FOO
>>>> /* some documentation */
>>>> #define some_macro
>>>> #else
>>>> #define some_macro
>>>> #endif
>>>>
>>>> #ifdef SOME_FOO
>>>> /* some documentation 2 */
>>>> #define some_macro2
>>>> #else
>>>> #define some_macro2
>>>> #endif
>>>>
>>>> i can either duplicate the documentation for every define so it stays
>>>> "attached" or i can only document the first expansion. let me know what
>>>> you expect.
>>>>
>>>> so something like this?
>>>>
>>>> #ifdef SOME_FOO
>>>> /* some documentation */
>>>> #define some_macro
>>>> /* some documentation 2 */
>>>> #define some_macro2
>>>> #else
>>>> #define some_macro
>>>> #define some_macro2
>>>> #endif
>>>>
>>>> or should all documentation be duplicated? which can become a teadious
>>>> redundancy for anyone maintaining it. keep in mind we might have to make
>>>> an exception for rte_common.h because it seems doing this would be
>>>> really ugly there. take a look let me know.
>>>
>>> My personal preference would be to keep one documentation block for both cases
>>> (yes, I suppose it needs to be updated if required):
>>>
>>> /* some documentation, probably for both SOME_FOO on/off */
>>> #ifdef SOME_FOO
>>> #define some_macro
>>> #else
>>> #define some_macro
>>> #endif
>>>
>>
>> Or the third option of using a dummy for documentation purposes only. rte_memcpy() does this [1], although it is an inline function and not a macro.
>>
>> [1]: https://elixir.bootlin.com/dpdk/v23.03/source/lib/eal/include/generic/rte_memcpy.h#L90
>>
>> No preferences here, just mentioning it!
>>
>>>
>>>>
>>>>> It is really hard to read an follow acode that is heavily ifdefed.
>>
>> Yes, and sometimes it is even harder reading code that is spread across multiple arch-depending files.
>>
>> It is a choice between the plague or cholera.

That's an interesting analogy - never thought about programming
as a decease :)
Back to the subject, at least to me two clear files with one conditional
include is much more sane then one file with bunch of ifdefs inside.

>>
>> But it is one of the unavoidable downsides to supporting many architectures and compilers, so we have to seek out the best compromises.
> 
> yes, there is a conditional that has to be *somewhere*. i would propose
> for now that it be done per-macro or whatever. but when i get the bulk
> of the changes in i can commit to refactoring some groups out into their
> own header. rte_common.h is a good candidate for this because of how
> many conditionals it will contain.
> 
> generic/rte_common.h
> -> msvc/rte_common.h #include "generic/rte_common.h"
> -> gnu?/rte_common.h #include "generic/rte_common.h"
> 
> this could carry inline/macro documentation in generic/rte_common.h but
> again i'd prefer to do this after msvc work is stood up at least to the
> point of having unit tests working before i start moving code around.

About special rte_common.h for msvc - that sounds like a good option to me.

> for other conditionals i don't think it's worth having a whole file e.g.
> x86/include/rte_pause.h or something only a couple of blocks are
> conditional.
> 
> 
> ty
>