[Bug target/103302] New: wrong code with -fharden-compares

[Bug target/103302] New: wrong code with -fharden-compares

[zsojka at seznam dot cz]

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

            Bug ID: 103302
           Summary: wrong code with -fharden-compares
           Product: gcc
           Version: 12.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: riscv64-unknown-linux-gnu

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

Output:
$ riscv64-unknown-linux-gnu-gcc -Og -fharden-compares -fno-tree-dce
-fno-tree-fre testcase.c -static
$ qemu-riscv64 -- ./a.out 
Trace/breakpoint trap

Debugging shows one of the compares on the first statement probably goes wrong:

(gdb) 
0x00000000000106ea      19        u32_0 <= (v512u128) (v512u128_0 != u8_0);
(gdb) 
0x0000000000010a48      24      }

I could find any other target where this testcase is failing.

$ riscv64-unknown-linux-gnu-gcc -v
Using built-in specs.
COLLECT_GCC=/repo/gcc-trunk/binary-latest-riscv64/bin/riscv64-unknown-linux-gnu-gcc
COLLECT_LTO_WRAPPER=/repo/gcc-trunk/binary-trunk-r12-5221-20211113015125-g2f3d43a3515-checking-yes-rtl-df-extra-riscv64/bin/../libexec/gcc/riscv64-unknown-linux-gnu/12.0.0/lto-wrapper
Target: riscv64-unknown-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
--with-sysroot=/usr/riscv64-unknown-linux-gnu --build=x86_64-pc-linux-gnu
--host=x86_64-pc-linux-gnu --target=riscv64-unknown-linux-gnu
--with-ld=/usr/bin/riscv64-unknown-linux-gnu-ld
--with-as=/usr/bin/riscv64-unknown-linux-gnu-as --disable-multilib
--disable-libstdcxx-pch
--prefix=/repo/gcc-trunk//binary-trunk-r12-5221-20211113015125-g2f3d43a3515-checking-yes-rtl-df-extra-riscv64
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 12.0.0 20211113 (experimental) (GCC)

[Bug target/103302] wrong code with -fharden-compares

[aoliva at gcc dot gnu.org]

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

Alexandre Oliva <aoliva at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |aoliva at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2021-11-19
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #1 from Alexandre Oliva <aoliva at gcc dot gnu.org> ---
Mine

[Bug target/103302] wrong code with -fharden-compares

[wilson at gcc dot gnu.org]

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

Jim Wilson <wilson at gcc dot gnu.org> changed:

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

--- Comment #2 from Jim Wilson <wilson at gcc dot gnu.org> ---
It is the second reversed comparison that is wrong.  This is the
  u32_0 <= (...)
on the first line of foo0.  In the assembly file, this ends up as
        mv      a0,a6
        mv      a1,a7
        xor     a6,a0,a6
        xor     a7,a1,a7
        or      a6,a6,a7
        seqz    a6,a6
and note that it is comparing a value against itself when it should be
comparing two different values.

The harden compare pass is generating RTL

insn 156 152 155 6 (set (reg:TI 201)
        (asm_operands:TI ("") ("=g") 0 [
                (reg:TI 77 [ _8 ])
            ]
             [
                (asm_input:TI ("0"))
            ]
             [])) -1
     (nil))
(insn 155 156 153 6 (clobber (reg:TI 77 [ _8 ])) -1
     (nil))
(insn 153 155 154 6 (set (subreg:DI (reg:TI 77 [ _8 ]) 0)
        (subreg:DI (reg:TI 201) 0)) -1
     (nil))
(insn 154 153 160 6 (set (subreg:DI (reg:TI 77 [ _8 ]) 8)
        (subreg:DI (reg:TI 201) 8)) -1
     (nil))

Then the asmcons pass is changing this to

(insn 851 152 849 5 (clobber (reg:TI 201)) -1
     (nil))
(insn 849 851 850 5 (set (subreg:DI (reg:TI 201) 0)
        (subreg:DI (reg:TI 77 [ _8 ]) 0)) -1
     (nil))
