intreadwrite: Use the __unaligned keyword on MSVC for ARM and x86_64

Message ID 1470035380-5728-1-git-send-email-martin@martin.st
State Committed
Headers show

Commit Message

Martin Storsjö Aug. 1, 2016, 7:09 a.m.
AV_WN64 is meant for unaligned data, but the existing av_alias* unions
(without a definition for the av_alias attribute - we don't have one
for MSVC) indicate to the compiler that they would have sufficient
alignment for normal access, i.e. the compiler is free to assume
8 byte alignment.

On ARM, this makes sure that AV_WN64 (or two consecutive AV_WN32) is
done with two str instructions instead of one strd.
---
Or should we define av_alias to __unaligned in attributes.h instead?
---
 libavutil/intreadwrite.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Luca Barbato Aug. 1, 2016, 8:10 a.m. | #1
On 01/08/16 09:09, Martin Storsjö wrote:
> AV_WN64 is meant for unaligned data, but the existing av_alias* unions
> (without a definition for the av_alias attribute - we don't have one
> for MSVC) indicate to the compiler that they would have sufficient
> alignment for normal access, i.e. the compiler is free to assume
> 8 byte alignment.
> 
> On ARM, this makes sure that AV_WN64 (or two consecutive AV_WN32) is
> done with two str instructions instead of one strd.
> ---
> Or should we define av_alias to __unaligned in attributes.h instead?
> ---
>  libavutil/intreadwrite.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavutil/intreadwrite.h b/libavutil/intreadwrite.h
> index f77fd60..0ba9254 100644
> --- a/libavutil/intreadwrite.h
> +++ b/libavutil/intreadwrite.h
> @@ -197,6 +197,11 @@ union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
>  #   define AV_RN(s, p) (*((const __unaligned uint##s##_t*)(p)))
>  #   define AV_WN(s, p, v) (*((__unaligned uint##s##_t*)(p)) = (v))
>  
> +#elif defined(_MSC_VER) && (defined(_M_ARM) || defined(_M_X64))
> +
> +#   define AV_RN(s, p) (*((const __unaligned uint##s##_t*)(p)))
> +#   define AV_WN(s, p, v) (*((__unaligned uint##s##_t*)(p)) = (v))
> +
>  #elif AV_HAVE_FAST_UNALIGNED
>  
>  #   define AV_RN(s, p) (((const av_alias##s*)(p))->u##s)
> 

Maybe add the additional conditions to the __DECC block?

lu
Martin Storsjö Aug. 1, 2016, 8:12 a.m. | #2
On Mon, 1 Aug 2016, Luca Barbato wrote:

> On 01/08/16 09:09, Martin Storsjö wrote:
>> AV_WN64 is meant for unaligned data, but the existing av_alias* unions
>> (without a definition for the av_alias attribute - we don't have one
>> for MSVC) indicate to the compiler that they would have sufficient
>> alignment for normal access, i.e. the compiler is free to assume
>> 8 byte alignment.
>>
>> On ARM, this makes sure that AV_WN64 (or two consecutive AV_WN32) is
>> done with two str instructions instead of one strd.
>> ---
>> Or should we define av_alias to __unaligned in attributes.h instead?
>> ---
>>  libavutil/intreadwrite.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/libavutil/intreadwrite.h b/libavutil/intreadwrite.h
>> index f77fd60..0ba9254 100644
>> --- a/libavutil/intreadwrite.h
>> +++ b/libavutil/intreadwrite.h
>> @@ -197,6 +197,11 @@ union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
>>  #   define AV_RN(s, p) (*((const __unaligned uint##s##_t*)(p)))
>>  #   define AV_WN(s, p, v) (*((__unaligned uint##s##_t*)(p)) = (v))
>>
>> +#elif defined(_MSC_VER) && (defined(_M_ARM) || defined(_M_X64))
>> +
>> +#   define AV_RN(s, p) (*((const __unaligned uint##s##_t*)(p)))
>> +#   define AV_WN(s, p, v) (*((__unaligned uint##s##_t*)(p)) = (v))
>> +
>>  #elif AV_HAVE_FAST_UNALIGNED
>>
>>  #   define AV_RN(s, p) (((const av_alias##s*)(p))->u##s)
>>
>
> Maybe add the additional conditions to the __DECC block?

I guess that would work as well, although it feels more like coincidence 
that they happen to be similar. Or maybe not (since this MSVC attribute 
came from itanium first I think). Since they're quite different compilers 
I'd rather have separate blocks though, since any potential future 
adjustments might not apply to both.

// Martin
Luca Barbato Aug. 1, 2016, 9:04 a.m. | #3
On 01/08/16 10:12, Martin Storsjö wrote:
> I guess that would work as well, although it feels more like coincidence
> that they happen to be similar. Or maybe not (since this MSVC attribute
> came from itanium first I think). Since they're quite different
> compilers I'd rather have separate blocks though, since any potential
> future adjustments might not apply to both.

