Reimar Döffinger: > On 03.06.2019, at 14:07, Andreas Rheinhardt <andreas.rheinha...@gmail.com> > wrote: > >> Reimar Döffinger: >>> On 03.06.2019, at 00:37, Andreas Rheinhardt <andreas.rheinha...@gmail.com> >>> wrote: >>> >>>> Up until now, a temporary variable was used and initialized every time a >>>> value was read in CBS; if reading turned out to be successfull, this >>>> value was overwritten (without having ever been looked at) with the >>>> value read if reading was successfull; on failure the variable wasn't >>>> touched either. Therefore these initializations can be and have been >>>> removed. >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com> >>>> --- >>>> And? What did the ancient compilers say? >>> >>> Not sure how to read that, but some compilers probably produce "may be used >>> uninitialized" warnings after this change. >>> IMHO it would be better to need evidence of an advantage to remove variable >>> initialization for non-trivial code, even if they are unnecessary. Your >>> commit message doesn't seem to mention one at least. >>> But I am happy to let some maintainer have the last word. >> This comment refers to what Mark said in his review [1] of an earlier >> version of this patchset. He explicitly mentioned that (if his memory >> is right) an older compiler warned without the initializations. >> And I actually thought that there is no need for a further reason to >> remove unnecessary initializations despite the usual ones: Speed and >> code size. E.g. the size of my cbs_mpeg2.o went down from 30421 B to >> 29445 B after this patch; for cbs_h2645.o the numbers are 249605 B >> and 242245 B. These macros are used a lot (mostly indirectly via other >> macros); see the various cbs_*_syntax_template.c files for that. > > Well, your commit message did not mention that. > I kind of assumed that given the compiler does not emit a "may be used > uninitialized" warning it figured out that the initialization was unnecessary > and would produce identical code. > Maybe the compiler just is more conservative about that warning - or the code > size increase might be a bug worth reporting. But I admit finding out which > might be a bit too much work. > Either way I would appreciate mentioning explicitly in the commit message > code size and speed advantages if you have numbers already anyway. In case of ff_cbs_read_unsigned and ff_cbs_read_signed the compiler can't figure out whether the initialization is necessary or not because these functions live in different translation units. In the other cases, the compiler can figure it out, but GCC 8.3 often doesn't do so at O3: - For AV1, it detects for uvlc and subexp that the initializations are unnecessary, not for the the other ones. - For H2645, the uselessness of the initializations in the exponential golomb macros is not detected. - For VP9, it detects it for increment, but not for cbs_vp9_read_s. So let's really see what Mark (the maintainer) says.
- Andreas PS: When trying to find out what is different wrt. uvlc and subexp compared to the other ones I found out that both cbs_av1_read_leb128 and cbs_av1_read_subexp call ff_cbs_read_unsigned with a pointer to an unitialized variable as argument. _______________________________________________ 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".
