Re: [Mesa-dev] [PATCH] radeonsi: correct si_vgt_param_key on big endian machines

[Michel Dänzer]

On 2018-04-10 10:03 AM, Bas Vermeulen wrote:
> On Mon, Apr 9, 2018 at 11:19 PM, Gert Wollny  wrote:
>> Am Montag, den 09.04.2018, 14:03 -0400 schrieb Marek Olšák:
>>> On Mon, Apr 9, 2018 at 10:51 AM, Bas Vermeulen 
> 
>> There is another option: Check at configuration time whether the bit
>> field layout is like the low or the high endian layout you already
>> implemented, and instead of basing the selection of the struct layout
>> on the big/low-endianess of the architecture, base it on this test.
>>
>> It would probably be prudent to test both layouts and then fail
>> configuration if non of the two reflect the actual layout (at which
>> point one would have to thing about how to implement all the bit
>> shifting properly).
> 
> Or just keep the union dependent on endianness, and add an assert/check/test
> to make sure that everything works as expected.

Again, it's nothing to do with endianness — the CPU always accesses the
union in its native byte order.

The issue is that the C standard doesn't define the memory layout of
bit-fields, and the Linux powerpc architecture uses a different layout
than x86.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: correct si_vgt_param_key on big endian machines

[Bas Vermeulen]

On Mon, Apr 9, 2018 at 11:19 PM, Gert Wollny  wrote:

> Am Montag, den 09.04.2018, 14:03 -0400 schrieb Marek Olšák:
> > On Mon, Apr 9, 2018 at 10:51 AM, Bas Vermeulen 
> > wrote:
> Which solution is better depends on what is done more often: reading
> the index or writing to the bit fields.
>

The bitfields are read and written, and the index is mostly read. I found
four instances of the bitfields being written after which the index needs to
be updated.


> > > I am working on a new version of this patch. I have one version
> > > which does away with all the bitfields, and uses functions to
> > > update the index.
> This emulates the code the compiler would create, but it requires that
> for each bit field setters (and getters?) must be implemented.
>

Yes. I have a git branch with this change ready if that's what's
wanted/needed.


> > > Another approach would be to change the union to a struct, and use
> > > a function to get the index.
> This method has the advantage that only the access to the index needs
> new implementation.
>

I can prepare a patch for this as well.


> > > Yet another approach would be to keep the contents of the union and
> > > the index in one struct, and use a function to
> > > (re)calculate the index.
> I don't think that would make much sense.
>

It adds four lines to the code, all the key->u.xxx has it's u. removed.
But future implementation needs to remember to call that function if any of
the bitfields are changed. Which can be annoying.

There is another option: Check at configuration time whether the bit
> field layout is like the low or the high endian layout you already
> implemented, and instead of basing the selection of the struct layout
> on the big/low-endianess of the architecture, base it on this test.
>
> It would probably be prudent to test both layouts and then fail
> configuration if non of the two reflect the actual layout (at which
> point one would have to thing about how to implement all the bit
> shifting properly).


Or just keep the union dependent on endianness, and add an assert/check/test
to make sure that everything works as expected.


> > >
> > > Which would you prefer?
> > >
> >
> > I don't mind bitfields. They make the code nice and tiny. Shifts
> > would decrease readability.
> The problem is, that the layout of bitfields is compiler dependend.
>

Let me know what you guys want to have this included. I just want it fixed,
I don't really care on the form. :)

Bas Vermeulen
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: correct si_vgt_param_key on big endian machines

[Michel Dänzer]

On 2018-04-10 08:38 AM, Gert Wollny wrote:
> Am Montag, den 09.04.2018, 17:26 -0400 schrieb Marek Olšák:
>> On Mon, Apr 9, 2018 at 5:19 PM, Gert Wollny 
>> wrote:
>>>
>>> There is another option: Check at configuration time whether the
>>> bit field layout is like the low or the high endian layout you
>>> already implemented, and instead of basing the selection of the
>>> struct layout on the big/low-endianess of the architecture, base it
>>> on this test.
>>>
>>> It would probably be prudent to test both layouts and then fail
>>> configuration if non of the two reflect the actual layout (at which
>>> point one would have to thing about how to implement all the bit
>>> shifting properly).
>>>
>
> Which would you prefer?
>

 I don't mind bitfields. They make the code nice and tiny. Shifts
 would decrease readability.
