Re: [FFmpeg-devel] [PATCH v3] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.
>-Original Message- >From: ffmpeg-devel-boun...@ffmpeg.org [mailto:ffmpeg-devel-boun...@ffmpeg.org] >On Behalf Of >Reimar D?ffinger >Sent: Tuesday, July 30, 2019 2:54 AM >To: FFmpeg development discussions and patches >Subject: Re: [FFmpeg-devel] [PATCH v3] avutil/mips: Avoid instruction >exception caused by gssqc1/gslqc1. > >On 29.07.2019, at 11:54, "Shiyou Yin" wrote: >>> >> DECLARE_ALIGNED is defined in ' libavutil/mem.h ' and related to compiler. >> No matter mips or x86, >> it's definition is ' #define DECLARE_ALIGNED(n,t,v) t __attribute__ >> ((aligned (n))) v' when build >> with gcc or clang (Specific implementation within the compiler is not >> considered here.). >> In libavcodec/x86, DECLARE_ALIGNED is used to define 8/16/32-byte aligned >> variable too. > >The aligned attribute does not work reliably with stack variables in some >cases. >Compare with other code, I think you need to use LOCAL_ALIGNED_16 for the >stack variable. >Yes, it might work in your test even with DECLARE_ALIGNED, but it might not be >robust. You are right, LOCAL_ALIGNED_16 might be more robust. In v5 LOCAL_ALIGNED_16 is used for the stack variables . ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v3] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.
On 29.07.2019, at 11:54, "Shiyou Yin" wrote: >> > DECLARE_ALIGNED is defined in ' libavutil/mem.h ' and related to compiler. No > matter mips or x86, > it's definition is ' #define DECLARE_ALIGNED(n,t,v) t __attribute__ > ((aligned (n))) v' when build > with gcc or clang (Specific implementation within the compiler is not > considered here.). > In libavcodec/x86, DECLARE_ALIGNED is used to define 8/16/32-byte aligned > variable too. The aligned attribute does not work reliably with stack variables in some cases. Compare with other code, I think you need to use LOCAL_ALIGNED_16 for the stack variable. Yes, it might work in your test even with DECLARE_ALIGNED, but it might not be robust. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v3] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.
>-Original Message- >From: ffmpeg-devel-boun...@ffmpeg.org [mailto:ffmpeg-devel-boun...@ffmpeg.org] >On Behalf Of >Reimar D?ffinger >Sent: Sunday, July 28, 2019 6:37 AM >To: FFmpeg development discussions and patches >Subject: Re: [FFmpeg-devel] [PATCH v3] avutil/mips: Avoid instruction >exception caused by gssqc1/gslqc1. > >On 26.07.2019, at 07:18, Shiyou Yin wrote: > >> Ensure the address accesed by gssqc1/gslqc1 are 16-bits memory-aligned. > >Looks good to me if standard DECLARE_ALIGNED should work for stack on MIPS. >(on x86 it used to be possible for the stack pointer to only be 8-byte >aligned, in which case >DECLARE_ALIGNED could not actually provide 16-byte aligned stack variables, so >there were special >macros - I have not checked how it works nowadays or if MIPS has that issue). >Well, I guess regardless of that, this is better than before, so should be >fine to apply either way. > DECLARE_ALIGNED is defined in ' libavutil/mem.h ' and related to compiler. No matter mips or x86, it's definition is ' #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v' when build with gcc or clang (Specific implementation within the compiler is not considered here.). In libavcodec/x86, DECLARE_ALIGNED is used to define 8/16/32-byte aligned variable too. Here is a test on mips. 1) Defined variable with 'double temp[8];', and compiled with 'gcc -fstack-protector-all'. In this case, address of temp is not 16-byte aligned and gssqc1/gslqc1 caused instruction exception. 2) Replacing ' double temp[8];' with ' double __attribute__((aligned(16))) __back_temp[8];'. Address of temp is 16-byte aligned and gssqc1/gslqc1 works properly. There is a typo in the commit message and have revised in v4. Please help to apply. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH v3] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.
On 26.07.2019, at 07:18, Shiyou Yin wrote:
> Ensure the address accesed by gssqc1/gslqc1 are 16-bits memory-aligned.
Looks good to me if standard DECLARE_ALIGNED should work for stack on MIPS.
(on x86 it used to be possible for the stack pointer to only be 8-byte aligned,
in which case DECLARE_ALIGNED could not actually provide 16-byte aligned stack
variables, so there were special macros - I have not checked how it works
nowadays or if MIPS has that issue).
Well, I guess regardless of that, this is better than before, so should be fine
to apply either way.
> ---
> libavcodec/mips/simple_idct_mmi.c | 2 +-
> libavutil/mips/mmiutils.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/mips/simple_idct_mmi.c
> b/libavcodec/mips/simple_idct_mmi.c
> index 7f4bb74..73d797f 100644
> --- a/libavcodec/mips/simple_idct_mmi.c
> +++ b/libavcodec/mips/simple_idct_mmi.c
> @@ -39,7 +39,7 @@
> #define COL_SHIFT 20
> #define DC_SHIFT 3
>
> -DECLARE_ALIGNED(8, const int16_t, W_arr)[46] = {
> +DECLARE_ALIGNED(16, const int16_t, W_arr)[46] = {
> W4, W2, W4, W6,
> W1, W3, W5, W7,
> W4, W6, -W4, -W2,
> diff --git a/libavutil/mips/mmiutils.h b/libavutil/mips/mmiutils.h
> index 05f6b31..14b6d20 100644
> --- a/libavutil/mips/mmiutils.h
> +++ b/libavutil/mips/mmiutils.h
> @@ -205,7 +205,7 @@
> * backup register
> */
> #define BACKUP_REG \
> - double temp_backup_reg[8];\
> + DECLARE_ALIGNED(16, double, temp_backup_reg)[8]; \
> if (_MIPS_SIM == _ABI64) \
> __asm__ volatile ( \
> "gssqc1 $f25, $f24, 0x00(%[temp]) \n\t" \
> --
> 2.1.0
>
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH v3] avutil/mips: Avoid instruction exception caused by gssqc1/gslqc1.
Ensure the address accesed by gssqc1/gslqc1 are 16-bits memory-aligned.
---
libavcodec/mips/simple_idct_mmi.c | 2 +-
libavutil/mips/mmiutils.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/libavcodec/mips/simple_idct_mmi.c
b/libavcodec/mips/simple_idct_mmi.c
index 7f4bb74..73d797f 100644
--- a/libavcodec/mips/simple_idct_mmi.c
+++ b/libavcodec/mips/simple_idct_mmi.c
@@ -39,7 +39,7 @@
#define COL_SHIFT 20
#define DC_SHIFT 3
-DECLARE_ALIGNED(8, const int16_t, W_arr)[46] = {
+DECLARE_ALIGNED(16, const int16_t, W_arr)[46] = {
W4, W2, W4, W6,
W1, W3, W5, W7,
W4, W6, -W4, -W2,
diff --git a/libavutil/mips/mmiutils.h b/libavutil/mips/mmiutils.h
index 05f6b31..14b6d20 100644
--- a/libavutil/mips/mmiutils.h
+++ b/libavutil/mips/mmiutils.h
@@ -205,7 +205,7 @@
* backup register
*/
#define BACKUP_REG \
- double temp_backup_reg[8];\
+ DECLARE_ALIGNED(16, double, temp_backup_reg)[8]; \
if (_MIPS_SIM == _ABI64) \
__asm__ volatile ( \
"gssqc1 $f25, $f24, 0x00(%[temp]) \n\t" \
--
2.1.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
