Re: [FFmpeg-devel] [PATCH] Fix missing used attribute for inline assembly variables
I implemented this change to add a new macro. I tried to find the variables used in MANGLE and change those to use the new DECLARE_ASM_ALIGNED, and the build succeeds with these changes. New changes are in cl/172133815. Teresa On Wed, Nov 1, 2017 at 7:25 AM, Teresa Johnson <tejohn...@google.com> wrote: > > > On Tue, Oct 31, 2017 at 5:42 PM, Michael Niedermayer < > mich...@niedermayer.cc> wrote: > >> Hi >> >> On Tue, Oct 31, 2017 at 04:29:18PM +, Teresa Johnson wrote: >> > It's needed for the same reason the used attribute was already added to >> the >> > "static const" variables. For those, when building with just -O2, they >> > could be removed by optimization since they had local (file) scope, and >> we >> > couldn't see the uses in the inline assembly (without the used >> attribute). >> > With ThinLTO we have whole program scope, so even though they are >> > non-static and have global scope we can do dead code elimination on >> them if >> > we don't see that they are used. >> >> currently we add "used" to DECLARE_ASM_CONST() >> which is specific to inline asm use. >> >> DECLARE_ALIGNED() is not specific to use in asm. >> >> For DECLARE_ASM_CONST() the "used" is unneeded only in the subset of >> inline asm cases where it is accessed through the asm operands like: >> __asm__ volatile( >> "movq %0, %%mm7\n\t" >> "movq %1, %%mm6\n\t" >> ::"m"(red_16mask),"m"(green_16mask)); >> >> The compiler has full vissibility here of the access and if it removes >> it its a compiler bug. >> >> this is compared to code like: >> "pand "MANGLE(mask24l)", %%mm0\n\t" >> >> Here the compiler has no easy vissibility of the use, it is part of >> the asm string. >> >> and comparing this to DECLARE_ALIGNED(), the big difference is >> that DECLARE_ALIGNED() is used by plain C code which never should need >> "used". So adding "used" to DECLARE_ALIGNED() would add alot more >> "unused detection overriding" than what is needed >> > > Perhaps then an additional macro is needed for variables that are > currently DECLARED_ALIGNED but used by MANGLE, which adds the used > attribute. What do you suggest? > Teresa > > >> >> >> >> > >> > Teresa >> > >> > On Tue, Oct 31, 2017 at 4:19 PM, Michael Niedermayer >> <mich...@niedermayer.cc >> > > wrote: >> > >> > > On Tue, Oct 31, 2017 at 03:52:14PM +, Thomas Köppe wrote: >> > > > +Teresa, who drafted the patch initially. >> > > > >> > > > On 31 October 2017 at 15:38, Michael Niedermayer >> <mich...@niedermayer.cc >> > > > >> > > > wrote: >> > > > >> > > > > On Tue, Oct 31, 2017 at 12:16:18PM +, Thomas Köppe wrote: >> > > > > > Variables used in inline assembly need to be marked with >> > > > > attribute((used)). >> > > > > >> > > > > This should not be the case. >> > > > > Variables referenced through in/out operands should not need that. >> > > > > Only ones accessed directly bypassing operands should need this >> > > > > >> > > > >> > > > I've added Teresa to the thread, who initially analyzed the >> problem. (For >> > > > background on ThinLTO, see here cppcon talk: >> > > > https://www.youtube.com/watch?v=p9nH2vZ2mNo.) >> > > > >> > > > [...] >> > > > > > -#define DECLARE_ALIGNED(n,t,v) t __attribute__ >> ((aligned >> > > (n))) >> > > > > v >> > > > > > +#define DECLARE_ALIGNED(n,t,v) t av_used __attribute__ >> > > > > ((aligned (n))) v >> > > > > >> > > > > which variables actually are affected/ need this ? >> > > > > >> > > > >> > > > Without this annotation, and when compiling and linking with >> ThinLTO, I >> > > get >> > > > an undefined reference to "ff_h264_cabac_tables", and also to >> > > > "ff_pw_96", "ff_pw_53", >> > > > "ff_pw_42", "ff_w" and many more. >> > > >> > > i guess these are all accessed directly, like thr
Re: [FFmpeg-devel] [PATCH] Fix missing used attribute for inline assembly variables
On Tue, Oct 31, 2017 at 5:42 PM, Michael Niedermayer <mich...@niedermayer.cc > wrote: > Hi > > On Tue, Oct 31, 2017 at 04:29:18PM +, Teresa Johnson wrote: > > It's needed for the same reason the used attribute was already added to > the > > "static const" variables. For those, when building with just -O2, they > > could be removed by optimization since they had local (file) scope, and > we > > couldn't see the uses in the inline assembly (without the used > attribute). > > With ThinLTO we have whole program scope, so even though they are > > non-static and have global scope we can do dead code elimination on them > if > > we don't see that they are used. > > currently we add "used" to DECLARE_ASM_CONST() > which is specific to inline asm use. > > DECLARE_ALIGNED() is not specific to use in asm. > > For DECLARE_ASM_CONST() the "used" is unneeded only in the subset of > inline asm cases where it is accessed through the asm operands like: > __asm__ volatile( > "movq %0, %%mm7\n\t" > "movq %1, %%mm6\n\t" > ::"m"(red_16mask),"m"(green_16mask)); > > The compiler has full vissibility here of the access and if it removes > it its a compiler bug. > > this is compared to code like: > "pand "MANGLE(mask24l)", %%mm0\n\t" > > Here the compiler has no easy vissibility of the use, it is part of > the asm string. > > and comparing this to DECLARE_ALIGNED(), the big difference is > that DECLARE_ALIGNED() is used by plain C code which never should need > "used". So adding "used" to DECLARE_ALIGNED() would add alot more > "unused detection overriding" than what is needed > Perhaps then an additional macro is needed for variables that are currently DECLARED_ALIGNED but used by MANGLE, which adds the used attribute. What do you suggest? Teresa > > > > > > > Teresa > > > > On Tue, Oct 31, 2017 at 4:19 PM, Michael Niedermayer > <mich...@niedermayer.cc > > > wrote: > > > > > On Tue, Oct 31, 2017 at 03:52:14PM +, Thomas Köppe wrote: > > > > +Teresa, who drafted the patch initially. > > > > > > > > On 31 October 2017 at 15:38, Michael Niedermayer > <mich...@niedermayer.cc > > > > > > > > wrote: > > > > > > > > > On Tue, Oct 31, 2017 at 12:16:18PM +, Thomas Köppe wrote: > > > > > > Variables used in inline assembly need to be marked with > > > > > attribute((used)). > > > > > > > > > > This should not be the case. > > > > > Variables referenced through in/out operands should not need that. > > > > > Only ones accessed directly bypassing operands should need this > > > > > > > > > > > > > I've added Teresa to the thread, who initially analyzed the problem. > (For > > > > background on ThinLTO, see here cppcon talk: > > > > https://www.youtube.com/watch?v=p9nH2vZ2mNo.) > > > > > > > > [...] > > > > > > -#define DECLARE_ALIGNED(n,t,v) t __attribute__ > ((aligned > > > (n))) > > > > > v > > > > > > +#define DECLARE_ALIGNED(n,t,v) t av_used __attribute__ > > > > > ((aligned (n))) v > > > > > > > > > > which variables actually are affected/ need this ? > > > > > > > > > > > > > Without this annotation, and when compiling and linking with > ThinLTO, I > > > get > > > > an undefined reference to "ff_h264_cabac_tables", and also to > > > > "ff_pw_96", "ff_pw_53", > > > > "ff_pw_42", "ff_w" and many more. > > > > > > i guess these are all accessed directly, like through MANGLE() > > > > > > > > > > > > > > > > > > > Marking all aligned variables as used makes it harder to spot > unused > > > > > variables leading to accumulation of cruft > > > > > > > > > > > > > I see. Maybe we can annotate only the affected variables more > granularly? > > > > ___ > > > > ffmpeg-devel mailing list > > > > ffmpeg-devel@ffmpeg.org > > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > -- > > > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7 > 87040B0FAB > > > > > > Avoid a single point of failure, be that a person or equipment. > > > > > > > > > > > -- > > Teresa Johnson | Software Engineer | tejohn...@google.com | > 408-460-2413 > > ___ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > I am the wisest man alive, for I know one thing, and that is that I know > nothing. -- Socrates > -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH] Fix missing used attribute for inline assembly variables
It's needed for the same reason the used attribute was already added to the "static const" variables. For those, when building with just -O2, they could be removed by optimization since they had local (file) scope, and we couldn't see the uses in the inline assembly (without the used attribute). With ThinLTO we have whole program scope, so even though they are non-static and have global scope we can do dead code elimination on them if we don't see that they are used. Teresa On Tue, Oct 31, 2017 at 4:19 PM, Michael Niedermayer <mich...@niedermayer.cc > wrote: > On Tue, Oct 31, 2017 at 03:52:14PM +, Thomas Köppe wrote: > > +Teresa, who drafted the patch initially. > > > > On 31 October 2017 at 15:38, Michael Niedermayer <mich...@niedermayer.cc > > > > wrote: > > > > > On Tue, Oct 31, 2017 at 12:16:18PM +, Thomas Köppe wrote: > > > > Variables used in inline assembly need to be marked with > > > attribute((used)). > > > > > > This should not be the case. > > > Variables referenced through in/out operands should not need that. > > > Only ones accessed directly bypassing operands should need this > > > > > > > I've added Teresa to the thread, who initially analyzed the problem. (For > > background on ThinLTO, see here cppcon talk: > > https://www.youtube.com/watch?v=p9nH2vZ2mNo.) > > > > [...] > > > > -#define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned > (n))) > > > v > > > > +#define DECLARE_ALIGNED(n,t,v) t av_used __attribute__ > > > ((aligned (n))) v > > > > > > which variables actually are affected/ need this ? > > > > > > > Without this annotation, and when compiling and linking with ThinLTO, I > get > > an undefined reference to "ff_h264_cabac_tables", and also to > > "ff_pw_96", "ff_pw_53", > > "ff_pw_42", "ff_w" and many more. > > i guess these are all accessed directly, like through MANGLE() > > > > > > > > > Marking all aligned variables as used makes it harder to spot unused > > > variables leading to accumulation of cruft > > > > > > > I see. Maybe we can annotate only the affected variables more granularly? > > ___ > > ffmpeg-devel mailing list > > ffmpeg-devel@ffmpeg.org > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > -- > Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB > > Avoid a single point of failure, be that a person or equipment. > -- Teresa Johnson | Software Engineer | tejohn...@google.com | 408-460-2413 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