(insn 850 849 156 5 (set (subreg:DI (reg:TI 201) 8)
        (subreg:DI (reg:TI 77 [ _8 ]) 8)) -1
     (nil))
(insn 156 850 155 5 (set (reg:TI 201)
        (asm_operands:TI ("") ("=g") 0 [
                (reg:TI 201)
            ]
             [
                (asm_input:TI ("0"))
            ]
             [])) -1
     (expr_list:REG_DEAD (reg:TI 77 [ _8 ])
        (nil)))
(insn 155 156 153 5 (clobber (reg:TI 77 [ _8 ])) -1
     (nil))
(insn 153 155 154 5 (set (subreg:DI (reg:TI 77 [ _8 ]) 0)
        (subreg:DI (reg:TI 201) 0)) 135 {*movdi_64bit}
     (nil))
(insn 154 153 854 5 (set (subreg:DI (reg:TI 77 [ _8 ]) 8)
        (subreg:DI (reg:TI 201) 8)) 135 {*movdi_64bit}
     (expr_list:REG_DEAD (reg:TI 201)
        (nil)))

Then the register allocator puts both 77 and 201 in the same register, which
means we are now clobbering values we need.

In the reload dump I see

(insn 851 152 849 5 (clobber (reg:TI 16 a6 [201])) -1
     (nil))
(insn 849 851 850 5 (set (reg:DI 16 a6 [201])
        (reg:DI 10 a0 [orig:77 _8 ] [77])) 135 {*movdi_64bit}
     (nil))
(insn 850 849 907 5 (set (reg:DI 17 a7 [+8 ])
        (reg:DI 11 a1 [ _8+8 ])) 135 {*movdi_64bit}
     (nil))
(insn 907 850 1014 5 (clobber (reg:TI 16 a6 [201])) -1
     (nil))

so the insns 849 and 850 get optimized away, but we need them.  Also, we have

(insn 854 154 852 5 (clobber (reg:TI 16 a6 [202])) -1
     (nil))
(insn 852 854 853 5 (set (reg:DI 16 a6 [202])
        (reg:DI 6 t1 [orig:86 _39 ] [86])) 135 {*movdi_64bit}
     (nil))
(insn 853 852 913 5 (set (reg:DI 17 a7 [+8 ])
        (reg:DI 7 t2 [ _39+8 ])) 135 {*movdi_64bit}
     (nil))
(insn 913 853 1010 5 (clobber (reg:TI 16 a6 [202])) -1
     (nil))

and the insns 852 and 853 get optimized away, but we need them.  The comparison
is supposed to be a0/a1 versus t1/t2, but we end up with comparing a6/a7
against itself.

asmcons is calling emit_move_insn to copy the asm source to the asm dest so it
can simplify the asm.  Since this is a multiword mode, and the riscv backend
doesn't have a movti pattern, this ends up calling emit_move_multi_word which
emits the extra clobber that causes the problem.

I suppose we could fix this by adding a movti pattern to the riscv backend to
avoid the clobbers but we shouldn't have to.  Though it would be interesting to
see if this maybe results in better code optimization.

It isn't clear exactly where the problem is.  Maybe asmcons shouldn't try to
fix an asm when the mode is larger than the word mode?  This could be left to
the register allocator to fix.  Or maybe harden compare shouldn't generate RTL
like this?  This could be a harden compare issue, or maybe an issue with the
RTL expander to emit the rtl differently.  Looks like the same issue with the
RTL expander calling emit_move_multi_word which generates the clobber.  Or
maybe a movti pattern is actually required now?

I did verify that disabling asmcons fixes the problem for this testcase.  I had
to hack the code in function.c to do that as there is no option to disable it.

[Bug target/103302] wrong code with -fharden-compares

[wilson at gcc dot gnu.org]

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

--- Comment #3 from Jim Wilson <wilson at gcc dot gnu.org> ---
Maybe the register allocator should remove clobbers of pseudos, instead of
turning them into clobbers of hard register pairs.  That would eliminate the
ambiguity after register allocation.  It is also true that we don't needs hard
reg clobbers.  The clobbers are only there for tracking pseudo reg subregs.

