Re: [Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

[Christophe Fergeau]

On Mon, Sep 25, 2017 at 04:37:43PM +0200, Christophe Fergeau wrote:
> On Mon, Sep 25, 2017 at 10:22:57AM -0400, Frediano Ziglio wrote:
> > > 
> > > On Mon, Sep 25, 2017 at 09:04:16AM -0400, Frediano Ziglio wrote:
> > > > Yes, intentional and consistent with previous code.
> > > > The usage of unsigned for BYTESWAP avoid sign extension during 
> > > > arithmetic.
> > > 
> > > If BYTESWAP has such issues (and I don't think it does), then this should 
> > > be
> > > fixed.
> > > ack on being consistent with what was done before.
> > > 
> > > Christophe
> > > 
> > 
> > Looking at macro sources do not seem to have such issue.
> > 
> > OT: Looking at macro sources why don't we use asm/swab.h or byteswap.h on 
> > Linux?
> > They provide support for more architectures. Or something like
> 
> Even el6 has gcc 4.4, I'd just drop the whole #elif defined (__GNUC__) && 
> (__GNUC__ >= 2) && defined (__OPTIMIZE__)
> block and only keep __builtin_bswap*() and the generic fallback.


by the way, for the series,
Acked-by: Christophe Fergeau 

> 
> Christophe
> ___
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

[Christophe Fergeau]

On Mon, Sep 25, 2017 at 10:22:57AM -0400, Frediano Ziglio wrote:
> > 
> > On Mon, Sep 25, 2017 at 09:04:16AM -0400, Frediano Ziglio wrote:
> > > Yes, intentional and consistent with previous code.
> > > The usage of unsigned for BYTESWAP avoid sign extension during arithmetic.
> > 
> > If BYTESWAP has such issues (and I don't think it does), then this should be
> > fixed.
> > ack on being consistent with what was done before.
> > 
> > Christophe
> > 
> 
> Looking at macro sources do not seem to have such issue.
> 
> OT: Looking at macro sources why don't we use asm/swab.h or byteswap.h on 
> Linux?
> They provide support for more architectures. Or something like

Even el6 has gcc 4.4, I'd just drop the whole #elif defined (__GNUC__) && 
(__GNUC__ >= 2) && defined (__OPTIMIZE__)
block and only keep __builtin_bswap*() and the generic fallback.

Christophe
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

[Frediano Ziglio]

> 
> On Mon, Sep 25, 2017 at 09:04:16AM -0400, Frediano Ziglio wrote:
> > Yes, intentional and consistent with previous code.
> > The usage of unsigned for BYTESWAP avoid sign extension during arithmetic.
> 
> If BYTESWAP has such issues (and I don't think it does), then this should be
> fixed.
> ack on being consistent with what was done before.
> 
> Christophe
> 

Looking at macro sources do not seem to have such issue.

OT: Looking at macro sources why don't we use asm/swab.h or byteswap.h on Linux?
They provide support for more architectures. Or something like

--- a/spice/macros.h
+++ b/spice/macros.h
@@ -248,7 +248,11 @@
 
 /* Arch specific stuff for speed
  */
-#if defined (__GNUC__) && (__GNUC__ >= 2) && defined (__OPTIMIZE__)
+#if defined (__GNUC__) && ((__GNUC__ > 4) || (__GNUC__ == 4 && __GNUC_MINOR__ 
>= 3))
+#  define SPICE_BYTESWAP16(val) __builtin_bswap16(val)
+#  define SPICE_BYTESWAP32(val) __builtin_bswap32(val)
+#  define SPICE_BYTESWAP64(val) __builtin_bswap64(val)
+#elif defined (__GNUC__) && (__GNUC__ >= 2) && defined (__OPTIMIZE__)
 #  if defined (__i386__)
 #define SPICE_BYTESWAP16_IA32(val) \
(__extension__  \

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

[Christophe Fergeau]

On Mon, Sep 25, 2017 at 09:04:16AM -0400, Frediano Ziglio wrote:
> Yes, intentional and consistent with previous code.
> The usage of unsigned for BYTESWAP avoid sign extension during arithmetic.

If BYTESWAP has such issues (and I don't think it does), then this should be 
fixed.
ack on being consistent with what was done before.

Christophe
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

[Frediano Ziglio]

> 
> On Fri, Sep 22, 2017 at 11:10:02AM +0100, Frediano Ziglio wrote:
> > Instead of assuming that the system can safely do unaligned access
> > to memory use packed structures to allow the compiler generate
> > best code possible.
> > A packed structure tells the compiler to not leave padding inside it
> > and that the structure can be unaligned so any field can be unaligned
> > having to generate proper access code based on architecture.
> > For instance ARM7 can use unaligned access but not for 64 bit
> > numbers (currently these accesses are emulated by Linux kernel
> > with obvious performance consequences).
> > 
> > This changes the current methods from:
> > 
> > #ifdef WORDS_BIGENDIAN
> > #define read_uint32(ptr) ((uint32_t)SPICE_BYTESWAP32(*((uint32_t *)(ptr
> > #define write_uint32(ptr, val) *(uint32_t *)(ptr) =
> > SPICE_BYTESWAP32((uint32_t)val)
> > #else
> > #define read_uint32(ptr) (*((uint32_t *)(ptr)))
> > #define write_uint32(ptr, val) (*((uint32_t *)(ptr))) = val
> > #endif
> > 
> > to:
> > 
> > #include 
> > typedef struct SPICE_ATTR_PACKED {
> > uint32_t v;
> > } uint32_unaligned_t;
> > #include 
> > 
> > #ifdef WORDS_BIGENDIAN
> > #define read_uint32(ptr) ((uint32_t)SPICE_BYTESWAP32(((uint32_unaligned_t
> > *)(ptr))->v))
> 
> For int32, this generates
> #define read_uint32(ptr) ((int32_t)SPICE_BYTESWAP32(((uint32_unaligned_t
> *)(ptr))->v))
> rather than
> #define read_uint32(ptr) ((int32_t)SPICE_BYTESWAP32(((int32_unaligned_t
> *)(ptr))->v))
> I don't know if this is intentional?