>>> The problem is, that the layout of bitfields is compiler dependend.
>>
>> We can fix it after we discover that it's a real problem on a
>> compiler we care about.
>>
> I don't think it is a good idea to rely on undefined behaviour, but if
> it is done, then the least one can do is to add a test that flags an
> upcoming problem before it can do any damage; one example how to do
> this I described above, another approach would be to add a unit test.

I agree. In particular, this has nothing to do with endianness, since
union si_vgt_param_key is only ever accessed by the CPU in its native
byte order.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: correct si_vgt_param_key on big endian machines

[Gert Wollny]

Am Montag, den 09.04.2018, 17:26 -0400 schrieb Marek Olšák:
> On Mon, Apr 9, 2018 at 5:19 PM, Gert Wollny 
> wrote:
> > 
> > 
> > There is another option: Check at configuration time whether the
> > bit field layout is like the low or the high endian layout you
> > already implemented, and instead of basing the selection of the
> > struct layout on the big/low-endianess of the architecture, base it
> > on this test.
> > 
> > It would probably be prudent to test both layouts and then fail
> > configuration if non of the two reflect the actual layout (at which
> > point one would have to thing about how to implement all the bit
> > shifting properly).
> > 
> > > >
> > > > Which would you prefer?
> > > >
> > >
> > > I don't mind bitfields. They make the code nice and tiny. Shifts
> > > would decrease readability.
> > The problem is, that the layout of bitfields is compiler dependend.
> 
> We can fix it after we discover that it's a real problem on a
> compiler we care about.
> 
I don't think it is a good idea to rely on undefined behaviour, but if
it is done, then the least one can do is to add a test that flags an
upcoming problem before it can do any damage; one example how to do
this I described above, another approach would be to add a unit test.

Best, 
Gert

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: correct si_vgt_param_key on big endian machines

[Marek Olšák]

On Mon, Apr 9, 2018 at 5:19 PM, Gert Wollny  wrote:

> Am Montag, den 09.04.2018, 14:03 -0400 schrieb Marek Olšák:
> > On Mon, Apr 9, 2018 at 10:51 AM, Bas Vermeulen 
> > wrote:
> Which solution is better depends on what is done more often: reading
> the index or writing to the bit fields.
>
> > > I am working on a new version of this patch. I have one version
> > > which does away with all the bitfields, and uses functions to
> > > update the index.
> This emulates the code the compiler would create, but it requires that
> for each bit field setters (and getters?) must be implemented.
>
> > > Another approach would be to change the union to a struct, and use
> > > a function to get the index.
> This method has the advantage that only the access to the index needs
> new implementation.
>
> > > Yet another approach would be to keep the contents of the union and
> > > the index in one struct, and use a function to
> > > (re)calculate the index.
> I don't think that would make much sense.
>
> There is another option: Check at configuration time whether the bit
> field layout is like the low or the high endian layout you already
> implemented, and instead of basing the selection of the struct layout
> on the big/low-endianess of the architecture, base it on this test.
>
> It would probably be prudent to test both layouts and then fail
> configuration if non of the two reflect the actual layout (at which
> point one would have to thing about how to implement all the bit
> shifting properly).
>
> > >
> > > Which would you prefer?
> > >
> >
> > I don't mind bitfields. They make the code nice and tiny. Shifts
> > would decrease readability.
> The problem is, that the layout of bitfields is compiler dependend.
>

We can fix it after we discover that it's a real problem on a compiler we
care about.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: correct si_vgt_param_key on big endian machines

[Gert Wollny]

Am Montag, den 09.04.2018, 14:03 -0400 schrieb Marek Olšák:
> On Mon, Apr 9, 2018 at 10:51 AM, Bas Vermeulen 
> wrote:
Which solution is better depends on what is done more often: reading
the index or writing to the bit fields. 

> > I am working on a new version of this patch. I have one version
> > which does away with all the bitfields, and uses functions to
> > update the index.
This emulates the code the compiler would create, but it requires that
for each bit field setters (and getters?) must be implemented. 

> > Another approach would be to change the union to a struct, and use
> > a function to get the index.
This method has the advantage that only the access to the index needs
new implementation.