[Bug target/103302] wrong code with -fharden-compares

[wilson at gcc dot gnu.org]

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

--- Comment #4 from Jim Wilson <wilson at gcc dot gnu.org> ---
See also bug 103271 which can also be fixed by adding a movti pattern.

[Bug target/103302] wrong code with -fharden-compares

[aoliva at gcc dot gnu.org]

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

--- Comment #5 from Alexandre Oliva <aoliva at gcc dot gnu.org> ---
Hello, Jim,

Thanks for the investigation, that's useful.  I guess the register allocator
shouldn't choose to coalesce registers when there's a clobber afterwards, or it
should drop the clobber, since otherwise it clobbers the coalesced set.

Anyway, I'm reworking the asm that prevents optimization because of bug 103149
and bug 103097, it might end up avoiding this unfortunate situation.  We'll
see...

[Bug target/103302] wrong code with -fharden-compares

[aoliva at gcc dot gnu.org]

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

--- Comment #6 from Alexandre Oliva <aoliva at gcc dot gnu.org> ---
https://gcc.gnu.org/pipermail/gcc-patches/2021-December/585963.html appears to
no longer hit this error, though I've only inspected the asm output, not tried
to run it yet; can anyone confirm?

[Bug target/103302] wrong code with -fharden-compares

[zsojka at seznam dot cz]

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

