On Wed, Jan 29, 2020 at 3:55 AM Jeff Law <l...@redhat.com> wrote:
>
>
> Here's two approaches to fix the regression in 89689. Note this only
> fixes the regression in the originally reported testcase. THe deeper
> issue Martin raises in C#1 is not addressed. Thus if we go forward
> with either patch ISTM that we would drop the regression markers, but
> keep the BZ open.
>
> So the key blocks in question as we enter DSE1:
>
> >
>
> > ;; basic block 2, loop depth 0, maybe hot
> > ;; prev block 0, next block 3, flags: (NEW, REACHABLE, VISITED)
> > ;; pred: ENTRY (FALLTHRU,EXECUTABLE)
> > h = l;
> > h$data_33 = l.data;
> > if (h$data_33 != &__sb_slop)
> > goto <bb 3>; [70.00%]
> > else
> > goto <bb 4>; [30.00%]
> > ;; succ: 3 [70.0% (guessed)] (TRUE_VALUE,EXECUTABLE)
> > ;; 4 [30.0% (guessed)] (FALSE_VALUE,EXECUTABLE)
> >
> > ;; basic block 3, loop depth 0, maybe hot
> > ;; prev block 2, next block 4, flags: (NEW, REACHABLE, VISITED)
> > ;; pred: 2 [70.0% (guessed)] (TRUE_VALUE,EXECUTABLE)
> > *h$data_33 = 0;
> > ;; succ: 4 [always] (FALLTHRU,EXECUTABLE)
> >
> > ;; basic block 4, loop depth 0, maybe hot
> > ;; prev block 3, next block 5, flags: (NEW, REACHABLE, VISITED)
> > ;; pred: 3 [always] (FALLTHRU,EXECUTABLE)
> > ;; 2 [30.0% (guessed)] (FALSE_VALUE,EXECUTABLE)
> > _23 = __builtin_object_size (h$data_33, 0);
> > _25 = __builtin___memcpy_chk (h$data_33, "abcd", 4, _23);
> > __builtin_puts (h$data_33);
> > _6 = h$data_33 == &__sb_slop;
> > _7 = (int) _6;
> > printf ("%d\n", _7);
> > _9 = h$data_33 == &buf;
> > _10 = (int) _9;
> > printf ("%d\n", _10);
> > if (h$data_33 != &__sb_slop)
> > goto <bb 5>; [70.00%]
> > else
> > goto <bb 6>; [30.00%]
> >
>
> So vrp1 is going to want to thread the jump at the end of bb4 which
> requires duplicating bb4. One version of bb4 will only be reachable
> via the false edge from bb2. That in turn exposes a cprop opportunity
> to replace h$data_33 with &__sbloop and the type of &__sbloop is a
> char[1] array thus triggering the false positive warning.
>
> We can avoid all that by realizing the store in bb3 is dead. That in
> turn makes the conditional at the end of bb2 pointless as bb3 is empty
> and thus both arms ultimately transfer control to bb4 without any other
> side effects meaning we can eliminate that conditional which in turn
> eliminates the need for jump threading.
>
> Again, this only fixes the original testcase and if there were other
> statements in bb3 it wouldn't work and it won't work for the test in
> c#1. But the proposed changes are a general improvement.
>
> DSE misses this case because ref_maybe_used_by_stmt_p returns true for
> the virtual operand of the __builtin_object_size call. Opps!
>
> There's two easy ways to fix this, I've bootstrapped and regression
> tested both.
>
> First, the most conservative fix and perhaps the most appropriate for
> gcc-10 -- special casing __builtin_object_size in tree-ssa-dse.c.
>
> The second approach is more general and marks __builtin_object_size as
> const rather than just pure. Jakub and I kicked this around in IRC a
> bit. We've always marked __builtin_object_size as pure. It was never
> const. THere is some concern that code motion might cause _b_o_s to
> get different results, which would be particularly concerning for
> _b_o_s types #1 and #3 which do use some contextual information.
> However, ISTM that motion is still going to be limited by the SSA
> graph, though somewhat less because we wouldn't have a dependency on
> the virtual operands. So it *should* be safe, but there's some degree
> of hesitation to make this change at this point in gcc-10's lifecycle.
>
> Thoughts? If we go forward obviously I'd add a testcase and ChangeLog
> entries.
P2 please (make _b_o_s const). No idea why it wasn't that way in the first
place. As you say the value-dependence on the result keeps everything
live and stores or loads are completely irrelevant to the size of objects.
So OK for P2.
Thanks,
Richard.
> Jeff