> > Yet another approach would be to keep the contents of the union and
> > the index in one struct, and use a function to
> > (re)calculate the index.
I don't think that would make much sense. 

There is another option: Check at configuration time whether the bit
field layout is like the low or the high endian layout you already
implemented, and instead of basing the selection of the struct layout
on the big/low-endianess of the architecture, base it on this test.

It would probably be prudent to test both layouts and then fail
configuration if non of the two reflect the actual layout (at which
point one would have to thing about how to implement all the bit
shifting properly). 

> > 
> > Which would you prefer?
> > 
> 
> I don't mind bitfields. They make the code nice and tiny. Shifts
> would decrease readability.
The problem is, that the layout of bitfields is compiler dependend. 

Best, 
Gert

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: correct si_vgt_param_key on big endian machines

[Marek Olšák]

On Mon, Apr 9, 2018 at 10:51 AM, Bas Vermeulen  wrote:

> I am working on a new version of this patch. I have one version which does
> away with all the bitfields, and uses
> functions to update the index.
> Another approach would be to change the union to a struct, and use a
> function to get the index.
> Yet another approach would be to keep the contents of the union and the
> index in one struct, and use a function to
> (re)calculate the index.
>
> Which would you prefer?
>

I don't mind bitfields. They make the code nice and tiny. Shifts would
decrease readability.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: correct si_vgt_param_key on big endian machines

[Bas Vermeulen]

I am working on a new version of this patch. I have one version which does
away with all the bitfields, and uses
functions to update the index.
Another approach would be to change the union to a struct, and use a
function to get the index.
Yet another approach would be to keep the contents of the union and the
index in one struct, and use a function to
(re)calculate the index.

Which would you prefer?

Bas Vermeulen

On Tue, Mar 20, 2018 at 6:33 PM, Gert Wollny  wrote:

