Re: [RFC PATCH, RTL]: Fix PR63348, gcc.dg/pr43670.c fail -fcompare-debug on MIPS

[Uros Bizjak <ubizjak at gmail dot com>]

On Thu, Sep 25, 2014 at 7:44 PM, Uros Bizjak <ubizjak@gmail.com> wrote:

>>> The failure was caused by barrier detection code, which failed to
>>> detect barrier after call insn was to be split when
>>> NOTE_CALL_ARG_LOCATION was present. This problem caused
>>> -fcompare-debug failure.
>>>
>>> Digging a bit deeped, and as hinted in the PR, the handling of
>>> barriers in try_split seems to be broken. The code is emitting extra
>>> barrier for non-debug compiles, but it "forgots" to remove the
>>> existing one, leading to duplicated barriers. The barrier is not
>>> detected at all for debug build.
>>>
>>> I have removed special handling of barriers here (also, the comment in
>>> removed code was not helpful at all), and this solved -fcompare-debug
>>> failure.
>>>
>>> The patch was also bootstrapped and regression tested on
>>> x86_64-linux-gnu {,-m32} which in -m32 mode splits x87 FP jump insns,
>>> and there were no regressions. However, I am not too familiar with
>>> rtl-optimization part and I am not confident that this code surgery is
>>> fully correct, so this is the reason for RFC status of the patch.
>>>
>>> 2014-09-24  Uros Bizjak  <ubizjak@gmail.com>
>>>
>>>      PR rtl-optimization/63348
>>>      * emit-rtl.c (try_split): Do not emit extra barrier.
>>
>> Good grief, the code you're removing pre-dates any version control we have.
>> ie, it's in the first revision of emit-rtl.c from 1992.   Egad.
>>
>> It's going to be a hell of a time figuring out why that code exists in the
>> first place.  I don't like removing code if we don't know why the code
>> exists...  Any reason you picked that route rather than looking forward
>> through the NOTEs to see if they're followed by a suitable BARRIER?
>
> I have tried with alternative patch that just skipped the NOTE:
>
> --cut here--
> Index: emit-rtl.c
> ===================================================================
> --- emit-rtl.c  (revision 215606)
> +++ emit-rtl.c  (working copy)
> @@ -3622,6 +3622,10 @@ try_split (rtx pat, rtx uncast_trial, int last)
>    int njumps = 0;
>    rtx call_insn = NULL_RTX;
>
> +  if (after && NOTE_P (after)
> +      && NOTE_KIND (after) == NOTE_INSN_CALL_ARG_LOCATION)
> +    after = NEXT_INSN (after);
> +
>    /* We're not good at redistributing frame information.  */
>    if (RTX_FRAME_RELATED_P (trial))
>      return trial;
> --cut here--
>
> and resulted in:
>
> (call_insn 184 190 185 (parallel [
>             (call (mem:SI (reg:SI 25 $25 [217]) [0  S4 A32])
>                 (const_int 16 [0x10]))
>             (clobber (reg:SI 31 $31))
>             (clobber (reg:SI 28 $28))
>         ]) pr43670.c:29 595 {call_split}
>      (expr_list:REG_NORETURN (const_int 0 [0])
>         (nil))
>     (expr_list (use (reg:SI 79 $fakec))
>         (expr_list (use (reg:SI 28 $28))
>             (nil))))
> (barrier 185 184 175)
> (note 175 185 130 (expr_list:REG_DEP_TRUE (concat:SI (pc)
>         (unspec:SI [
>                 (reg:SI 28 $28)
>                 (const:SI (unspec:SI [
>                             (symbol_ref:SI ("abort") [flags 0x41]
> <function_decl 0x7fe1e3af2e58 abort>)
>                         ] 227))
>                 (reg:SI 79 $fakec)
>             ] UNSPEC_LOAD_CALL))
>     (nil)) NOTE_INSN_CALL_ARG_LOCATION)
> (barrier 130 175 174)
>
> I have noticed that the barrier is always there, since without -g, we have:
>
> (call_insn 76 82 77 (parallel [
>             (call (mem:SI (reg:SI 25 $25 [217]) [0  S4 A32])
>                 (const_int 16 [0x10]))
>             (clobber (reg:SI 31 $31))
>             (clobber (reg:SI 28 $28))
>         ]) pr43670.c:29 595 {call_split}
>      (expr_list:REG_NORETURN (const_int 0 [0])
>         (nil))
>     (expr_list (use (reg:SI 79 $fakec))
>         (expr_list (use (reg:SI 28 $28))
>             (nil))))
> (barrier 77 76 37)
> (barrier 37 77 40)
>
> and considering the fact that the code didn't process barriers
> correctly with -g, I simply removed the emission. The - probably
> stalled - comment was not helpful at all.

FYI, unpatched gcc created (-g):

(call_insn 184 189 175 (parallel [
            (call (mem:SI (reg:SI 25 $25 [217]) [0  S4 A32])
                (const_int 16 [0x10]))
            (clobber (reg:SI 31 $31))
            (clobber (reg:SI 28 $28))
        ]) pr43670.c:29 595 {call_split}
     (expr_list:REG_NORETURN (const_int 0 [0])
        (nil))
    (expr_list (use (reg:SI 79 $fakec))
        (expr_list (use (reg:SI 28 $28))
            (nil))))
(note 175 184 130 (expr_list:REG_DEP_TRUE (concat:SI (pc)
        (unspec:SI [
                (reg:SI 28 $28)
                (const:SI (unspec:SI [
                            (symbol_ref:SI ("abort") [flags 0x41]
<function_decl 0x7fc774a8ee58 abort>)
                        ] 227))
                (reg:SI 79 $fakec)
            ] UNSPEC_LOAD_CALL))
    (nil)) NOTE_INSN_CALL_ARG_LOCATION)
(barrier 130 175 174)

so, it didn't emit barrier at all.

Uros.