| Message ID | 20171031121618.21297-1-tkoeppe@google.com |
|---|---|
| State | Superseded |
| Headers | show |
On Tue, Oct 31, 2017 at 12:16:18PM +0000, 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 > Static constants already were, via the define of DECLARE_ASM_CONST. > But DECLARE_ALIGNED does not add this attribute, and some of the variables > defined with it are const only used in inline assembly, and therefore > appeared dead. > --- > libavcodec/cabac.c | 2 +- > libavutil/mem.h | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/cabac.c b/libavcodec/cabac.c > index dd2b057c6d..7321b48901 100644 > --- a/libavcodec/cabac.c > +++ b/libavcodec/cabac.c > @@ -32,7 +32,7 @@ > #include "cabac.h" > #include "cabac_functions.h" > > -const uint8_t ff_h264_cabac_tables[512 + 4*2*64 + 4*64 + 63] = { > +DECLARE_ALIGNED(1, const uint8_t, ff_h264_cabac_tables)[512 + 4*2*64 + 4*64 + 63] = { > 9,8,7,7,6,6,6,6,5,5,5,5,5,5,5,5, > 4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4, > 3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3, > diff --git a/libavutil/mem.h b/libavutil/mem.h > index 527cd03191..c4ee11af58 100644 > --- a/libavutil/mem.h > +++ b/libavutil/mem.h > @@ -101,7 +101,7 @@ > #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v > #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v > #elif defined(__GNUC__) || defined(__clang__) > - #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 ? Marking all aligned variables as used makes it harder to spot unused variables leading to accumulation of cruft [...]
+Teresa, who drafted the patch initially. On 31 October 2017 at 15:38, Michael Niedermayer <michael@niedermayer.cc> wrote: > On Tue, Oct 31, 2017 at 12:16:18PM +0000, 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_w1111" and many more. > 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?
On Tue, Oct 31, 2017 at 03:52:14PM +0000, Thomas Köppe wrote: > +Teresa, who drafted the patch initially. > > On 31 October 2017 at 15:38, Michael Niedermayer <michael@niedermayer.cc> > wrote: > > > On Tue, Oct 31, 2017 at 12:16:18PM +0000, 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_w1111" 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
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 <michael@niedermayer.cc > wrote: > On Tue, Oct 31, 2017 at 03:52:14PM +0000, Thomas Köppe wrote: > > +Teresa, who drafted the patch initially. > > > > On 31 October 2017 at 15:38, Michael Niedermayer <michael@niedermayer.cc > > > > wrote: > > > > > On Tue, Oct 31, 2017 at 12:16:18PM +0000, 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_w1111" 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. >
Hi On Tue, Oct 31, 2017 at 04:29:18PM +0000, 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 > > Teresa > > On Tue, Oct 31, 2017 at 4:19 PM, Michael Niedermayer <michael@niedermayer.cc > > wrote: > > > On Tue, Oct 31, 2017 at 03:52:14PM +0000, Thomas Köppe wrote: > > > +Teresa, who drafted the patch initially. > > > > > > On 31 October 2017 at 15:38, Michael Niedermayer <michael@niedermayer.cc > > > > > > wrote: > > > > > > > On Tue, Oct 31, 2017 at 12:16:18PM +0000, 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_w1111" 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 | tejohnson@google.com | 408-460-2413 > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Tue, Oct 31, 2017 at 5:42 PM, Michael Niedermayer <michael@niedermayer.cc > wrote: > Hi > > On Tue, Oct 31, 2017 at 04:29:18PM +0000, 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 > <michael@niedermayer.cc > > > wrote: > > > > > On Tue, Oct 31, 2017 at 03:52:14PM +0000, Thomas Köppe wrote: > > > > +Teresa, who drafted the patch initially. > > > > > > > > On 31 October 2017 at 15:38, Michael Niedermayer > <michael@niedermayer.cc > > > > > > > > wrote: > > > > > > > > > On Tue, Oct 31, 2017 at 12:16:18PM +0000, 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_w1111" 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 | tejohnson@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 >
Hi On Wed, Nov 01, 2017 at 07:25:08AM -0700, Teresa Johnson wrote: > On Tue, Oct 31, 2017 at 5:42 PM, Michael Niedermayer <michael@niedermayer.cc > > wrote: > > > Hi > > > > On Tue, Oct 31, 2017 at 04:29:18PM +0000, 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? Ive no specific suggestion, but a 2nd macro seems a option thanks [...]
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 <tejohnson@google.com> wrote: > > > On Tue, Oct 31, 2017 at 5:42 PM, Michael Niedermayer < > michael@niedermayer.cc> wrote: > >> Hi >> >> On Tue, Oct 31, 2017 at 04:29:18PM +0000, 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 >> <michael@niedermayer.cc >> > > wrote: >> > >> > > On Tue, Oct 31, 2017 at 03:52:14PM +0000, Thomas Köppe wrote: >> > > > +Teresa, who drafted the patch initially. >> > > > >> > > > On 31 October 2017 at 15:38, Michael Niedermayer >> <michael@niedermayer.cc >> > > > >> > > > wrote: >> > > > >> > > > > On Tue, Oct 31, 2017 at 12:16:18PM +0000, 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_w1111" 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 | tejohnson@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 | tejohnson@google.com | > 408-460-2413 <(408)%20460-2413> >
Thank you, I'll package this up as a patch and send it as a separate mail. On 9 November 2017 at 11:52, Teresa Johnson <tejohnson@google.com> wrote: > 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 <https://critique.corp.google.com/#review/172133815>. > > Teresa > > On Wed, Nov 1, 2017 at 7:25 AM, Teresa Johnson <tejohnson@google.com> > wrote: > >> >> >> On Tue, Oct 31, 2017 at 5:42 PM, Michael Niedermayer < >> michael@niedermayer.cc> wrote: >> >>> Hi >>> >>> On Tue, Oct 31, 2017 at 04:29:18PM +0000, 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 >>> <michael@niedermayer.cc >>> > > wrote: >>> > >>> > > On Tue, Oct 31, 2017 at 03:52:14PM +0000, Thomas Köppe wrote: >>> > > > +Teresa, who drafted the patch initially. >>> > > > >>> > > > On 31 October 2017 at 15:38, Michael Niedermayer >>> <michael@niedermayer.cc >>> > > > >>> > > > wrote: >>> > > > >>> > > > > On Tue, Oct 31, 2017 at 12:16:18PM +0000, 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_w1111" 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 | tejohnson@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 | tejohnson@google.com | >> 408-460-2413 <(408)%20460-2413> >> > > > > -- > Teresa Johnson | Software Engineer | tejohnson@google.com | > 408-460-2413 >
diff --git a/libavcodec/cabac.c b/libavcodec/cabac.c index dd2b057c6d..7321b48901 100644 --- a/libavcodec/cabac.c +++ b/libavcodec/cabac.c @@ -32,7 +32,7 @@ #include "cabac.h" #include "cabac_functions.h" -const uint8_t ff_h264_cabac_tables[512 + 4*2*64 + 4*64 + 63] = { +DECLARE_ALIGNED(1, const uint8_t, ff_h264_cabac_tables)[512 + 4*2*64 + 4*64 + 63] = { 9,8,7,7,6,6,6,6,5,5,5,5,5,5,5,5, 4,4,4,4,4,4,4,4,4,4,4,4,4,4,4,4, 3,3,3,3,3,3,3,3,3,3,3,3,3,3,3,3, diff --git a/libavutil/mem.h b/libavutil/mem.h index 527cd03191..c4ee11af58 100644 --- a/libavutil/mem.h +++ b/libavutil/mem.h @@ -101,7 +101,7 @@ #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (FFMIN(n, 16)))) v #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (FFMIN(n, 16)))) v #elif defined(__GNUC__) || defined(__clang__) - #define DECLARE_ALIGNED(n,t,v) t __attribute__ ((aligned (n))) v + #define DECLARE_ALIGNED(n,t,v) t av_used __attribute__ ((aligned (n))) v #define DECLARE_ASM_CONST(n,t,v) static const t av_used __attribute__ ((aligned (n))) v #elif defined(_MSC_VER) #define DECLARE_ALIGNED(n,t,v) __declspec(align(n)) t v