[Bug target/99863] New: [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2

[Bug target/99863] New: [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2

[zsojka at seznam dot cz]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

            Bug ID: 99863
           Summary: [10/11 Regression] wrong code with -O
                    -fno-tree-forwprop -mno-sse2
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: zsojka at seznam dot cz
  Target Milestone: ---
              Host: x86_64-pc-linux-gnu
            Target: x86_64-pc-linux-gnu

Created attachment 50493
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50493&action=edit
reduced testcase

Output:
$ x86_64-pc-linux-gnu-gcc -O -fno-tree-forwprop -mno-sse2 testcase.c -Wno-psabi
$ ./a.out 
Aborted

$ x86_64-pc-linux-gnu-gcc -v
Using built-in specs.
COLLECT_GCC=/repo/gcc-trunk/binary-latest-amd64/bin/x86_64-pc-linux-gnu-gcc
COLLECT_LTO_WRAPPER=/repo/gcc-trunk/binary-trunk-r11-7937-20210331154556-geadf009b229-checking-yes-rtl-df-extra-amd64/bin/../libexec/gcc/x86_64-pc-linux-gnu/11.0.1/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /repo/gcc-trunk//configure --enable-languages=c,c++
--enable-valgrind-annotations --disable-nls --enable-checking=yes,rtl,df,extra
--with-cloog --with-ppl --with-isl --build=x86_64-pc-linux-gnu
--host=x86_64-pc-linux-gnu --target=x86_64-pc-linux-gnu
--with-ld=/usr/bin/x86_64-pc-linux-gnu-ld
--with-as=/usr/bin/x86_64-pc-linux-gnu-as --disable-libstdcxx-pch
--prefix=/repo/gcc-trunk//binary-trunk-r11-7937-20210331154556-geadf009b229-checking-yes-rtl-df-extra-amd64
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 11.0.1 20210331 (experimental) (GCC)

[Bug target/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Target Milestone|---                         |10.3
     Ever confirmed|0                           |1
           Keywords|                            |needs-bisection
   Last reconfirmed|                            |2021-04-01
           Priority|P3                          |P2

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed.  It's a bit hard to follow what goes wrong (didn't yet spot it).

[Bug target/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Also time to fix this stupid veclower behavior:

   _7 = (unsigned int) _14;
   _5 = {_7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7};
-  v512u32_0_16 = _5 * v512u32_0_15(D);
+  _53 = {_7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7};
+  _54 = BIT_FIELD_REF <_53, 32, 0>;
+  _55 = BIT_FIELD_REF <v512u32_0_15(D), 32, 0>;
+  _56 = _54 * _55;
+  _57 = {_7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7};
+  _58 = BIT_FIELD_REF <_57, 32, 32>;
+  _59 = BIT_FIELD_REF <v512u32_0_15(D), 32, 32>;
+  _60 = _58 * _59;
...

instead of

   _5 = {_7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7};
-  v512u32_0_16 = _5 * v512u32_0_15(D);
   _54 = BIT_FIELD_REF <_5, 32, 0>;
...

or even better

   _5 = {_7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7, _7};
-  v512u32_0_16 = _5 * v512u32_0_15(D);
+  _55 = BIT_FIELD_REF <v512u32_0_15(D), 32, 0>;
+  _56 = _7 * _55;

it's all "fixed" later by CSE of course.

The error might be involving the clever handling of

-  v256u8_r_17 = _8 + _19;
+  _117 = BIT_FIELD_REF <_8, 64, 0>;
+  _118 = BIT_FIELD_REF <_19, 64, 0>;
+  _119 = _117 ^ _118;
+  _120 = _118 & 9187201950435737471;
+  _121 = _117 & 9187201950435737471;
+  _122 = _119 & 9259542123273814144;
+  _123 = _120 + _121;
+  _124 = _122 ^ _123;
+  _125 = BIT_FIELD_REF <_8, 64, 64>;
+  _126 = BIT_FIELD_REF <_19, 64, 64>;
...
+  _149 = {_124, _132, _140, _148};
+  _150 = VIEW_CONVERT_EXPR<v256u8>(_149);
+  v256u8_r_17 = _150;

it's enough to -fdisable-tree-forwprop4 (forwprop after veclower) to make
the problem show up so it might be as well an RTL optimization issue.

[Bug target/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 6da66985ad6..1f417a52702 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -5231,6 +5231,7 @@ gimplify_init_constructor (tree *expr_p, gimple_seq
*pre_p, gimple_seq *post_p,
                                                       TREE_TYPE (ce->value)))
              TREE_STATIC (ctor) = 0;
          }
+       recompute_constructor_flags (ctor);
        if (!is_gimple_reg (TREE_OPERAND (*expr_p, 0)))
          TREE_OPERAND (*expr_p, 1) = get_formal_tmp_var (ctor, pre_p);
       }

simplifies the IL (vector lowering was supposed to simplify the element
extraction but GENERIC match.pd folding is confused about TREE_SIDE_EFFECTS
on the vector CTOR).  It still nicely triggers the bug though.

[Bug target/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[10/11 Regression] wrong    |[10/11 Regression] wrong
                   |code with -O                |code with -O
                   |-fno-tree-forwprop          |-fno-tree-forwprop
                   |-mno-sse2                   |-mno-sse2 since
                   |                            |r10-7268-g529ea7d9596b26ba
                 CC|                            |law at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org

--- Comment #4 from Martin Liška <marxin at gcc dot gnu.org> ---
Started with r10-7268-g529ea7d9596b26ba.

[Bug target/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Martin Liška from comment #4)
> Started with r10-7268-g529ea7d9596b26ba.

Reverting on trunk fixes the issue.  Good vs. bad assembly shows the likely
culprit:

        shrq    $32, %rax
-       movq    %rax, %r8
-       testq   %rdi, %rdi
+       movq    %rax, %rdi
        setne   %al

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|target                      |rtl-optimization

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #5)
> (In reply to Martin Liška from comment #4)
> > Started with r10-7268-g529ea7d9596b26ba.
> 
> Reverting on trunk fixes the issue.  Good vs. bad assembly shows the likely
> culprit:
> 
>         shrq    $32, %rax
> -       movq    %rax, %r8
> -       testq   %rdi, %rdi
> +       movq    %rax, %rdi
>         setne   %al

And this is

(insn 6 3 7 2 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg/v:DI 177 [ u64_0 ])
            (const_int 0 [0]))) "t.c":14:30 8 {*cmpdi_ccno_1}
     (nil))
(insn 7 6 8 2 (set (reg:QI 179)
        (eq:QI (reg:CCZ 17 flags)
            (const_int 0 [0]))) "t.c":14:30 802 {*setcc_qi}
     (expr_list:REG_DEAD (reg:CCZ 17 flags)
        (nil)))
...
(insn 23 22 24 2 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg/v:DI 177 [ u64_0 ])
            (const_int 0 [0]))) "t.c":15:19 8 {*cmpdi_ccno_1}
     (nil))
