| Message ID | 20190602223730.10992-2-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 03.06.2019, at 00:37, Andreas Rheinhardt <andreas.rheinhardt@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.rheinhardt@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.
Reimar Döffinger: > On 03.06.2019, at 00:37, Andreas Rheinhardt <andreas.rheinhardt@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.rheinhardt@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. - Andreas
On 03.06.2019, at 14:07, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > Reimar Döffinger: >> On 03.06.2019, at 00:37, Andreas Rheinhardt <andreas.rheinhardt@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.rheinhardt@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. Thanks! Reimar
Reimar Döffinger: > On 03.06.2019, at 14:07, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > >> Reimar Döffinger: >>> On 03.06.2019, at 00:37, Andreas Rheinhardt <andreas.rheinhardt@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.rheinhardt@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.
diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c index eb2d03ef43..e31fc0c411 100644 --- a/libavcodec/cbs_av1.c +++ b/libavcodec/cbs_av1.c @@ -574,7 +574,7 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc) #define RWContext GetBitContext #define xf(width, name, var, range_min, range_max, subs, ...) do { \ - uint32_t value = range_min; \ + uint32_t value; \ CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), \ &value, range_min, range_max)); \ @@ -582,7 +582,7 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc) } while (0) #define xsu(width, name, var, subs, ...) do { \ - int32_t value = 0; \ + int32_t value; \ CHECK(ff_cbs_read_signed(ctx, rw, width, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), &value, \ MIN_INT_BITS(width), \ @@ -591,27 +591,27 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc) } while (0) #define uvlc(name, range_min, range_max) do { \ - uint32_t value = range_min; \ + uint32_t value; \ CHECK(cbs_av1_read_uvlc(ctx, rw, #name, \ &value, range_min, range_max)); \ current->name = value; \ } while (0) #define ns(max_value, name, subs, ...) do { \ - uint32_t value = 0; \ + uint32_t value; \ CHECK(cbs_av1_read_ns(ctx, rw, max_value, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), &value)); \ current->name = value; \ } while (0) #define increment(name, min, max) do { \ - uint32_t value = 0; \ + uint32_t value; \ CHECK(cbs_av1_read_increment(ctx, rw, min, max, #name, &value)); \ current->name = value; \ } while (0) #define subexp(name, max, subs, ...) do { \ - uint32_t value = 0; \ + uint32_t value; \ CHECK(cbs_av1_read_subexp(ctx, rw, max, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), &value)); \ current->name = value; \ @@ -629,7 +629,7 @@ static size_t cbs_av1_get_payload_bytes_left(GetBitContext *gbc) } while (0) #define leb128(name) do { \ - uint64_t value = 0; \ + uint64_t value; \ CHECK(cbs_av1_read_leb128(ctx, rw, #name, &value)); \ current->name = value; \ } while (0) diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c index 319202fc48..8655d8015e 100644 --- a/libavcodec/cbs_h2645.c +++ b/libavcodec/cbs_h2645.c @@ -290,28 +290,28 @@ static int cbs_write_se_golomb(CodedBitstreamContext *ctx, PutBitContext *pbc, #define RWContext GetBitContext #define xu(width, name, var, range_min, range_max, subs, ...) do { \ - uint32_t value = range_min; \ + uint32_t value; \ CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), \ &value, range_min, range_max)); \ var = value; \ } while (0) #define xue(name, var, range_min, range_max, subs, ...) do { \ - uint32_t value = range_min; \ + uint32_t value; \ CHECK(cbs_read_ue_golomb(ctx, rw, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), \ &value, range_min, range_max)); \ var = value; \ } while (0) #define xi(width, name, var, range_min, range_max, subs, ...) do { \ - int32_t value = range_min; \ + int32_t value; \ CHECK(ff_cbs_read_signed(ctx, rw, width, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), \ &value, range_min, range_max)); \ var = value; \ } while (0) #define xse(name, var, range_min, range_max, subs, ...) do { \ - int32_t value = range_min; \ + int32_t value; \ CHECK(cbs_read_se_golomb(ctx, rw, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), \ &value, range_min, range_max)); \ diff --git a/libavcodec/cbs_jpeg.c b/libavcodec/cbs_jpeg.c index 5a72f0e2e7..2830f99a31 100644 --- a/libavcodec/cbs_jpeg.c +++ b/libavcodec/cbs_jpeg.c @@ -45,7 +45,7 @@ #define FUNC(name) cbs_jpeg_read_ ## name #define xu(width, name, range_min, range_max, subs, ...) do { \ - uint32_t value = range_min; \ + uint32_t value; \ CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), \ &value, range_min, range_max)); \ diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c index 5e971f3a54..ede6e806e1 100644 --- a/libavcodec/cbs_mpeg2.c +++ b/libavcodec/cbs_mpeg2.c @@ -57,7 +57,7 @@ #define RWContext GetBitContext #define xui(width, name, var, range_min, range_max, subs, ...) do { \ - uint32_t value = 0; \ + uint32_t value; \ CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), \ &value, range_min, range_max)); \ diff --git a/libavcodec/cbs_vp9.c b/libavcodec/cbs_vp9.c index 0b5f137ed8..fea0c8f17e 100644 --- a/libavcodec/cbs_vp9.c +++ b/libavcodec/cbs_vp9.c @@ -267,14 +267,14 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc, #define RWContext GetBitContext #define xf(width, name, var, subs, ...) do { \ - uint32_t value = 0; \ + uint32_t value; \ CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), \ &value, 0, (1 << width) - 1)); \ var = value; \ } while (0) #define xs(width, name, var, subs, ...) do { \ - int32_t value = 0; \ + int32_t value; \ CHECK(cbs_vp9_read_s(ctx, rw, width, #name, \ SUBSCRIPTS(subs, __VA_ARGS__), &value)); \ var = value; \ @@ -282,7 +282,7 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc, #define increment(name, min, max) do { \ - uint32_t value = 0; \ + uint32_t value; \ CHECK(cbs_vp9_read_increment(ctx, rw, min, max, #name, &value)); \ current->name = value; \ } while (0) @@ -315,7 +315,7 @@ static int cbs_vp9_write_le(CodedBitstreamContext *ctx, PutBitContext *pbc, } while (0) #define fixed(width, name, value) do { \ - av_unused uint32_t fixed_value = value; \ + av_unused uint32_t fixed_value; \ CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \ 0, &fixed_value, value, value)); \ } while (0)
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.rheinhardt@gmail.com> --- And? What did the ancient compilers say? libavcodec/cbs_av1.c | 14 +++++++------- libavcodec/cbs_h2645.c | 8 ++++---- libavcodec/cbs_jpeg.c | 2 +- libavcodec/cbs_mpeg2.c | 2 +- libavcodec/cbs_vp9.c | 8 ++++---- 5 files changed, 17 insertions(+), 17 deletions(-)