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

[Ilya Leoshkevich]

On Fri, 2020-06-26 at 15:34 -0600, Jeff Law wrote:
> On Fri, 2020-06-26 at 23:17 +0200, Ilya Leoshkevich wrote:
> > On Fri, 2020-06-26 at 14:27 -0600, Jeff Law wrote:
> > > On Thu, 2020-06-04 at 02:37 +0200, Ilya Leoshkevich via Gcc-
> > > patches
> > > wrote:
> > > > Bootstrapped and regtested on x86_64-redhat-linux, ppc64le-
> > > > redhat-
> > > > linux
> > > > and s390x-redhat-linux.
> > > > 
> > > > 
> > > > Linux Kernel (specifically, drivers/leds/trigger/ledtrig-cpu.c)
> > > > build
> > > > with GCC 10 fails on s390 with "impossible constraint".
> > > > 
> > > > The problem is that jump threading makes __builtin_constant_p
> > > > lie
> > > > when
> > > > an expression in question appears to be a constant on a
> > > > threading
> > > > path.
> > > What do you mean by this?
> > > 
> > > What should happen is the path where the queried object is
> > > constant,
> > > builtin_constant_p will return true.  On the other path where it
> > > is
> > > not,
> > > builtin_constant_p will return false.
> > 
> > But what if a path ends before we use the `builtin_constant_p`
> > result?
> > In the testcase from the patch we have `i = is_active ? 1 : -1`,
> > which 
> > produces two paths.  On each path `builtin_constant_p (i)` returns 
> > true, however, the asm statement which depends on this being true
> > is
> > not on either path and comes only later.
> > 
> > Here are the relevant tree dumps (sorry, a bit verbose, I couldn't
> > make
> > them smaller):
> > 
> > gcc/cc1 ../gcc/testsuite/gcc.target/s390/builtin-constant-p-
> > threading.c 
> > -O2 -march=z196 -mzarch -fdump-tree-all-all
> > 
> > Before 106t.thread1:
> > 
> > ;;   basic block 2
> >   if (is_active_2(D) != 0)
> >     goto <bb 3>; [50.00%]
> >   else
> >     goto <bb 4>; [50.00%]
> > 
> > ;;   basic block 3
> >   # iftmp.0_6 = PHI <1(2)>
> >   _7 = __builtin_constant_pD.1098 (iftmp.0_6);
> >   if (_7 != 0)
> >     goto <bb 6>; [50.00%]
> >   else
> >     goto <bb 8>; [50.00%]
> > 
> > ;;   basic block 4
> >   # iftmp.0_4 = PHI <-1(2)>
> >   _12 = __builtin_constant_pD.1098 (iftmp.0_4);
> >   if (_12 != 0)
> >     goto <bb 5>; [50.00%]
> >   else
> >     goto <bb 8>; [50.00%]
> > 
> > ;;   basic block 5
> >   if (iftmp.0_4 >= -128)
> >     goto <bb 7>; [20.00%]
> >   else
> >     goto <bb 8>; [80.00%]
> > 
> > ;;   basic block 6
> >   if (iftmp.0_6 <= 127)
> >     goto <bb 7>; [12.00%]
> >   else
> >     goto <bb 8>; [88.00%]
> > 
> > ;;   basic block 7
> >   # iftmp.0_13 = PHI <iftmp.0_6(6), iftmp.0_4(5)>
> >   # .MEM_10 = VDEF <.MEM_3(D)>
> >   __asm__ __volatile__("asi %0,%1
> > " : "ptr" "=Q" MEM[(intD.6 *)&num_active_cpusD.2277] : "val" "i"
> > iftmp.0_13, "Q" MEM[(intD.6 *)&num_active_cpusD.2277] : "memory",
> > "cc");
> >   goto <bb 9>; [100.00%]
> > ;;    succ:       9 [always]  count:91268056 (estimated locally)
> > (FALLTHRU,EXECUTABLE)
> This looks broken already.  iftmp.0_13 is not a constant prior to
> thread1 and
> thus, IIUC is not suitable for use in this instruction.  Threading
> just
> simplified things, but it's just as broken before threading as it is
> after
> threading.

How should this work ideally?  Suppose we have:

static inline void foo (int i)
{
  if (__builtin_is_constant_p (i))
    asm volatile ("bar" :: "i" (i))
  else
    asm volatile ("baz" :: "d" (i));
}

First of all, this is a supported use case, right?

Then, the way I see it, is that at least for a while there must exist
trees like the ones above, regardless of whether their asm arguments
match constraints.  But ultimately dead code elimination should get rid
of the wrong ones before they reach RTL.

E.g. in the example above, the non-inlined version should have
`!__builtin_is_constant_p (i)`, so `bar` should not survive until
RTL.  The same for inlined `foo (1)` version's `baz`.

Is 106t.thread1 already too late for such trees?  Should some earlier
pass have cleaned them up already?

> Jeff