[Bug rtl-optimization/99114] New: [WORD_REGISTER_OPERATIONS] wrong code for (u16_var & 3) == (u32)1

[Bug rtl-optimization/99114] New: [WORD_REGISTER_OPERATIONS] wrong code for (u16_var & 3) == (u32)1

[pipcet at gmail dot com]

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

            Bug ID: 99114
           Summary: [WORD_REGISTER_OPERATIONS] wrong code for (u16_var &
                    3) == (u32)1
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: pipcet at gmail dot com
  Target Milestone: ---

I was seeing a miscompilation issue with my wasm32 backend (which was
somewhat difficult to track down because it was actually the
cross-compiled native compiler miscompiling JIT code).

The symptom is that combine.c replaces the correct RTL

(gtu (and:SI (subreg:SI (reg:HI 593)) (const_int 3))
     (const_int 1))

with the incorrect RTL

(gtu (subreg:SI (reg:HI 593)) (const_int 1))

when reg:HI 593 is known to be <= 3.

The culprit is this code, in combine.c:

                  op0 = simplify_gen_binary (AND, tmode,
                                             SUBREG_REG (XEXP (op0, 0)),
                                             gen_int_mode (c1, tmode));
                  op0 = gen_lowpart (mode, op0);

The assumption here is that op0 will be an (and:HI) after the first
statement (and we assume (subreg:SI (and:HI ... (const_int 3))) is
defined because of WORD_REGISTER_OPERATIONS) but it's actually
simplified to be just the (reg:HI 593), and (subreg:SI (reg:HI 593))
is not defined.

I'm unsure whether this can cause wrong code for in-tree backends or backends
which don't define WORD_REGISTER_OPERATIONS.

[Bug rtl-optimization/99114] [WORD_REGISTER_OPERATIONS] wrong code for (u16_var & 3) == (u32)1