Ok as is then =)

lu
Janne Grunau Aug. 1, 2016, 7:31 p.m. | #4
On 2016-08-01 10:09:40 +0300, Martin Storsjö wrote:
> AV_WN64 is meant for unaligned data, but the existing av_alias* unions
> (without a definition for the av_alias attribute - we don't have one
> for MSVC) indicate to the compiler that they would have sufficient
> alignment for normal access, i.e. the compiler is free to assume
> 8 byte alignment.
> 
> On ARM, this makes sure that AV_WN64 (or two consecutive AV_WN32) is
> done with two str instructions instead of one strd.
> ---
> Or should we define av_alias to __unaligned in attributes.h instead?
> ---
>  libavutil/intreadwrite.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/libavutil/intreadwrite.h b/libavutil/intreadwrite.h
> index f77fd60..0ba9254 100644
> --- a/libavutil/intreadwrite.h
> +++ b/libavutil/intreadwrite.h
> @@ -197,6 +197,11 @@ union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
>  #   define AV_RN(s, p) (*((const __unaligned uint##s##_t*)(p)))
>  #   define AV_WN(s, p, v) (*((__unaligned uint##s##_t*)(p)) = (v))
>  
> +#elif defined(_MSC_VER) && (defined(_M_ARM) || defined(_M_X64))

does MSVC only targets armv6 and later? if not the condition needs a '&& 
AV_HAVE_FAST_UNALIGNED'

> +
> +#   define AV_RN(s, p) (*((const __unaligned uint##s##_t*)(p)))
> +#   define AV_WN(s, p, v) (*((__unaligned uint##s##_t*)(p)) = (v))
> +

otherwise ok

Janne
Martin Storsjö Aug. 1, 2016, 7:35 p.m. | #5
On Mon, 1 Aug 2016, Janne Grunau wrote:

> On 2016-08-01 10:09:40 +0300, Martin Storsjö wrote:
>> AV_WN64 is meant for unaligned data, but the existing av_alias* unions
>> (without a definition for the av_alias attribute - we don't have one
>> for MSVC) indicate to the compiler that they would have sufficient
>> alignment for normal access, i.e. the compiler is free to assume
>> 8 byte alignment.
>>
>> On ARM, this makes sure that AV_WN64 (or two consecutive AV_WN32) is
>> done with two str instructions instead of one strd.
>> ---
>> Or should we define av_alias to __unaligned in attributes.h instead?
>> ---
>>  libavutil/intreadwrite.h | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/libavutil/intreadwrite.h b/libavutil/intreadwrite.h
>> index f77fd60..0ba9254 100644
>> --- a/libavutil/intreadwrite.h
>> +++ b/libavutil/intreadwrite.h
>> @@ -197,6 +197,11 @@ union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
>>  #   define AV_RN(s, p) (*((const __unaligned uint##s##_t*)(p)))
>>  #   define AV_WN(s, p, v) (*((__unaligned uint##s##_t*)(p)) = (v))
>>
>> +#elif defined(_MSC_VER) && (defined(_M_ARM) || defined(_M_X64))
>
> does MSVC only targets armv6 and later? if not the condition needs a '&&
> AV_HAVE_FAST_UNALIGNED'

Current MSVC only targets armv7/thumb2, but older ones (2008) that support 
WinCE target armv4. I'll amend it with the && AV_HAVE_FAST_UNALIGNED just 
to be clear though.

>> +
>> +#   define AV_RN(s, p) (*((const __unaligned uint##s##_t*)(p)))
>> +#   define AV_WN(s, p, v) (*((__unaligned uint##s##_t*)(p)) = (v))
>> +
>
> otherwise ok

Thanks

// Martin

Patch

diff --git a/libavutil/intreadwrite.h b/libavutil/intreadwrite.h
index f77fd60..0ba9254 100644
--- a/libavutil/intreadwrite.h
+++ b/libavutil/intreadwrite.h
@@ -197,6 +197,11 @@  union unaligned_16 { uint16_t l; } __attribute__((packed)) av_alias;
 #   define AV_RN(s, p) (*((const __unaligned uint##s##_t*)(p)))
 #   define AV_WN(s, p, v) (*((__unaligned uint##s##_t*)(p)) = (v))
 
+#elif defined(_MSC_VER) && (defined(_M_ARM) || defined(_M_X64))
+
+#   define AV_RN(s, p) (*((const __unaligned uint##s##_t*)(p)))
+#   define AV_WN(s, p, v) (*((__unaligned uint##s##_t*)(p)) = (v))
+
 #elif AV_HAVE_FAST_UNALIGNED
 
 #   define AV_RN(s, p) (((const av_alias##s*)(p))->u##s)