[Bug target/95252] New: testcase gcc.dg/torture/pr67916.c failure when testing with -msave-restore

[Bug target/95252] New: testcase gcc.dg/torture/pr67916.c failure when testing with -msave-restore

[bina2374 at gmail dot com]

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

            Bug ID: 95252
           Summary: testcase gcc.dg/torture/pr67916.c failure when testing
                    with -msave-restore
           Product: gcc
           Version: 10.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: bina2374 at gmail dot com
                CC: kito at gcc dot gnu.org, wilson at gcc dot gnu.org
  Target Milestone: ---
            Target: riscv64-unknown-elf

FAIL: gcc.dg/torture/pr67916.c   -O3 -fomit-frame-pointer -funroll-loops
-fpeel-loops -ftracer -finline-functions  execution test

The ABI is lp64d, arch is rv64imafdc, and mcmodel is medany.

The full execution command line is:
<TOOLCHAIN-PATH>/bin/riscv64-unknown-elf-gcc
<GCC-SOURCE-PATH>/gcc/testsuite/gcc.dg/torture/pr67916.c
-fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers
-fdiagnostics-color=never -fdiagnostics-urls=never -msave-restore -O3
-fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions
-lm -o ./pr67916.exe

Execute with qemu:
$ <QEMU-PATH>/bin/qemu-riscv64./pr67916.exe
Segmentation fault (core dumped)

By the way, I try to reduce the number of compilation options, and found the
smallest combination of options that will cause an execution error:
<TOOLCHAIN-PATH>/bin/riscv64-unknown-elf-gcc
<GCC-SOURCE-PATH>/gcc/testsuite/gcc.dg/torture/pr67916.c -msave-restore -O3
-funroll-loops -lm  -o ./pr67916.exe

[Bug target/95252] testcase gcc.dg/torture/pr67916.c failure when testing with -msave-restore