> Am Dienstag, den 20.03.2018, 15:33 +0100 schrieb Nicolai Hähnle:
> > Nice, did you actually get it to work entirely on a big endian
> > machine?
> >
> > Bit fields aren't super portable, [...]
> Indeed, the order of the bits in a bit field is compiler implementation
> dependent. To make sure that changing the compiler doesn't change the
> behaviour of the code I'd suggest that instead of using a bit field the
> index should be created by explicitly shifting the bits into the right
> positions.
>
> Best,
> Gert
>
> > However, I
> > think we should use the PIPE_ARCH_LITTLE_ENDIAN define from
> > u_endian.h
> >
> > Cheers,
> > Nicolai
> >
> > On 20.03.2018 15:21, Bas Vermeulen wrote:
> > > Using mesa OpenCL failed on a big endian PowerPC machine because
> > > si_vgt_param_key is using bitfields and a 32 bit int for an
> > > index into an array.
> > >
> > > Fix si_vgt_param_key to work correctly on both little endian
> > > and big endian machines.
> > >
> > > Signed-off-by: Bas Vermeulen 
> > > ---
> > >   src/gallium/drivers/radeonsi/si_pipe.h | 13 +
> > >   1 file changed, 13 insertions(+)
> > >
> > > diff --git a/src/gallium/drivers/radeonsi/si_pipe.h
> > > b/src/gallium/drivers/radeonsi/si_pipe.h
> > > index 2053dcb9fc..32dbdf6e2c 100644
> > > --- a/src/gallium/drivers/radeonsi/si_pipe.h
> > > +++ b/src/gallium/drivers/radeonsi/si_pipe.h
> > > @@ -385,6 +385,7 @@ struct si_shader_ctx_state {
> > >*/
> > >   union si_vgt_param_key {
> > > struct {
> > > +#if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
> > > unsigned prim:4;
> > > unsigned uses_instancing:1;
> > > unsigned
> > > multi_instances_smaller_than_primgroup:1;
> > > @@ -395,6 +396,18 @@ union si_vgt_param_key {
> > > unsigned tess_uses_prim_id:1;
> > > unsigned uses_gs:1;
> > > unsigned _pad:32 - SI_NUM_VGT_PARAM_KEY_BITS;
> > > +#else /* __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ */
> > > +   unsigned _pad:32 - SI_NUM_VGT_PARAM_KEY_BITS;
> > > +   unsigned uses_gs:1;
> > > +   unsigned tess_uses_prim_id:1;
> > > +   unsigned uses_tess:1;
> > > +   unsigned line_stipple_enabled:1;
> > > +   unsigned count_from_stream_output:1;
> > > +   unsigned primitive_restart:1;
> > > +   unsigned multi_instances_smaller_than_primgroup:1;
> > > +   unsigned uses_instancing:1;
> > > +   unsigned prim:4;
> > > +#endif
> > > } u;
> > > uint32_t index;
> > >   };
> > >
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: correct si_vgt_param_key on big endian machines

[Gert Wollny]

Am Dienstag, den 20.03.2018, 15:33 +0100 schrieb Nicolai Hähnle:
> Nice, did you actually get it to work entirely on a big endian
> machine?
> 
> Bit fields aren't super portable, [...]
Indeed, the order of the bits in a bit field is compiler implementation
dependent. To make sure that changing the compiler doesn't change the
behaviour of the code I'd suggest that instead of using a bit field the
index should be created by explicitly shifting the bits into the right
positions.

Best,
Gert 

> However, I 
> think we should use the PIPE_ARCH_LITTLE_ENDIAN define from
> u_endian.h
> 
> Cheers,
> Nicolai
> 
> On 20.03.2018 15:21, Bas Vermeulen wrote:
> > Using mesa OpenCL failed on a big endian PowerPC machine because
> > si_vgt_param_key is using bitfields and a 32 bit int for an
> > index into an array.
> > 
> > Fix si_vgt_param_key to work correctly on both little endian
> > and big endian machines.
> > 
> > Signed-off-by: Bas Vermeulen 
> > ---
> >   src/gallium/drivers/radeonsi/si_pipe.h | 13 +
> >   1 file changed, 13 insertions(+)
> > 
> > diff --git a/src/gallium/drivers/radeonsi/si_pipe.h
> > b/src/gallium/drivers/radeonsi/si_pipe.h
> > index 2053dcb9fc..32dbdf6e2c 100644
> > --- a/src/gallium/drivers/radeonsi/si_pipe.h
> > +++ b/src/gallium/drivers/radeonsi/si_pipe.h
> > @@ -385,6 +385,7 @@ struct si_shader_ctx_state {
> >*/
> >   union si_vgt_param_key {
> > struct {
> > +#if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
> > unsigned prim:4;
> > unsigned uses_instancing:1;
> > unsigned
> > multi_instances_smaller_than_primgroup:1;
> > @@ -395,6 +396,18 @@ union si_vgt_param_key {
> > unsigned tess_uses_prim_id:1;
> > unsigned uses_gs:1;
> > unsigned _pad:32 - SI_NUM_VGT_PARAM_KEY_BITS;
> > +#else /* __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ */
> > +   unsigned _pad:32 - SI_NUM_VGT_PARAM_KEY_BITS;
> > +   unsigned uses_gs:1;
> > +   unsigned tess_uses_prim_id:1;
> > +   unsigned uses_tess:1;
> > +   unsigned line_stipple_enabled:1;
> > +   unsigned count_from_stream_output:1;
> > +   unsigned primitive_restart:1;
> > +   unsigned multi_instances_smaller_than_primgroup:1;
> > +   unsigned uses_instancing:1;
> > +   unsigned prim:4;
> > +#endif
> > } u;
> > uint32_t index;
> >   };
> > 
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: correct si_vgt_param_key on big endian machines

[Bas Vermeulen]

I'm able to call clinfo without things crashing. Without this fix, clinfo
results in a signal 11 because key.index is byte swapped. With it,
I get the information I would expect. I'm working to test the OpenCL
currently.

I'll update the patch to use PIPE_ARCH_LITTLE_ENDIAN instead of my own #if.

Bas Vermeulen

On Tue, Mar 20, 2018 at 3:33 PM, Nicolai Hähnle 
wrote:

> Nice, did you actually get it to work entirely on a big endian machine?
>
> Bit fields aren't super portable, but this looks good enough. However, I
> think we should use the PIPE_ARCH_LITTLE_ENDIAN define from u_endian.h
>
> Cheers,
> Nicolai
>
>
> On 20.03.2018 15:21, Bas Vermeulen wrote:
>
>> Using mesa OpenCL failed on a big endian PowerPC machine because
>> si_vgt_param_key is using bitfields and a 32 bit int for an
>> index into an array.
>>
>> Fix si_vgt_param_key to work correctly on both little endian
>> and big endian machines.
>>
>> Signed-off-by: Bas Vermeulen 
>> ---
>>   src/gallium/drivers/radeonsi/si_pipe.h | 13 +
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/src/gallium/drivers/radeonsi/si_pipe.h
>> b/src/gallium/drivers/radeonsi/si_pipe.h
>> index 2053dcb9fc..32dbdf6e2c 100644
>> --- a/src/gallium/drivers/radeonsi/si_pipe.h
>> +++ b/src/gallium/drivers/radeonsi/si_pipe.h
>> @@ -385,6 +385,7 @@ struct si_shader_ctx_state {
>>*/
>>   union si_vgt_param_key {
>> struct {
>> +#if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
>> unsigned prim:4;
>> unsigned uses_instancing:1;
>> unsigned multi_instances_smaller_than_primgroup:1;
>> @@ -395,6 +396,18 @@ union si_vgt_param_key {
>> unsigned tess_uses_prim_id:1;
>> unsigned uses_gs:1;
>> unsigned _pad:32 - SI_NUM_VGT_PARAM_KEY_BITS;
>> +#else /* __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ */
>> +   unsigned _pad:32 - SI_NUM_VGT_PARAM_KEY_BITS;
>> +   unsigned uses_gs:1;
>> +   unsigned tess_uses_prim_id:1;
>> +   unsigned uses_tess:1;
>> +   unsigned line_stipple_enabled:1;
>> +   unsigned count_from_stream_output:1;
>> +   unsigned primitive_restart:1;
>> +   unsigned multi_instances_smaller_than_primgroup:1;
>> +   unsigned uses_instancing:1;
>> +   unsigned prim:4;
>> +#endif
>> } u;
>> uint32_t index;
>>   };
>>
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radeonsi: correct si_vgt_param_key on big endian machines

[Nicolai Hähnle]

Nice, did you actually get it to work entirely on a big endian machine?

Bit fields aren't super portable, but this looks good enough. However, I 
think we should use the PIPE_ARCH_LITTLE_ENDIAN define from u_endian.h


Cheers,
Nicolai

On 20.03.2018 15:21, Bas Vermeulen wrote:

Using mesa OpenCL failed on a big endian PowerPC machine because
si_vgt_param_key is using bitfields and a 32 bit int for an
index into an array.

Fix si_vgt_param_key to work correctly on both little endian
and big endian machines.

Signed-off-by: Bas Vermeulen 
---
  src/gallium/drivers/radeonsi/si_pipe.h | 13 +
  1 file changed, 13 insertions(+)

diff --git a/src/gallium/drivers/radeonsi/si_pipe.h 
b/src/gallium/drivers/radeonsi/si_pipe.h
index 2053dcb9fc..32dbdf6e2c 100644
--- a/src/gallium/drivers/radeonsi/si_pipe.h
+++ b/src/gallium/drivers/radeonsi/si_pipe.h
@@ -385,6 +385,7 @@ struct si_shader_ctx_state {
   */
  union si_vgt_param_key {
struct {
+#if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
unsigned prim:4;
unsigned uses_instancing:1;
unsigned multi_instances_smaller_than_primgroup:1;
@@ -395,6 +396,18 @@ union si_vgt_param_key {
unsigned tess_uses_prim_id:1;
unsigned uses_gs:1;
unsigned _pad:32 - SI_NUM_VGT_PARAM_KEY_BITS;
+#else /* __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ */
+   unsigned _pad:32 - SI_NUM_VGT_PARAM_KEY_BITS;
+   unsigned uses_gs:1;
+   unsigned tess_uses_prim_id:1;
+   unsigned uses_tess:1;
+   unsigned line_stipple_enabled:1;
+   unsigned count_from_stream_output:1;
+   unsigned primitive_restart:1;
+   unsigned multi_instances_smaller_than_primgroup:1;
+   unsigned uses_instancing:1;
+   unsigned prim:4;
+#endif
} u;
uint32_t index;
  };



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev