On Wed, 2020-12-02 at 11:42 -0700, Jeff Law wrote: > > On 12/1/20 7:09 PM, Ilya Leoshkevich wrote: > > On Tue, 2020-12-01 at 15:34 -0700, Jeff Law wrote: > > > No strong opinions. I think whichever is less invasive in terms > > > of > > > code > > > quality is probably the way to go. What we want to avoid is > > > suppressing > > > threading unnecessarily as that often leads to false positives > > > from > > > middle-end based warnings. Suppressing threading can also lead > > > to > > > build > > > failures in the kernel due to the way they use b_c_p. > > I think v1 is better then. Would you mind approving the following? > > That's the same code as in v1, but with the improved commit message > > and > > comments. > > > > > > > > Linux Kernel (specifically, drivers/leds/trigger/ledtrig-cpu.c) > > build > > with GCC 10 fails on s390 with "impossible constraint". > > > > Explanation by Jeff Law: > > > > ``` > > So what we have is a b_c_p at the start of an if-else > > chain. Subsequent > > tests on the "true" arm of the the b_c_p test may throw us off the > > constant path (because the constants are out of range). Once all > > the > > tests are passed (it's constant and the constant is in range) the > > true > > arm's terminal block has a special asm that requires a constant > > argument. In the case where we get to the terminal block on the > > true > > arm, the argument to the b_c_p is used as the constant argument to > > the > > special asm. > > > > At first glace jump threading seems to be doing the right > > thing. Except > > that we end up with two paths to that terminal block with the > > special > > asm, one for each of the two constant arguments to the b_c_p call. > > Naturally since that same value is used in the asm, we have to > > introduce > > a PHI to select between them at the head of the terminal > > block. Now > > the argument in the asm is no longer constant and boom we fail. > > ``` > > > > Fix by disallowing __builtin_constant_p on threading paths. > > > > gcc/ChangeLog: > > > > 2020-06-03 Ilya Leoshkevich <i...@linux.ibm.com> > > > > * tree-ssa-threadbackward.c > > (thread_jumps::profitable_jump_thread_path): > > Do not allow __builtin_constant_p on a threading path. > > > > gcc/testsuite/ChangeLog: > > > > 2020-06-03 Ilya Leoshkevich <i...@linux.ibm.com> > > > > * gcc.target/s390/builtin-constant-p-threading.c: New test. > OK. I think the old forward threader has the same problem. Which I > think can be fixed by returning NULL from > record_temporary_equivalences_from_stmts_at_dest when we see the > B_C_P > call. Fixing that in the obvious way is pre-approved once it's gone > through the usual testing.
Thanks! I've committed both: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=70a62009181f66d1d1c90d3c74de38e153c96eb0 https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=614aff0adf8fba5d843ec894603160151c20f0aa Best regards, Ilya