(insn 24 23 25 2 (set (reg:QI 190)
        (ne:QI (reg:CCZ 17 flags)
            (const_int 0 [0]))) "t.c":15:19 802 {*setcc_qi}
     (nil))

deferring deletion of insn with uid = 23.

likely somehow seeing the set as a noop move.  trial is (reg:CCZ 17 flags)
but likely CSE doesn't track CC flag clobbers appropriately though there's
no other CC clobber insn visible at this point.  At least CSE1 fails
to remove the REG_DEAD note on reg:CCZ in insn 7?

Later we happily insert CC clobbers inbetween - notably DSE1 (eh?!)
produces

(insn 166 12 168 2 (set (reg:SI 262)
        (subreg:SI (reg:DI 182 [ foo0_v256u32_0 ]) 0)) "t.c":14:36 75
{*movsi_internal}
     (nil))
(insn 168 166 167 2 (set (reg:DI 263)
        (reg:DI 182 [ foo0_v256u32_0 ])) "t.c":14:36 74 {*movdi_internal}
     (expr_list:REG_DEAD (reg:DI 182 [ foo0_v256u32_0 ])
        (nil)))
(insn 167 168 169 2 (parallel [
            (set (reg:DI 263)
                (lshiftrt:DI (reg:DI 263)
                    (const_int 32 [0x20])))
            (clobber (reg:CC 17 flags))
        ]) "t.c":14:36 703 {*lshrdi3_1}
     (expr_list:REG_UNUSED (reg:CC 17 flags)
        (nil)))

which uses subregs/shifts to elide a load/store pair (I think).

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:512429a885e87bef51057a001681b7d8d106e807

