[PATCH v2 1/3] vcomp: Implement _vcomp_for_dynamic_init_i8()

[Zebediah Figura (she/her)]

On 2/18/22 15:17, David Koller wrote:
> Thanks a lot, Zebediah, for your comments on my vcomp patches; I
> really appreciate you taking time to review them!
> 
>> Does it even make sense to make these unions? Can we just make the
>> relevant members 64-bit?
>>
>> Alternatively, should these be stored separately from the 32-bit
>> versions? Tests showing what happens if you mix and match 32-bit and
>> 64-bit calls might be helpful.
> 
> Right, I considered three alternatives:
> 1. Create new separate vcomp_thread and vcomp_task structures for the
> 64-bit data,
> 2. Increase the 32-bit data members to 64-bit, or
> 3. Use unions
> 
> Creating new separate 64-bit data structures for the 64-bit calls
> would necessitate adding a bunch of new and redundant code (to
> initialize the new structs, etc.), but would be the most performant.
> Increasing the 32-bit members to 64-bit would waste the most memory
> and be the least performant for 32-bit calls, but would eliminate
> redundant code. My choice of unions is a compromise; new data
> structures are unnecessary and the separate 32-bit and 64-bit calls
> each access their native data types, which is good for performance.
> 
> Using unions in this way is a traditional approach in OpenMP runtime
> implementations. For example, if you look at the code for libgomp, the
> most widely used OpenMP implementation, you will see that it uses
> unions to handle 32/64-bit variants in a similar manner:
> 
>    https://github.com/gcc-mirror/gcc/blob/1931cbad498e625b1e24452dcfffe02539b12224/libgomp/libgomp.h#L288-L396
> 

Increasing the existing fields to 64-bit uses the exact same amount of
memory as using a union. I don't know why libgomp did that, but it seems
pointless to me.

Note that I also asked for tests as this may help determine what the
correct approach is. E.g. if mixing 32- and 64-bit types works without a
hitch, that suggests that widening the existing fields is the correct
approach.

>>
>> Could we share any (or all) of this code with the 32-bit version?
>>
> 
> Yes, if the top priority was to minimize redundant code, then
> refactoring these functions to handle both the 32-bit and 64-bit cases
> is possible. However, performance would suffer. If you look at the
> other previously-implemented vcomp functions, you'll see that earlier
> Wine contributors implemented separate 32-bit and 64-bit versions of
> functions for other scheduling modes: _vcomp_for_static_init() and
> _vcomp_for_static_init_i8(), _vcomp_for_static_simple_init() and
> _vcomp_for_static_simple_init_i8(), etc. My patches follow this same
> pre-established pattern.

Just from reading the code, it seems that the "static" initializers are
something of a different case, since they're writing to fields which are
by definition either 32- or 64-bit. If we were to use the same fields,
that concern wouldn't seem to apply here, although I may be missing
something.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20220222/39bf0116/attachment.sig>