http://bugzilla.suse.com/show_bug.cgi?id=1169156 http://bugzilla.suse.com/show_bug.cgi?id=1169156#c1 --- Comment #1 from Petr Tesařík --- *** Bug 1169132 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1169156 http://bugzilla.suse.com/show_bug.cgi?id=1169156#c3 --- Comment #3 from Richard Biener --- Ouch. There's nothing "wrong" here - GCC expects to have access to libgcc. That your "optimization" using __builtin_constant_p falls apart is a missed optimization. I'm not 100% sure where it goes "wrong", but I see things like a long chain of : _49 = divisor_3(D) & 1073741824; if (_49 == 0) goto ; [50.00%] else goto ; [50.00%] : _50 = divisor_3(D) & 536870912; if (_50 == 0) goto ; [50.00%] else goto ; [50.00%] ... : _77 = divisor_3(D) & 4; if (_77 != 0) goto ; [50.00%] else goto ; [50.00%] : : # iftmp.68_80 = PHI <1(42), 2147483648(43), 1073741824(44), 536870912(45), 268435456(46), 134217728(47), 67108864(48), 33554432(49), 16777216(50), 8388608(51), 4194304(52), 2097152(53), 1048576(54), 524288(55), 262144(56), 131072(57), 65536(58), 32768(59), 16384(60), 8192(61), 4096(62), 2048(63), 1024(64), 512(65), 256(66), 128(67), 64(68), 32(69), 16(70), 8(71), 4(72), 2(73)> _78 = (long long unsigned int) divisor_3(D); _79 = 18446744073709551615 / _78; ... with the expectation of the compiler to force constant evaluation of the code along all conditional edges. A __builtin_constant_p guarding conditional does _not_ force constant evaluation on all expressions dependent on the constant argument. Of course the issue here is likely the appearant jump threading opportunity here: if (__builtin_constant_p(__base) && is_power_of_2(__base)) { __rem = (dividend) & (__base - 1); (dividend) >>= ilog2(__base); } else if (__builtin_constant_p(__base) && __base != 0) { it's tempting to factor the common conditional given that is_power_of_2 starts is bool is_power_of_2(unsigned long n) { return (n != 0 && ((n & (n - 1)) == 0)); not sure what that constant && is_power_of_2 really should do here, it looks like premature optimization. Removing that case "fixes" the issue. I expect refactoring the code to common the if (__builtin_constant_p(__base) && __base != 0) as outermost conditional and _inside_ of it special-casing if (is_power_of_2 ()) might help, too. So, not really a bug IMHO, just a bogus expectation. -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1169156 http://bugzilla.suse.com/show_bug.cgi?id=1169156#c4 --- Comment #4 from Petr Tesařík --- I've had a look at the generated assembly code. The code between the loop and the call to spin_lock() has two variants: A. when the loop has been run at least once. -> r5 is set to 0 B. when the loop condition was initially false. -> r5 is set to 1 I believe r5 corresponds to __builtin_constant_p(factor), and that's what gets checked after spin_unlock(). But the whole point of checking __builtin_constant_p() was to do some expensive calculations only if they can be performed at build-time. I wonder if there's a better way to achieve that goal... -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1169156 http://bugzilla.suse.com/show_bug.cgi?id=1169156#c6 Richard Biener changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |matz@suse.com --- Comment #6 from Richard Biener --- OK, so I dug into the actual reason which is (as expected) jump threading turning factor = 1; if (loop iterates at all) { loop changing factor; } constant = __builtin_constant_p (factor); if (constant) { ... } into if (loop iterates at all) { loop changing factor; constant = false; } else constant = true; if (constant) but in a way that where we earlier replaced all the "same" __builtin_constant_p (factor) with 'constant' the now no longer constant branch gets handled inconsistently as 'true'. Based on the phase of the moon we handle it inconsistently as 'false'. Disabling the particular threading phase (-fdisable-tree-thread2) avoids that issue. In GCC 10 we moved committing to __builtin_constant_p values even earlier (for https://gcc.gnu.org/PR90387) but that's not suitable for backporting. And it is not really early enough to avoid such issues since there's a threading phase very very early in the compilation as well. Not CSEing __builtin_constant_p () helps for the testcase but in the end it's the threading process which ends up creating PHIs to merge different __builtin_constant_p outcomes and replacing tests with tests based on those PHI results which cause the problem - those are later not "removed". As if you'd do if (flag) constant = __builtin_constant_p (a); else constant = __builtin_constant_p (b); if (constant) ... we end up with # constant_1 = PHI <0(2), 1(3)> if (constant_1 != 0) ... because while we replace __builtin_constant_p with either zero or one we only can resolve direct comparisons at compile-time but any merges will have to be resolved at runtime, meaning both paths would stay. There's appearantly not enough profitability to thread those PHIs away. For your testcase we have for example # _2559 = PHI <1(6), 0(12), 0(11)> <some intermediate control flow> if (_2559 != 0) .. the __builtin_constant_p conditionalized code and threading would involve duplicating the <some intermediate control flow> part (not sure if we'd even handle not triplicating it). So the part of your testcase that doesn't test the result of __builtin_constant_p directly but combines it with other predicates makes it more susceptible for the above issue. That's again the cited if (__builtin_constant_p(__base) && is_power_of_2(__base)) { __rem = (dividend) & (__base - 1); (dividend) >>= ilog2(__base); } else if (__builtin_constant_p(__base) && __base != 0) { esp. the is_power_of_2 conditional. Refactoring this to use if (__builtin_constant_p (__base)) checks would make the code less prone to the issue. Maybe Micha has other suggestions as well. -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1169156 http://bugzilla.suse.com/show_bug.cgi?id=1169156#c8 --- Comment #8 from Richard Biener --- If you are fine with __builtin_constant_p() being unknown at build time then just #define it to false ... seriously esp. kernel folks wanted to have as many __builtin_constant_p () resolved to true as possible. This is why we are resolving it to false so late. And this is why these issues pop up because GCC shoots itself^Wthe developer in the foot with code transforms in between the source AST and the point we are resolving __builtin_constant_p to false. The circumstances where things "break" (usually they break in a more invisble way than here) are rare but probably unavoidable. -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1169156 http://bugzilla.suse.com/show_bug.cgi?id=1169156#c10 --- Comment #10 from Michael Matz --- Hmm isn't the fix for https://gcc.gnu.org/PR90387 the one that exposes the problem here? If so it could just as well simply be reverted, as indicated there it was a fix for a minor problem with a valid work around. (If this is not the one exposing the difference between old-gcc and new-gcc, ignore the remark) Then, I agree that the kernel code depends on certain transformations happening, but other transformations not happening, in just the right way, and hence is inherently somewhat unstable :-/ I have no good suggestion to that, except rewriting the kernel code to not rely on builtin_constant_p(X) being inferrable as false even after X was clearly computed in the code before the predicate. But then I also agree with Petr, that the jump threading (or rather, just forwarding in this case) should definitely happen here: factor = 1; if (loop iterates at all) { loop changing factor; constant = false; } else constant = true; if (constant) { /* some operations using factor */ } The whole 'if (constant)' block should be moved into the else of the first conditional (by GCC I mean), and I think as QoI issue the users ought to be able to depend on GCC to do that. (I'm pretty sure with this structure GCC does do the right thing right now) Now it seems the testcase has stuff in front of the 'if (constant)'. With that all bets are off again, and the user should avoid doing that. Generally it feels a bit sad that GCC manages to confuse itself for user-code that ought to be easily optimizable with the hand-written _b_c_p(), but I don't have good suggestions to make this better. Possibly we could limit optimization related to _b_c_p in the early pipelines: i.e.: * no CSE of them * no jump threading through them * no CFG manipulations based on them with the intention that the control flow structure stays as close as possible to the original source in the area of where _b_c_p is used, so that once they will be resolved that CFG hasn't yet become unobvious to the optimizer. (I will note that the problem for the kernel here is non trivial: even if they were to provide an implementation of the division function of libgcc.a, it would then indeed be called at runtime, and the whole point was to not have to do the expensive division) -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1169156 http://bugzilla.suse.com/show_bug.cgi?id=1169156#c12 --- Comment #12 from Michal Suchanek --- It is not exactly nice that you have to link modules to the helper in the kernel. It may be also problematic if simple division emits a function call someplace where you are inside an exception handler with only limited memory mappings installed and user registers that are in need of saving. Still it's done for 32bit division anyway so all this does already happen. It sure requires that somebody implements these 64bit helpers but cut and pasting from libgcc should be license-compatible. -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1169156 http://bugzilla.suse.com/show_bug.cgi?id=1169156#c14 --- Comment #14 from Richard Biener --- Note we may not have special optimized 64bit / 32bit division handling. -- You are receiving this mail because: You are on the CC list for the bug.

http://bugzilla.suse.com/show_bug.cgi?id=1169156 http://bugzilla.suse.com/show_bug.cgi?id=1169156#c16 Richard Biener changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(rguenther@suse.co | |m) | --- Comment #16 from Richard Biener --- (In reply to Petr Tesařík from comment #15)

(In reply to Richard Biener from comment #14)

...

Note we may not have special optimized 64bit / 32bit division handling.

Bummer.

Shall I file a feature request for a future GCC version?

I can handle that for upstream (a SUSE bugzilla isn't helpful). But it would need details on what can be optimized [on specific targets], for example with testcases with specific (non-)constants. -- You are receiving this mail because: You are on the CC list for the bug.