commit r11-7949-g512429a885e87bef51057a001681b7d8d106e807
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Apr 1 11:51:33 2021 +0200

    tree-optimization/99863 - clear vector CTOR TREE_SIDE_EFFECTS

    When we gimplify a vector CTOR the original CONSTRUCTOR tree remains
    but we fail to recompute flags such as TREE_SIDE_EFFECTS.  This causes
    later GENERIC folding of them in vector lowering to give up since
    the match.pd machinery is careful about TREE_SIDE_EFFECTS.

    Fixing this makes vector lowering produce much less garbage and
    thus following the IL for PR99793 easier.

    2021-04-01  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/99863
            * gimplify.c (gimplify_init_constructor): Recompute vector
            constructor flags.

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

Eric Botcazou <ebotcazou at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #8 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Richard, did you mean to CC me for another PR by any chance?

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #9 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Richard, did you mean to CC me for another PR by any chance?

Never mind, I was confused by your commit.

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jakub at gcc dot gnu.org

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think the REG_DEAD note isn't the problem here.
At least, when I dump bb 2 in rest_of_handle_dse right before df_analyze call,
there is
(insn 6 3 7 2 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg/v:DI 203 [ y ])
            (const_int 0 [0]))) "pr99863.c":14:12 8 {*cmpdi_ccno_1}
     (nil))
(insn 7 6 8 2 (set (reg:QI 205)
        (eq:QI (reg:CCZ 17 flags)
            (const_int 0 [0]))) "pr99863.c":14:12 802 {*setcc_qi}
     (expr_list:REG_DEAD (reg:CCZ 17 flags)
        (nil)))
...
(insn 40 38 41 2 (set (reg:QI 220)
        (ne:QI (reg:CCZ 17 flags)
            (const_int 0 [0]))) "pr99863.c":15:11 802 {*setcc_qi}
     (expr_list:REG_DEAD (reg:CCZ 17 flags)
        (nil)))
but if I dump it right after df_analyze call, I see:
(insn 6 3 7 2 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg/v:DI 203 [ y ])
            (const_int 0 [0]))) "pr99863.c":14:12 8 {*cmpdi_ccno_1}
     (expr_list:REG_DEAD (reg/v:DI 203 [ y ])
        (nil)))
(insn 7 6 8 2 (set (reg:QI 205)
        (eq:QI (reg:CCZ 17 flags)
            (const_int 0 [0]))) "pr99863.c":14:12 802 {*setcc_qi}
     (nil))
...
(insn 40 38 41 2 (set (reg:QI 220)
        (ne:QI (reg:CCZ 17 flags)
            (const_int 0 [0]))) "pr99863.c":15:11 802 {*setcc_qi}
     (expr_list:REG_DEAD (reg:CCZ 17 flags)
        (nil)))

i.e. the problematic REG_DEAD note is gone and I think at this point DF knows
that CC is live there.
But replace_read has code to check for live hard regs, see
note_stores (this_insn, look_for_hardregs, regs_set);
in there etc.

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #11 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 1 Apr 2021, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |jakub at gcc dot gnu.org
> 
> --- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> I think the REG_DEAD note isn't the problem here.
> At least, when I dump bb 2 in rest_of_handle_dse right before df_analyze call,
> there is
> (insn 6 3 7 2 (set (reg:CCZ 17 flags)
>         (compare:CCZ (reg/v:DI 203 [ y ])
>             (const_int 0 [0]))) "pr99863.c":14:12 8 {*cmpdi_ccno_1}
>      (nil))
> (insn 7 6 8 2 (set (reg:QI 205)
>         (eq:QI (reg:CCZ 17 flags)
>             (const_int 0 [0]))) "pr99863.c":14:12 802 {*setcc_qi}
>      (expr_list:REG_DEAD (reg:CCZ 17 flags)
>         (nil)))
> ...
> (insn 40 38 41 2 (set (reg:QI 220)
>         (ne:QI (reg:CCZ 17 flags)
>             (const_int 0 [0]))) "pr99863.c":15:11 802 {*setcc_qi}
>      (expr_list:REG_DEAD (reg:CCZ 17 flags)
>         (nil)))
> but if I dump it right after df_analyze call, I see:
> (insn 6 3 7 2 (set (reg:CCZ 17 flags)
>         (compare:CCZ (reg/v:DI 203 [ y ])
>             (const_int 0 [0]))) "pr99863.c":14:12 8 {*cmpdi_ccno_1}
>      (expr_list:REG_DEAD (reg/v:DI 203 [ y ])
>         (nil)))
> (insn 7 6 8 2 (set (reg:QI 205)
>         (eq:QI (reg:CCZ 17 flags)
>             (const_int 0 [0]))) "pr99863.c":14:12 802 {*setcc_qi}
>      (nil))
> ...
> (insn 40 38 41 2 (set (reg:QI 220)
>         (ne:QI (reg:CCZ 17 flags)
>             (const_int 0 [0]))) "pr99863.c":15:11 802 {*setcc_qi}
>      (expr_list:REG_DEAD (reg:CCZ 17 flags)
>         (nil)))
> 
> i.e. the problematic REG_DEAD note is gone and I think at this point DF knows
> that CC is live there.
> But replace_read has code to check for live hard regs, see
> note_stores (this_insn, look_for_hardregs, regs_set);
> in there etc.

