[PATCH RESEND] tree-ssa-threadbackward.c (profitable_jump_thread_path): Do not allow __builtin_constant_p.

Ilya Leoshkevich iii@linux.ibm.com
Thu Dec 3 09:52:49 GMT 2020


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  <iii@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  <iii@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



More information about the Gcc-patches mailing list