Summary: | bitfield check causes maybe-uninitialized warning | ||
---|---|---|---|
Product: | gcc | Reporter: | Arnd Bergmann <arnd> |
Component: | tree-optimization | Assignee: | Not yet assigned to anyone <unassigned> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | dimhen, jakub, jeffreyalaw, law, msebor |
Priority: | P3 | Keywords: | diagnostic, missed-optimization |
Version: | 8.0.1 | ||
Target Milestone: | 13.0 | ||
See Also: | https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99919 | ||
Host: | Target: | ||
Build: | Known to work: | ||
Known to fail: | 10.2.0, 11.0, 8.3.0, 9.3.0 | Last reconfirmed: | 2021-04-05 00:00:00 |
Bug Depends on: | |||
Bug Blocks: | 24639 |
Description
Arnd Bergmann
2018-04-09 12:17:04 UTC
Hmm, this looks like a missed jump threading to me - possibly DOM being confused about the BIT_FIELD_REF and the & 3 that appears with the bitfield test due to my much beloved optimize_bit_field_compare: if ((BIT_FIELD_REF <tick_nohz_idle_exit_ts, 8, 0> & 3) != 0) { now = ktime_get (); } if ((BIT_FIELD_REF <tick_nohz_idle_exit_ts, 8, 0> & 1) != 0) { __tick_nohz_idle_restart_tick (now); } vs. if (tick_nohz_idle_exit_ts.idle_active != 0 || tick_nohz_idle_exit_ts.tick_stopped != 0) { now = ktime_get (); } if (tick_nohz_idle_exit_ts.tick_stopped != 0) { __tick_nohz_idle_restart_tick (now); } That is, jump-threading fails to do if (x & 3) ... else thread --> if (x & 1) ... else <--- here with the extra complication of that clobbering ktime_get () call on the true path and x being memory references where DOM sees it. PRE "cleans" that up on the interesting path but still late DOM doesn't perform the threading: <bb 2> [local count: 1073741825]: _1 = BIT_FIELD_REF <tick_nohz_idle_exit_ts, 8, 0>; _2 = _1 & 3; if (_2 != 0) goto <bb 3>; [33.00%] else goto <bb 4>; [67.00%] <bb 3> [local count: 354334802]: now_11 = ktime_get (); pretmp_14 = BIT_FIELD_REF <tick_nohz_idle_exit_ts, 8, 0>; <bb 4> [local count: 1073741825]: # now_5 = PHI <now_11(3), now_9(D)(2)> # prephitmp_15 = PHI <pretmp_14(3), _1(2)> _4 = prephitmp_15 & 1; if (_4 != 0) goto <bb 5>; [33.00%] else else goto <bb 6>; [67.00%] <bb 5> [local count: 354334802]: __tick_nohz_idle_restart_tick (now_5); <bb 6> [local count: 1073741825]: Maybe uninit just doesn't understand that in: <bb 2> [local count: 1073741825]: _1 = BIT_FIELD_REF <tick_nohz_idle_exit_ts, 8, 0>; _2 = _1 & 3; if (_2 != 0) goto <bb 3>; [33.00%] else goto <bb 7>; [67.00%] <bb 7> [local count: 719407025]: goto <bb 4>; [100.00%] <bb 3> [local count: 354334802]: now_11 = ktime_get (); pretmp_14 = BIT_FIELD_REF <tick_nohz_idle_exit_ts, 8, 0>; <bb 4> [local count: 1073741825]: # now_5 = PHI <now_11(3), now_9(D)(7)> # prephitmp_15 = PHI <pretmp_14(3), _1(7)> _4 = prephitmp_15 & 1; if (_4 != 0) goto <bb 5>; [33.00%] else goto <bb 8>; [67.00%] <bb 8> [local count: 719407025]: goto <bb 6>; [100.00%] <bb 5> [local count: 354334802]: __tick_nohz_idle_restart_tick (now_5); [tail call] <bb 6> [local count: 1073741825]: return; if (_1 & 3) != 0 it implies (_1 & 1) != 0. Though, if I change the testcase to: void tick_nohz_idle_exit(void) { long now; struct tick_sched t = tick_nohz_idle_exit_ts; if (t.idle_active || t.tick_stopped) now = ktime_get(); if (t.tick_stopped) __tick_nohz_idle_restart_tick(now); } so that it doesn't have to reread tick_nohz_idle_exit_ts.tick_stopped after the ktime_get call, it doesn't warn anymore. In that case now is always initialized though, so it shouldn't matter what value we read for the uninit warning. Note, r142464 still doesn't warn, r142514 does, probably r142484 enabled the (premature) BIT_FIELD_REF comparison optimization in fold-const.c. FWIW, there's another similar bug where DOM doesn't do a particularly good job at tracking the state of objects implied the results of logical operations which in turn causes missed optimizations. I've poked at it a few times but haven't come up with anything good to address the deficiency. It's on the Wuninitialized tracker, so I'll certainly take a look over the coming months. I found that older versions (gcc-5 and before) did not warn when the type gets changed to bitfield of '_Bool' rather than 'unsigned int'. It seems that this was only because they tested each bit separately in the _Bool case rather than combining the first two comparisons into a &3 mask. So I went and found pr68548 which is related, but clearly different. Just thinking out loud here, I think we can pick up this missed jump thread by recording _1 & 1 == 0 on the false edge of the first conditional (when we know that _1 & 3 == 0). It doesn't scale well in the general case, but when there's just a few bits in the mask it might be reasonable. That ought to allow us to determine that _4 == 0 when bb4 is reached via bb2 and thread the jump. Not planning to look at any implementation for gcc-8 though. Reconfirmed with GCC 11 with the ever-so-slightly slightly simplified program below and enhanced output. The warning has been issued since at least GCC 4.1 and so is not a regression. $ cat pr85301.c && gcc -O2 -S -Wall -DUSE_BITFIELD pr85301.c struct A { #ifdef USE_BITFIELD unsigned i : 1; unsigned j : 1; #else unsigned i; unsigned j; #endif }; int z, f (void); struct A a; void h (void) { int y; if (a.j || a.i) y = f (); if (a.i) z = y; } pr85301.c: In function ‘h’: pr85301.c:24:7: warning: ‘y’ may be used uninitialized in this function [-Wmaybe-uninitialized] 24 | z = y; | ~~^~~ pr85301.c:18:7: note: when ‘!(((unsigned char*)&a)[0] & 3)’ 18 | int y; | ^ pr85301.c:18:7: note: used when ‘prephitmp_15 = PHI <_1(7), pretmp_14(3)> & 1’ pr85301.c:18:7: note: ‘y’ was declared here Jump threading and other optimization opportunities aside (I have raised pr99918 for one of those), there does also seem to be a limitation in the warning code in that it doesn't understand BIT_FIELD_REF expressions. The warning sees the IL below from which it should be able to determine that y's use is conditional on its definition (i.e., the use predicate a strict subset of the predicate controlling the definition). void h () { int y; unsigned char _1; unsigned char _2; unsigned char _4; unsigned char pretmp_14; unsigned char prephitmp_15; <bb 2> [local count: 1073741824]: # VUSE <.MEM_8(D)> _1 = BIT_FIELD_REF <a, 8, 0>; _2 = _1 & 3; if (_2 != 0) goto <bb 3>; [33.00%] else goto <bb 7>; [67.00%] <bb 7> [local count: 719407024]: goto <bb 4>; [100.00%] <bb 3> [local count: 354334800]: # .MEM_10 = VDEF <.MEM_8(D)> y_11 = f (); # VUSE <.MEM_10> pretmp_14 = BIT_FIELD_REF <a, 8, 0>; <bb 4> [local count: 1073741824]: # y_5 = PHI <y_9(D)(7), y_11(3)> # .MEM_6 = PHI <.MEM_8(D)(7), .MEM_10(3)> # prephitmp_15 = PHI <_1(7), pretmp_14(3)> _4 = prephitmp_15 & 1; if (_4 != 0) goto <bb 5>; [50.00%] else goto <bb 8>; [50.00%] <bb 8> [local count: 536870912]: goto <bb 6>; [100.00%] <bb 5> [local count: 536870913]: ## y_5 = PHI <y_9(D)(7), y_11(3)> ## uninit when: _2 == 0 ## : BIT_FIELD_REF <a, 8, 0> & 3 == 0 ## ## used when: prephitmp_15 & != 0 ## : PHI <_1(7), pretmp_14(3)> ## : _1(7) != 0 || pretmp_14 != 0 ## : BIT_FIELD_REF <a, 8, 0> != 0 || BIT_FIELD_REF <a, 8, 0> != 0 ## : BIT_FIELD_REF <a, 8, 0> != 0 # .MEM_12 = VDEF <.MEM_6> z = y_5; <bb 6> [local count: 1073741824]: # .MEM_7 = PHI <.MEM_6(8), .MEM_12(5)> # VUSE <.MEM_7> return; } This has been fixed on the trunk. |