But find_shift_sequence doesn't seem to check whether the emitted
insns contain any extra hardreg clobbers that make the replacement
invalid.

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Ah, I see the actual problem.  replace_read checks the hard regs in the insn
sequence and compares them to the live hard regs at the point of the read_insn,
in the testcase insn 134 after the CC setter 6 and CC users 7 and 40.
CC is not live on the insn 134.
But we emit that sequence elsewhere:
2076          /* Insert this right before the store insn where it will be safe
2077             from later insns that might change it before the read.  */
2078          emit_insn_before (insns, store_insn->insn);
and store_insn->insn in this case is insn 19, which is in between insn 7 and
40, and CC is live there.
So, I think we need to do this live hard regs testing in record_store instead
or in addition to.
We already call get_stored_val there just for testing purposes and throw it
away afterwards.

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, I think we want to do:
--- gcc/dse.c.jj        2021-01-28 09:59:11.973750676 +0100
+++ gcc/dse.c   2021-04-01 15:16:22.003913061 +0200
@@ -1970,8 +1970,7 @@ get_stored_val (store_info *store_info,

 static bool
 replace_read (store_info *store_info, insn_info_t store_insn,
-             read_info_t read_info, insn_info_t read_insn, rtx *loc,
-             bitmap regs_live)
+             read_info_t read_info, insn_info_t read_insn, rtx *loc)
 {
   machine_mode store_mode = GET_MODE (store_info->mem);
   machine_mode read_mode = GET_MODE (read_info->mem);
@@ -2040,7 +2039,8 @@ replace_read (store_info *store_info, in
          note_stores (this_insn, look_for_hardregs, regs_set);
        }

-      bitmap_and_into (regs_set, regs_live);
+      if (store_insn->fixed_regs_live)
+       bitmap_and_into (regs_set, store_insn->fixed_regs_live);
       if (!bitmap_empty_p (regs_set))
        {
          if (dump_file && (dump_flags & TDF_DETAILS))
@@ -2286,7 +2286,7 @@ check_mem_read_rtx (rtx *loc, bb_info_t
                                                 offset - store_info->offset,
                                                 width)
                      && replace_read (store_info, i_ptr, read_info,
-                                      insn_info, loc, bb_info->regs_live))
+                                      insn_info, loc))
                    return;

                  /* The bases are the same, just see if the offsets
@@ -2352,8 +2352,7 @@ check_mem_read_rtx (rtx *loc, bb_info_t
                                   store_info->width)
              && all_positions_needed_p (store_info,
                                         offset - store_info->offset, width)
-             && replace_read (store_info, i_ptr,  read_info, insn_info, loc,
-                              bb_info->regs_live))
+             && replace_read (store_info, i_ptr,  read_info, insn_info, loc))
            return;

          remove = canon_true_dependence (store_info->mem,
to fix this.  Perhaps together with filling fixed_regs_live on more stores,
currently we fill it in only for insn with exactly one store or on memset
calls, perhaps we want to fill it on insns with more than one store?

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Though, we don't add the multiple stores case to active_local_stores and all
the replace_read calls are from the active_local_stores chain.
So I think the above patch should DTRT.

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #15 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 1 Apr 2021, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863
> 
> --- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> Though, we don't add the multiple stores case to active_local_stores and all
> the replace_read calls are from the active_local_stores chain.
> So I think the above patch should DTRT.

I suppose it already fails elsewhere if fixed_regs_live is not
available?

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to rguenther@suse.de from comment #15)
> On Thu, 1 Apr 2021, jakub at gcc dot gnu.org wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863
> > 
> > --- Comment #14 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> > Though, we don't add the multiple stores case to active_local_stores and all
> > the replace_read calls are from the active_local_stores chain.
> > So I think the above patch should DTRT.
> 
> I suppose it already fails elsewhere if fixed_regs_live is not
> available?

No.  But the way I wrote the above patch, if fixed_regs_live is not available,
it will punt if any hard regs are live in the considered sequence.

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 50496
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50496&action=edit
gcc11-pr99863.patch

Full untested patch I'm going to bootstrap/regtest overnight.

[Bug rtl-optimization/99863] [10/11 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #18 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:9c7473688e78dc41fd4312a983453df195dd7786

commit r11-7970-g9c7473688e78dc41fd4312a983453df195dd7786
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Apr 3 10:07:09 2021 +0200

    dse: Fix up hard reg conflict checking in replace_read [PR99863]

    Since PR37922 fix RTL DSE has hard register conflict checking
    in replace_read, so that if the replacement sequence sets (or typically
just
    clobbers) some hard register (usually condition codes) we verify that
    hard register is not live.
    Unfortunately, it compares the hard reg set clobbered/set by the sequence
    (regs_set) against the currently live hard register set, but it then
    emits the insn sequence not at the current insn position, but before
    store_insn->insn.
    So, we should not compare against the current live hard register set,
    but against the hard register live set at the point of the store insn.
    Fortunately, we already have that remembered in
store_insn->fixed_regs_live.

    In addition to bootstrapping/regtesting this patch on x86_64-linux and
    i686-linux, I've also added statistics gathering and it seems the only
    place where we end up rejecting the replace_read is the newly added
    testcase (the PR37922 is no longer effective at that) and fixed_regs_live
    has been always non-NULL at the if (store_insn->fixed_regs_live) spot.
    Rather than having there an assert, I chose to just keep regs_set
    as is, which means in that hypothetical case where fixed_regs_live wouldn't
    be computed for some store we'd still accept sequences that don't
    clobber/set any hard registers and just punt on those that clobber/set
    those.

    2021-04-03  Jakub Jelinek  <jakub@redhat.com>

            PR rtl-optimization/99863
            * dse.c (replace_read): Drop regs_live argument.  Instead of
            regs_live, use store_insn->fixed_regs_live if non-NULL,
            otherwise punt if insns sequence clobbers or sets any hard
            registers.

            * gcc.target/i386/pr99863.c: New test.

[Bug rtl-optimization/99863] [10 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[10/11 Regression] wrong    |[10 Regression] wrong code
                   |code with -O                |with -O -fno-tree-forwprop
                   |-fno-tree-forwprop          |-mno-sse2 since
                   |-mno-sse2 since             |r10-7268-g529ea7d9596b26ba
                   |r10-7268-g529ea7d9596b26ba  |

--- Comment #19 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far.

[Bug rtl-optimization/99863] [10 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|10.3                        |10.4

--- Comment #20 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 10.3 is being released, retargeting bugs to GCC 10.4.

[Bug rtl-optimization/99863] [10 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #21 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:7a2f91d413eb7a3eb0ba52c7ac9618a35addd12a

commit r10-9721-g7a2f91d413eb7a3eb0ba52c7ac9618a35addd12a
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Apr 3 10:07:09 2021 +0200

    dse: Fix up hard reg conflict checking in replace_read [PR99863]

    Since PR37922 fix RTL DSE has hard register conflict checking
    in replace_read, so that if the replacement sequence sets (or typically
just
    clobbers) some hard register (usually condition codes) we verify that
    hard register is not live.
    Unfortunately, it compares the hard reg set clobbered/set by the sequence
    (regs_set) against the currently live hard register set, but it then
    emits the insn sequence not at the current insn position, but before
    store_insn->insn.
    So, we should not compare against the current live hard register set,
    but against the hard register live set at the point of the store insn.
    Fortunately, we already have that remembered in
store_insn->fixed_regs_live.

    In addition to bootstrapping/regtesting this patch on x86_64-linux and
    i686-linux, I've also added statistics gathering and it seems the only
    place where we end up rejecting the replace_read is the newly added
    testcase (the PR37922 is no longer effective at that) and fixed_regs_live
    has been always non-NULL at the if (store_insn->fixed_regs_live) spot.
    Rather than having there an assert, I chose to just keep regs_set
    as is, which means in that hypothetical case where fixed_regs_live wouldn't
    be computed for some store we'd still accept sequences that don't
    clobber/set any hard registers and just punt on those that clobber/set
    those.

    2021-04-03  Jakub Jelinek  <jakub@redhat.com>

            PR rtl-optimization/99863
            * dse.c (replace_read): Drop regs_live argument.  Instead of
            regs_live, use store_insn->fixed_regs_live if non-NULL,
            otherwise punt if insns sequence clobbers or sets any hard
            registers.

            * gcc.target/i386/pr99863.c: New test.

    (cherry picked from commit 9c7473688e78dc41fd4312a983453df195dd7786)

[Bug rtl-optimization/99863] [10 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #22 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.

[Bug rtl-optimization/99863] [10 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #23 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:e9d56fab891afa3d40ed2ad75264c09aee65ab88

commit r9-9441-ge9d56fab891afa3d40ed2ad75264c09aee65ab88
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Apr 3 10:07:09 2021 +0200

    dse: Fix up hard reg conflict checking in replace_read [PR99863]

    Since PR37922 fix RTL DSE has hard register conflict checking
    in replace_read, so that if the replacement sequence sets (or typically
just
    clobbers) some hard register (usually condition codes) we verify that
    hard register is not live.
    Unfortunately, it compares the hard reg set clobbered/set by the sequence
    (regs_set) against the currently live hard register set, but it then
    emits the insn sequence not at the current insn position, but before
    store_insn->insn.
    So, we should not compare against the current live hard register set,
    but against the hard register live set at the point of the store insn.
    Fortunately, we already have that remembered in
store_insn->fixed_regs_live.

    In addition to bootstrapping/regtesting this patch on x86_64-linux and
    i686-linux, I've also added statistics gathering and it seems the only
    place where we end up rejecting the replace_read is the newly added
    testcase (the PR37922 is no longer effective at that) and fixed_regs_live
    has been always non-NULL at the if (store_insn->fixed_regs_live) spot.
    Rather than having there an assert, I chose to just keep regs_set
    as is, which means in that hypothetical case where fixed_regs_live wouldn't
    be computed for some store we'd still accept sequences that don't
    clobber/set any hard registers and just punt on those that clobber/set
    those.

    2021-04-03  Jakub Jelinek  <jakub@redhat.com>

            PR rtl-optimization/99863
            * dse.c (replace_read): Drop regs_live argument.  Instead of
            regs_live, use store_insn->fixed_regs_live if non-NULL,
            otherwise punt if insns sequence clobbers or sets any hard
            registers.

            * gcc.target/i386/pr99863.c: New test.

    (cherry picked from commit 7a2f91d413eb7a3eb0ba52c7ac9618a35addd12a)

[Bug rtl-optimization/99863] [10 Regression] wrong code with -O -fno-tree-forwprop -mno-sse2 since r10-7268-g529ea7d9596b26ba

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99863

--- Comment #24 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-8 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:e1ef85b4f29e04e97f2ac0998bfd62bac50428b9

commit r8-10904-ge1ef85b4f29e04e97f2ac0998bfd62bac50428b9
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Sat Apr 3 10:07:09 2021 +0200

    dse: Fix up hard reg conflict checking in replace_read [PR99863]

    Since PR37922 fix RTL DSE has hard register conflict checking
    in replace_read, so that if the replacement sequence sets (or typically
just
    clobbers) some hard register (usually condition codes) we verify that
    hard register is not live.
    Unfortunately, it compares the hard reg set clobbered/set by the sequence
    (regs_set) against the currently live hard register set, but it then
    emits the insn sequence not at the current insn position, but before
    store_insn->insn.
    So, we should not compare against the current live hard register set,
    but against the hard register live set at the point of the store insn.
    Fortunately, we already have that remembered in
store_insn->fixed_regs_live.

    In addition to bootstrapping/regtesting this patch on x86_64-linux and
    i686-linux, I've also added statistics gathering and it seems the only
    place where we end up rejecting the replace_read is the newly added
    testcase (the PR37922 is no longer effective at that) and fixed_regs_live
    has been always non-NULL at the if (store_insn->fixed_regs_live) spot.
    Rather than having there an assert, I chose to just keep regs_set
    as is, which means in that hypothetical case where fixed_regs_live wouldn't
    be computed for some store we'd still accept sequences that don't
    clobber/set any hard registers and just punt on those that clobber/set
    those.

    2021-04-03  Jakub Jelinek  <jakub@redhat.com>

            PR rtl-optimization/99863
            * dse.c (replace_read): Drop regs_live argument.  Instead of
            regs_live, use store_insn->fixed_regs_live if non-NULL,
            otherwise punt if insns sequence clobbers or sets any hard
            registers.

            * gcc.target/i386/pr99863.c: New test.

    (cherry picked from commit 7a2f91d413eb7a3eb0ba52c7ac9618a35addd12a)