Yes, intentional and consistent with previous code.
The usage of unsigned for BYTESWAP avoid sign extension during arithmetic.

> (the change would be:
> 
> diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
> index da87d44..241e620 100644
> --- a/python_modules/demarshal.py
> +++ b/python_modules/demarshal.py
> @@ -63,7 +63,7 @@ def write_parser_helpers(writer):
>  utype = "uint%d" % (size)
>  type = "%sint%d" % (sign, size)
>  swap = "SPICE_BYTESWAP%d" % size
> -writer.macro("read_%s" % type, "ptr",
> "((%s_t)%s(((%s_unaligned_t *)(ptr))->v))" % (type, swap, utype))
> +writer.macro("read_%s" % type, "ptr",
> "((%s_t)%s(((%s_unaligned_t *)(ptr))->v))" % (type, swap, type))
>  writer.macro("write_%s" % type, "ptr, val", "((%s_unaligned_t
>  *)(ptr))->v = %s((%s_t)val)" % (utype, swap, utype))
>  writer.writeln("#else")
>  for size in [16, 32, 64]:
> 
> Apart from this, looks good.
> 
> Christophe
> 

Frediano
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice-common v2 1/3] Make the compiler work out better way to write unaligned memory

[Christophe Fergeau]

On Fri, Sep 22, 2017 at 11:10:02AM +0100, Frediano Ziglio wrote:
> Instead of assuming that the system can safely do unaligned access
> to memory use packed structures to allow the compiler generate
> best code possible.
> A packed structure tells the compiler to not leave padding inside it
> and that the structure can be unaligned so any field can be unaligned
> having to generate proper access code based on architecture.
> For instance ARM7 can use unaligned access but not for 64 bit
> numbers (currently these accesses are emulated by Linux kernel
> with obvious performance consequences).
> 
> This changes the current methods from:
> 
> #ifdef WORDS_BIGENDIAN
> #define read_uint32(ptr) ((uint32_t)SPICE_BYTESWAP32(*((uint32_t *)(ptr
> #define write_uint32(ptr, val) *(uint32_t *)(ptr) = 
> SPICE_BYTESWAP32((uint32_t)val)
> #else
> #define read_uint32(ptr) (*((uint32_t *)(ptr)))
> #define write_uint32(ptr, val) (*((uint32_t *)(ptr))) = val
> #endif
> 
> to:
> 
> #include 
> typedef struct SPICE_ATTR_PACKED {
> uint32_t v;
> } uint32_unaligned_t;
> #include 
> 
> #ifdef WORDS_BIGENDIAN
> #define read_uint32(ptr) ((uint32_t)SPICE_BYTESWAP32(((uint32_unaligned_t 
> *)(ptr))->v))

For int32, this generates
#define read_uint32(ptr) ((int32_t)SPICE_BYTESWAP32(((uint32_unaligned_t 
*)(ptr))->v))
rather than
#define read_uint32(ptr) ((int32_t)SPICE_BYTESWAP32(((int32_unaligned_t 
*)(ptr))->v))
I don't know if this is intentional?
(the change would be:

diff --git a/python_modules/demarshal.py b/python_modules/demarshal.py
index da87d44..241e620 100644
--- a/python_modules/demarshal.py
+++ b/python_modules/demarshal.py
@@ -63,7 +63,7 @@ def write_parser_helpers(writer):
 utype = "uint%d" % (size)
 type = "%sint%d" % (sign, size)
 swap = "SPICE_BYTESWAP%d" % size
-writer.macro("read_%s" % type, "ptr", "((%s_t)%s(((%s_unaligned_t 
*)(ptr))->v))" % (type, swap, utype))
+writer.macro("read_%s" % type, "ptr", "((%s_t)%s(((%s_unaligned_t 
*)(ptr))->v))" % (type, swap, type))
 writer.macro("write_%s" % type, "ptr, val", "((%s_unaligned_t 
*)(ptr))->v = %s((%s_t)val)" % (utype, swap, utype))
 writer.writeln("#else")
 for size in [16, 32, 64]:

Apart from this, looks good.

Christophe
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/spice-devel