[wilson at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-05-25
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Jim Wilson <wilson at gcc dot gnu.org> ---
It appears to be failing in the rename register (rnreg) pass.  This is because
the unspec patterns for the save/restore calls don't mention the registers that
they use/modify.  This confuses rename reg into thinking that live regs are
dead, and it accidentally clobbers them before the save call.  This worked OK
when save/restore calls could only be at the beginning or end of a function. 
But now that this works with tail calls and shrink wrapping, we can get them in
inner blocks.  Since the different save/restore calls use/modify different sets
of registers, fixing this gets a little complicated.  Maybe we can just use the
max list of registers because listing extra ones shouldn't matter?

Another solution is to disable the rename register pass when -msave-restore is
used.  This isn't doing any checking for whether regs can be used in compressed
instructions or not.  This is currently encoded in REG_ALLOC_ORDER which this
pass doesn't use.  The result is that this is probably increasing code size
which is undesirable when -msave-restore it used.  Disabling this would reduce
code size and fix the -msave-restore problem.

The rename register pass does use the PREFERRED_RENAME_CLASS hook that we
haven't defined.  We should try defining this to convert registers classes to
subsets that only include the regs that can be used in compressed instructions.
 This might result in a code size decrease.  If this works, then the rename reg
pass should not be disabled, and we should find a way to fix the save/restore
pattern register lists instead.

I need to do some builds and experiments to verify this info.

[Bug target/95252] testcase gcc.dg/torture/pr67916.c failure when testing with -msave-restore

[kito at gcc dot gnu.org]

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

--- Comment #2 from Kito Cheng <kito at gcc dot gnu.org> ---
Hi Jim:

Not sure which way you will try first, maybe Monk or me can try to fix this by
add bunch of pattern of gpr_save/gpr_restore to model set/use register
correctly?

[Bug target/95252] testcase gcc.dg/torture/pr67916.c failure when testing with -msave-restore

[wilson at gcc dot gnu.org]

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

--- Comment #3 from Jim Wilson <wilson at gcc dot gnu.org> ---
I tried both.  Turning off register naming works.  It gives a code size
decrease of about 0.003% for the libraries I looked at which can be ignored. 
This probably also reduces performance; I didn't check that.  I think it would
be better to leave register naming on and define the PREFERRED_RENAME_CLASS
hook.  

Adding uses to the gpr_save pattern also works for the testcase.  I just added
uses for all of the saved regs.  We shouldn't need an exact list, because there
is little or no code before the prologue, and the prologues are added late.  An
exact list would be cleaner if you want to try to do that.  I also needed to
fix riscv_remove_unneeded_save_restore_calls to ignore the prologue_matched
insn when checking for USEs to avoid gcc testsuite regressions.  I now have 3
g++ testsuite regressions I haven't looked at yet.
FAIL: g++.dg/torture/stackalign/throw-1.C   -O3 -fomit-frame-pointer
-funroll-loops -fpeel-loops -ftracer -finline-functions -fpic execution test
FAIL: g++.dg/torture/stackalign/throw-2.C   -O3 -fomit-frame-pointer
-funroll-loops -fpeel-loops -ftracer -finline-functions  execution test
FAIL: g++.dg/torture/stackalign/throw-2.C   -O3 -fomit-frame-pointer
-funroll-loops -fpeel-loops -ftracer -finline-functions -fpic execution test

[Bug target/95252] testcase gcc.dg/torture/pr67916.c failure when testing with -msave-restore

[wilson at gcc dot gnu.org]

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

--- Comment #4 from Jim Wilson <wilson at gcc dot gnu.org> ---
Created attachment 48624
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48624&action=edit
disable reg rename when -msave-restore

the code using MASK_SAVE_RESTORE is just for testing purposes

[Bug target/95252] testcase gcc.dg/torture/pr67916.c failure when testing with -msave-restore

[wilson at gcc dot gnu.org]

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

--- Comment #5 from Jim Wilson <wilson at gcc dot gnu.org> ---
Created attachment 48625
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48625&action=edit
add uses to gpr_save pattern

the code using MASK_SAVE_RESTORE is just for testing purposes
unfinished, adds 3 new g++ testsuite failures

[Bug target/95252] testcase gcc.dg/torture/pr67916.c failure when testing with -msave-restore

[kito at gcc dot gnu.org]

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

--- Comment #6 from Kito Cheng <kito at gcc dot gnu.org> ---
Oh, seems like add uses is enough, exact pattern might add about ~200 line to
md file include RV32E/RV32/RV64 I guess.

[Bug target/95252] testcase gcc.dg/torture/pr67916.c failure when testing with -msave-restore

[wilson at gcc dot gnu.org]

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

--- Comment #7 from Jim Wilson <wilson at gcc dot gnu.org> ---
I've got 3 new g++ testsuite failures.  So we might still need an exact list of
USEs.  I hadn't thought about RVE.  That will have to be checked also. 
RV32/RV64 shouldn't matter, as the mode in the USEs doesn't matter.  Unless
maybe you want to use a multi-word load to match multiple registers with a
single USE to reduce the size of the patterns, in which case it would need to
be different for rv32 and rv64.  If we do need an exact list of USEs, maybe we
can use a match_parallel to simplify the patterns.

[Bug target/95252] testcase gcc.dg/torture/pr67916.c failure when testing with -msave-restore

[kito.cheng at gmail dot com]

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

Kito Cheng <kito.cheng at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kito.cheng at gmail dot com

--- Comment #8 from Kito Cheng <kito.cheng at gmail dot com> ---
It seems like because register rename thought all callee-saved register can be
used, because df_regs_ever_live_p return true for all callee-saved register
now, so seems like we really need an exact pattern to describe that, that's put
into my tomorrows TODO list.

[Bug target/95252] testcase gcc.dg/torture/pr67916.c failure when testing with -msave-restore

[kito at gcc dot gnu.org]

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

Kito Cheng <kito at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #9 from Kito Cheng <kito at gcc dot gnu.org> ---
Just update the status from my side, I have a workable version for exact list
of USEs, and under clean up.

[Bug target/95252] testcase gcc.dg/torture/pr67916.c failure when testing with -msave-restore

[kito at gcc dot gnu.org]

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

Kito Cheng <kito at gcc dot gnu.org> changed:

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

--- Comment #10 from Kito Cheng <kito at gcc dot gnu.org> ---
Seems like I forgot mention the PR in the ChangeLog so the bot can't detect
that, but the ChangeLog is auto-gen now, so I don't know how to fix that.

Anyway it already committed into trunk and plan to back port to gcc-10 branch
next week.

https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=d0e0c1300f9f08608873df5571e14a61308dd0c0