--- Comment #7 from Zdenek Sojka <zsojka at seznam dot cz> ---
(In reply to Alexandre Oliva from comment #6)
> https://gcc.gnu.org/pipermail/gcc-patches/2021-December/585963.html appears
> to no longer hit this error, though I've only inspected the asm output, not
> tried to run it yet; can anyone confirm?

Thank you for the patch. Compiling the testcase with a patched compiler fixes
this issue for me (using userspace qemu emulation). The asm diff is too big,
due to different register allocation and stack offsets, for me to quickly tell
what exactly changes on the asm level.

[Bug target/103302] wrong code with -fharden-compares

[zsojka at seznam dot cz]

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

--- Comment #8 from Zdenek Sojka <zsojka at seznam dot cz> ---
Created attachment 51917
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51917&action=edit
testcase failing with the patch applied

The attached unreduced testcase is failing with the patch applied, but is OK
without the patch:

$
/repo/gcc-trunk/binary-trunk-r12-5705-20211202014655-g1c5317d6214-checking-yes-rtl-df-extra-pr103149-riscv64/bin/riscv64-unknown-linux-gnu-gcc
-O3 -fharden-compares -fno-toplevel-reorder -fno-tree-fre -fno-tree-vrp
-funroll-all-loops xxx.c -static -w
$ qemu-riscv64 -- ./a.out 
Trace/breakpoint trap

[Bug target/103302] wrong code with -fharden-compares

[aoliva at gcc dot gnu.org]

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

--- Comment #9 from Alexandre Oliva <aoliva at gcc dot gnu.org> ---
Thanks, this alternate testcase confirms my suspicion that the original issue
was only going latent.  It's clearly a preexisting register allocation issue on
riscv, that was latent and that -fharden-compares exposed rather than
introduced.

If I understand correctly its pervasiveness, it may very well make
-fharden-compares too risky to use on riscv, so I may be on the hook to fix it
regardless.

ISTM that a post-reload insn_and_split would at least avoid the problem.  Is
there any reason why this approach is not in use on riscv?

[Bug target/103302] wrong code with -fharden-compares

[aoliva at gcc dot gnu.org]

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

--- Comment #10 from Alexandre Oliva <aoliva at gcc dot gnu.org> ---
Created attachment 51947
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=51947&action=edit
candidate patch under testing

Could the fix be as simple as this?

The resulting code is awful, with such stuff as loading and storing back each
word of multi-word pseudos that end up assigned to memory, but at least we
don't clobber the asm outputs with these clobbers, which is what makes them
look dead.

Anyhow, comments at the top of emit_move_multi_word state:

   Note that you will get better code if you define such
   patterns, even if they must turn into multiple assembler instructions.

[Bug target/103302] wrong code with -fharden-compares

[wilson at gcc dot gnu.org]

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

--- Comment #11 from Jim Wilson <wilson at gcc dot gnu.org> ---
FYI I have a patch to re-add the movti pattern to riscv.md which should also
fix this and another bug.  Kito removed the pattern in 2016 and I was hoping to
get an answer from him about why he removed it.  The RISC-V Summit is this week
so not much development work happening this week from RISC-V developers.  And
in general I've been in transition away from SiFive and not getting much done.

[Bug target/103302] wrong code with -fharden-compares

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Alexandre Oliva <aoliva@gcc.gnu.org>:

https://gcc.gnu.org/g:50e8b0c9bca6cdc57804f860ec5311b641753fbb

commit r12-5852-g50e8b0c9bca6cdc57804f860ec5311b641753fbb
Author: Alexandre Oliva <oliva@adacore.com>
Date:   Wed Dec 8 23:37:14 2021 -0300

    [PR103302] skip multi-word pre-move clobber during lra

    If we emit clobbers before multi-word moves during lra, we get
    confused if a copy ends up with input or output replaced with each
    other: the clobber then kills the previous set, and it gets deleted.

    This patch avoids emitting such clobbers when lra_in_progress.


    for  gcc/ChangeLog

            PR target/103302
            * expr.c (emit_move_multi_word): Skip clobber during lra.

    for  gcc/testsuite/ChangeLog

            PR target/103302
            * gcc.target/riscv/pr103302.c: New.

[Bug target/103302] wrong code with -fharden-compares

[aoliva at gcc dot gnu.org]

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

Alexandre Oliva <aoliva at gcc dot gnu.org> changed:

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

--- Comment #13 from Alexandre Oliva <aoliva at gcc dot gnu.org> ---
Fixed

[Bug target/103302] wrong code with -fharden-compares

[aoliva at gcc dot gnu.org]

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

Alexandre Oliva <aoliva at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|FIXED                       |---
                URL|https://gcc.gnu.org/piperma |
                   |il/gcc-patches/2021-Decembe |
                   |r/586370.html               |

--- Comment #14 from Alexandre Oliva <aoliva at gcc dot gnu.org> ---
The installed patch is wrong and will be reverted.

[Bug target/103302] wrong code with -fharden-compares

[cvs-commit at gcc dot gnu.org]

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

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Alexandre Oliva <aoliva@gcc.gnu.org>:

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

commit r12-7384-ga9e2ebe839d56416ceaff1a40df54de4890539be
Author: Alexandre Oliva <oliva@adacore.com>
Date:   Thu Feb 24 22:03:40 2022 -0300

    Revert commit r12-5852-g50e8b0c9bca6cdc57804f860ec5311b641753fbb

    The patch for PR103302 caused PR104121, and extended the live ranges
    of LRA reloads.


    for gcc/ChangeLog

            PR target/104121
            PR target/103302
            * expr.cc (emit_move_multi_word): Restore clobbers during LRA.

[Bug target/103302] wrong code with -fharden-compares

[aoliva at gcc dot gnu.org]

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

--- Comment #16 from Alexandre Oliva <aoliva at gcc dot gnu.org> ---
Created attachment 52518
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=52518&action=edit
Candidate patch

The problem is in undo_optional_reloads.  Here's a fix I'm testing.

[Bug target/103302] wrong code with -fharden-compares

[aoliva at gcc dot gnu.org]

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

Alexandre Oliva <aoliva at gcc dot gnu.org> changed:

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

--- Comment #17 from Alexandre Oliva <aoliva at gcc dot gnu.org> ---
The patch went in on 2022-03-02, commit
12f8dc0b642db5edc702f252af1a5231606b29db, but I failed to tag this PR in the
commit message.

[Bug target/103302] wrong code with -fharden-compares

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |12.0

--- Comment #18 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Alexandre Oliva from comment #17)
> The patch went in on 2022-03-02, commit
> 12f8dc0b642db5edc702f252af1a5231606b29db, but I failed to tag this PR in the
> commit message.

r12-7453-g12f8dc0b642db5edc702f