[ebotcazou at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
                 CC|                            |ebotcazou at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-02-15

--- Comment #1 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
Please provide a reproducer as documented in https://gcc.gnu.org/bugs

> The assumption here is that op0 will be an (and:HI) after the first
> statement (and we assume (subreg:SI (and:HI ... (const_int 3))) is
> defined because of WORD_REGISTER_OPERATIONS) but it's actually
> simplified to be just the (reg:HI 593), and (subreg:SI (reg:HI 593))
> is not defined.

Paradoxical registers are defined under specific circumstances though.

> I'm unsure whether this can cause wrong code for in-tree backends or
> backends which don't define WORD_REGISTER_OPERATIONS.

Well, obviously not for the latter, see the comment just above the code.

[Bug rtl-optimization/99114] [WORD_REGISTER_OPERATIONS] wrong code for (u16_var & 3) == (u32)1

[pipcet at gmail dot com]

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

--- Comment #2 from pipcet at gmail dot com ---
(In reply to Eric Botcazou from comment #1)
> Please provide a reproducer as documented in https://gcc.gnu.org/bugs

I'll try, but please consider investigating this without one. It happens after
a very lengthy compilation process (compiling a buggy gcc with a buggy
cross-compiler, then compiling JIT code with that, generating a buggy .so, then
running _that_, to see incorrect behavior rather than a clear crash), in C++
code, with an out-of-tree backend.

On the other hand, it's been investigated, and it's a clear bug with a one-line
fix.

> > The assumption here is that op0 will be an (and:HI) after the first
> > statement (and we assume (subreg:SI (and:HI ... (const_int 3))) is
> > defined because of WORD_REGISTER_OPERATIONS) but it's actually
> > simplified to be just the (reg:HI 593), and (subreg:SI (reg:HI 593))
> > is not defined.
> 
> Paradoxical registers are defined under specific circumstances though.

Thanks, I understand that. This isn't one of them.

> > I'm unsure whether this can cause wrong code for in-tree backends or
> > backends which don't define WORD_REGISTER_OPERATIONS.
> 
> Well, obviously not for the latter, see the comment just above the code.

As I said, I'm unsure. The buggy line of code is executed on other targets, and
the condition under which that happens is not !paradoxical_subreg_p. I think
it's equivalent, but I don't think that's obvious...

[Bug rtl-optimization/99114] [WORD_REGISTER_OPERATIONS] wrong code for (u16_var & 3) == (u32)1

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
If

(gtu (subreg:SI (reg:HI 593)) (const_int 1))

is incorrect, why is it then recognized?  And why should (subreg:SI (and:HI
..))
be OK?

The way I read WORD_REGISTER_OPERATIONS it's a bad design to make the IL do
something that could have better been explicitely represented.  (if it is
a SImode op then please make it an SImode op!)

[Bug rtl-optimization/99114] [WORD_REGISTER_OPERATIONS] wrong code for (u16_var & 3) == (u32)1

[ebotcazou at gcc dot gnu.org]

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

--- Comment #4 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> I'll try, but please consider investigating this without one. It happens
> after a very lengthy compilation process (compiling a buggy gcc with a buggy
> cross-compiler, then compiling JIT code with that, generating a buggy .so,
> then running _that_, to see incorrect behavior rather than a clear crash),
> in C++ code, with an out-of-tree backend.

Impressive indeed, but not without a precedent.  Can you upload the RTL dump
files of the pass preceding .combine and that of .combine itself?

> On the other hand, it's been investigated, and it's a clear bug with a
> one-line fix.

This needs to be double checked though, this code has been there for ages.

[Bug rtl-optimization/99114] [WORD_REGISTER_OPERATIONS] wrong code for (u16_var & 3) == (u32)1

[pipcet at gmail dot com]

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

pipcet at gmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pipcet at gmail dot com

--- Comment #5 from pipcet at gmail dot com ---
Created attachment 50203
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50203&action=edit
RTL dump of last pre-combine pass

(Gzipped because of the file size limit). The relevant section is around this
code:

(note 4724 4779 4727 667 [bb 667] NOTE_INSN_BASIC_BLOCK)
(insn 4727 4724 4728 667 (set (reg:SI 2326)
        (ashift:SI (subreg:SI (reg:HI 799 [ hrsi$word_no ]) 0)
            (const_int 16 [0x10]))) "../../../src/gcc/gcc/hard-reg-set.h":311:7
10 {*assignsi_binop}
     (nil))
(insn 4728 4727 4729 667 (set (reg:SI 2326)
        (lshiftrt:SI (reg:SI 2326)
            (const_int 16 [0x10]))) "../../../src/gcc/gcc/hard-reg-set.h":311:7
10 {*assignsi_binop}
     (nil))
(jump_insn 4729 4728 4730 667 (set (pc)
        (if_then_else (gtu (reg:SI 2326)
                (const_int 1 [0x1]))
            (label_ref 4783)
            (pc))) "../../../src/gcc/gcc/hard-reg-set.h":311:7 20 {*cbranchsi4}
     (expr_list:REG_DEAD (reg:SI 2326)
        (int_list:REG_BR_PROB 39298956 (nil)))
 -> 4783)

[Bug rtl-optimization/99114] [WORD_REGISTER_OPERATIONS] wrong code for (u16_var & 3) == (u32)1

[pipcet at gmail dot com]

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

--- Comment #6 from pipcet at gmail dot com ---
Created attachment 50204
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50204&action=edit
RTL dump of combine pass

(Gzipped because of the file size limit).

The relevant section is around this code:

(note 4724 4779 4727 667 [bb 667] NOTE_INSN_BASIC_BLOCK)
(note 4727 4724 4728 667 NOTE_INSN_DELETED)
(note 4728 4727 4729 667 NOTE_INSN_DELETED)
(jump_insn 4729 4728 4730 667 (set (pc)
        (if_then_else (gtu (subreg:SI (reg:HI 799 [ hrsi$word_no ]) 0)
                (const_int 1 [0x1]))
            (label_ref 4783)
            (pc))) "../../../src/gcc/gcc/hard-reg-set.h":311:7 20 {*cbranchsi4}
     (int_list:REG_BR_PROB 39298956 (nil))
 -> 4783)


allowing combination of insns 4727 and 4728
original costs 4 + 4 = 8
replacement cost 4
deferring deletion of insn with uid = 4727.
modifying insn i3  4728: r2326:SI=r799:HI#0&0x3
deferring rescan insn with uid = 4728.
allowing combination of insns 4728 and 4729
original costs 4 + 4 = 8
replacement cost 4
deferring deletion of insn with uid = 4728.
modifying insn i3  4729: pc={(gtu(r799:HI#0,0x1))?L4783:pc}
      REG_BR_PROB 39298956
deferring rescan insn with uid = 4729.

[Bug rtl-optimization/99114] [WORD_REGISTER_OPERATIONS] wrong code for (u16_var & 3) == (u32)1

[pipcet at gmail dot com]

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

--- Comment #7 from pipcet at gmail dot com ---
(In reply to Eric Botcazou from comment #4)
> > I'll try, but please consider investigating this without one. It happens
> > after a very lengthy compilation process (compiling a buggy gcc with a buggy
> > cross-compiler, then compiling JIT code with that, generating a buggy .so,
> > then running _that_, to see incorrect behavior rather than a clear crash),
> > in C++ code, with an out-of-tree backend.
> 
> Impressive indeed, but not without a precedent.  Can you upload the RTL dump
> files of the pass preceding .combine and that of .combine itself?

Sure. I've tried taking the .i and compiling it with other backends, but they
never seem to optimize away the inner (and:HI ...).

> > On the other hand, it's been investigated, and it's a clear bug with a
> > one-line fix.
> 
> This needs to be double checked though, this code has been there for ages.

I agree, of course. While I still believe that the problem is as I described,
it's not as severe a bug as I initially thought: it essentially requires the
architecture to have WORD_REGISTER_OPERATIONS but also memory compares, a very
unusual combination for real hardware.

(In reply to Richard Biener from comment #3)
> If
> 
> (gtu (subreg:SI (reg:HI 593)) (const_int 1))
> 
> is incorrect, why is it then recognized?  And why should (subreg:SI (and:HI
> ..))
> be OK?

My understanding is it's valid but essentially meaningless RTL, so it should be
okay to recognize.

> The way I read WORD_REGISTER_OPERATIONS it's a bad design to make the IL do
> something that could have better been explicitely represented.  (if it is
> a SImode op then please make it an SImode op!)

Maybe a first step would be to add a "when in doubt, don't define it" remark to
the documentation for WORD_REGISTER_OPERATIONS?