[PATCH 0/4] Some improvements to resource.cc, including fixing PR115182

[PATCH 0/4] Some improvements to resource.cc, including fixing PR115182

[Hans-Peter Nilsson]

The code in resource.cc is exclusively used by the delay-slot-filling
machinery: the "dbr" pass in reorg.cc, sometimes referred to just as
"reorg".  Its implementation is quite arcane, scanning RTL, with only
a little dash of cfgrtl.  I'm sure some think that rather than fixing
this machinery, it better die with architectures having delay-slots.
Those people will hopefully still enjoy these patches, as the bulk is
code removal.

1: Only the PR115182 missed-optimization bug-fix.

2: Does not depend on 1, but corrects an incidentally found wart:
find_basic_block calls fails too often.  Replace it with "modern"
insn-to-basic-block cross-referencing.

3: Just an addendum to 2: removes an "if", where the condition is now
always-true, dominated by a gcc_assert, and where the change in
indentation was too ugly.

4: Corrects another incidentally found wart: for the last 15 years the
code in resource.cc has only been called from within reorg.cc (and
reorg.c), specifically not possibly before calling init_resource_info
or after free_resource_info, so we can discard the code that tests
certain allocated arrays for NULL.  I didn't even bother with a
gcc_assert; besides some gen*-generated files, only reorg.cc includes
resource.h (not to be confused with the system sys/resource.h).
A grep says the #include resource.h can be removed from those gen*
files and presumably from RESOURCE_H(!) as well.  Some Other Time.
Also, removed a redundant "if (tinfo != NULL)" and moved the then-code
into the previous then-clause.

Hans-Peter Nilsson (4):
  resource.cc (mark_target_live_regs): Don't look past target insn,
    PR115182
  resource.cc: Replace calls to find_basic_block with cfgrtl
    BLOCK_FOR_INSN
  resource.cc (mark_target_live_regs): Remove check for bb not found
  resource.cc: Remove redundant conditionals

 gcc/resource.cc | 664 +++++++++++++-----------------------------------
 1 file changed, 179 insertions(+), 485 deletions(-)

-- 
2.30.2

Re: [PATCH 0/4] Some improvements to resource.cc, including fixing PR115182

[Jeff Law]

On 5/27/24 11:51 AM, Hans-Peter Nilsson wrote:
> The code in resource.cc is exclusively used by the delay-slot-filling
> machinery: the "dbr" pass in reorg.cc, sometimes referred to just as
> "reorg".  Its implementation is quite arcane, scanning RTL, with only
> a little dash of cfgrtl.  I'm sure some think that rather than fixing
> this machinery, it better die with architectures having delay-slots.
> Those people will hopefully still enjoy these patches, as the bulk is
> code removal.
Well, I always wanted it changed to use the scheduler's dependency 
analysis code to give us the candidate set of insns for delay slot 
filling rather than doing a bespoke life analysis.  But we never got 
that implemented and delay slot architectures have diminished in their 
importance over the last 25 years :-)

Jeff

Reverted recent patches to resource.cc

[Hans-Peter Nilsson]

> From: Hans-Peter Nilsson <hp@axis.com>
> Date: Mon, 27 May 2024 19:51:47 +0200

> 2: Does not depend on 1, but corrects an incidentally found wart:
> find_basic_block calls fails too often.  Replace it with "modern"
> insn-to-basic-block cross-referencing.
> 
> 3: Just an addendum to 2: removes an "if", where the condition is now
> always-true, dominated by a gcc_assert, and where the change in
> indentation was too ugly.
> 
> 4: Corrects another incidentally found wart: for the last 15 years the
> code in resource.cc has only been called from within reorg.cc (and
> reorg.c), specifically not possibly before calling init_resource_info
> or after free_resource_info, so we can discard the code that tests
> certain allocated arrays for NULL.  I didn't even bother with a
> gcc_assert; besides some gen*-generated files, only reorg.cc includes
> resource.h (not to be confused with the system sys/resource.h).
> A grep says the #include resource.h can be removed from those gen*
> files and presumably from RESOURCE_H(!) as well.  Some Other Time.
> Also, removed a redundant "if (tinfo != NULL)" and moved the then-code
> into the previous then-clause.
> 
>   resource.cc: Replace calls to find_basic_block with cfgrtl
>     BLOCK_FOR_INSN
>   resource.cc (mark_target_live_regs): Remove check for bb not found
>   resource.cc: Remove redundant conditionals

I had to revert those last three patches due to PR
bootstrap/115284.  I hope to revisit once I have a means to
reproduce (and fix) the underlying bug.  It doesn't have to
be a bug with those changes per-se: IMHO the "improved"
lifetimes could just as well have uncovered a bug elsewhere
in reorg.  It's still on me to resolve that situation; done.
I'm just glad the cause was the incidental improvements and
not the original bug I wanted to fix.

There appears to be only a single supported SPARC machine in
cfarm: cfarm216, and I currently can't reach it due to what
appears to be issues at my end.  I guess I'll either fix
that or breathe life into sparc-elf+sim.

brgds, H-P

Re: Reverted recent patches to resource.cc

[Jeff Law]

On 5/29/24 7:28 PM, Hans-Peter Nilsson wrote:
>> From: Hans-Peter Nilsson <hp@axis.com>
>> Date: Mon, 27 May 2024 19:51:47 +0200
> 
>> 2: Does not depend on 1, but corrects an incidentally found wart:
>> find_basic_block calls fails too often.  Replace it with "modern"
>> insn-to-basic-block cross-referencing.
>>
>> 3: Just an addendum to 2: removes an "if", where the condition is now
>> always-true, dominated by a gcc_assert, and where the change in
>> indentation was too ugly.
>>
>> 4: Corrects another incidentally found wart: for the last 15 years the
>> code in resource.cc has only been called from within reorg.cc (and
>> reorg.c), specifically not possibly before calling init_resource_info
>> or after free_resource_info, so we can discard the code that tests
>> certain allocated arrays for NULL.  I didn't even bother with a
>> gcc_assert; besides some gen*-generated files, only reorg.cc includes
>> resource.h (not to be confused with the system sys/resource.h).
>> A grep says the #include resource.h can be removed from those gen*
>> files and presumably from RESOURCE_H(!) as well.  Some Other Time.
>> Also, removed a redundant "if (tinfo != NULL)" and moved the then-code
>> into the previous then-clause.
>>
>>    resource.cc: Replace calls to find_basic_block with cfgrtl
>>      BLOCK_FOR_INSN
>>    resource.cc (mark_target_live_regs): Remove check for bb not found
>>    resource.cc: Remove redundant conditionals
> 
> I had to revert those last three patches due to PR
> bootstrap/115284.  I hope to revisit once I have a means to
> reproduce (and fix) the underlying bug.  It doesn't have to
> be a bug with those changes per-se: IMHO the "improved"
> lifetimes could just as well have uncovered a bug elsewhere
> in reorg.  It's still on me to resolve that situation; done.
> I'm just glad the cause was the incidental improvements and
> not the original bug I wanted to fix.
> 
> There appears to be only a single supported SPARC machine in
> cfarm: cfarm216, and I currently can't reach it due to what
> appears to be issues at my end.  I guess I'll either fix
> that or breathe life into sparc-elf+sim.
Or if you've got a reasonable server to use, QEMU might save you :-)

I do bootstraps and regression testsuite runs on a variety of systems 
via qemu (alpha, m68k, aarch64, s390, ppc64, etc).  It ain't fast, but 
it does work if QEMU is in pretty good shape and you can find a root 
filesystem to use.

jeff

Re: Reverted recent patches to resource.cc

[Hans-Peter Nilsson]

> Date: Wed, 29 May 2024 20:07:22 -0600
> From: Jeff Law <jeffreyalaw@gmail.com>

> > There appears to be only a single supported SPARC machine in
> > cfarm: cfarm216, and I currently can't reach it due to what
> > appears to be issues at my end.  I guess I'll either fix
> > that or breathe life into sparc-elf+sim.
> Or if you've got a reasonable server to use, QEMU might save you :-)

I believe so. :)

> I do bootstraps and regression testsuite runs on a variety of systems 
> via qemu (alpha, m68k, aarch64, s390, ppc64, etc).  It ain't fast, but 
> it does work if QEMU is in pretty good shape and you can find a root 
> filesystem to use.

That might certainly fit the bill.  I guess you mean with a
filesystem image for e.g. sparc-linux?

I keep postponing looking into getting a working setup
(mostly the baseboard file) for qemu-anything + newlib.
Last I looked, qemu.exp had a serious typo...but I see that
was just for arm-eabi and arm-pi4, so yes, that might be a
viable path, thanks for the reminder.

You (or anyone) don't happen to know if sparc-elf + qemu.exp
is in good shape, or some other specific sparc+qemu
configuration?

That "sparc=*" in qemu.exp entry (at dejagnu
ca371cf9c48186716d) looks suspicious though, so I guess it'd
be a tuple matching "sparc32plus-*" or "sparc64-*".

brgds, H-P

Re: Reverted recent patches to resource.cc

[Jeff Law]

On 5/29/24 8:41 PM, Hans-Peter Nilsson wrote:

> 
>> I do bootstraps and regression testsuite runs on a variety of systems
>> via qemu (alpha, m68k, aarch64, s390, ppc64, etc).  It ain't fast, but
>> it does work if QEMU is in pretty good shape and you can find a root
>> filesystem to use.
> 
> That might certainly fit the bill.  I guess you mean with a
> filesystem image for e.g. sparc-linux?
> 
> I keep postponing looking into getting a working setup
> (mostly the baseboard file) for qemu-anything + newlib.
> Last I looked, qemu.exp had a serious typo...but I see that
> was just for arm-eabi and arm-pi4, so yes, that might be a
> viable path, thanks for the reminder.
I don't bother with qemu.exp at all.  I've set up binfmt handlers so 
that I can execute foreign binaries.

So given a root filesystem, I can chroot into it and do whatever I need. 
  As far as dejagnu is concerned it looks like the native system.


Jeff

Re: Reverted recent patches to resource.cc

[Hans-Peter Nilsson]

> Date: Wed, 29 May 2024 21:23:58 -0600
> Cc: gcc-patches@gcc.gnu.org

> I don't bother with qemu.exp at all.  I've set up binfmt handlers so 
> that I can execute foreign binaries.
> 
> So given a root filesystem, I can chroot into it and do whatever I need. 
>   As far as dejagnu is concerned it looks like the native system.

Interesting.  In a Docker setup (or similar container)?
If so, care to share the Dockerfile (or equivalent)?

A quick web search shows similar attempts, but I found
nothing "turn-key ready" that enables bootstrapping.

I'll try to cook up something, likely Debian-based...later
this decade, aiming for suitability in contrib/.

brgds, H-P
ps. my hyperbolic time guesstimates somehow still expire too soon!

Re: Reverted recent patches to resource.cc

[Jeff Law]

On 5/30/24 8:09 PM, Hans-Peter Nilsson wrote:
>> Date: Wed, 29 May 2024 21:23:58 -0600
>> Cc: gcc-patches@gcc.gnu.org
> 
>> I don't bother with qemu.exp at all.  I've set up binfmt handlers so
>> that I can execute foreign binaries.
>>
>> So given a root filesystem, I can chroot into it and do whatever I need.
>>    As far as dejagnu is concerned it looks like the native system.
> 
> Interesting.  In a Docker setup (or similar container)?
I've got docker containers for some of these.  m68k, alpha, s390 for 
example.   They're based off debian base images.  I don't have anything 
for sparc and I don't see a base image to build from.  If there was a 
base image, then a Dockerfile like this would get you started:


FROM sparc64/debian:latest

# Basic environment variables so we can apt-get without interactive prompts
ENV LC_ALL C
ENV DEBIAN_FRONTEND noninteractive
ENV TZ=Etc/UTC

RUN apt-get update && apt-get -y install gcc dejagnu binutils 
default-jre-headless git build-essential autoconf bison flex gawk make 
texinfo help2man libncurses5-dev python3-dev python3-distutils libtool
libtool-bin unzip wget curl rsync texinfo g++ libmpc-dev libgmp-dev 
libmpfr-dev libgmp-dev python3 libisl-dev rsync vim automake autoconf 
autotools-dev unzip help2man libtool libtool-bin sudo curl wget pyt
hon3-dev bzip2 xz-utils gdb bc libssl-dev libelf-dev


With the lack of an existing docker image, you can probably use 
debootstrap to construct an initial chroot, the import that into docker, 
adding whatever bits you need to do the build (ie, compilers, dejagnu, 
make, texinfo, blah blah blah) via apt until it works.  It's been a 
while since I've done it, but I'm pretty sure that's how I got things 
going on the sh4 and sh4eb platforms.


The JRE bits are only needed because these get launched as a docker 
swarm service and thus need to connect to the Jenkins server using JNLP. 
  Some of the packages are only needed for kernel builds or glibc builds.


Jeff

Re: Reverted recent patches to resource.cc

[Jeff Law]

On 5/29/24 8:07 PM, Jeff Law wrote:
> 
> 
> On 5/29/24 7:28 PM, Hans-Peter Nilsson wrote:
>>> From: Hans-Peter Nilsson <hp@axis.com>
>>> Date: Mon, 27 May 2024 19:51:47 +0200
>>
>>> 2: Does not depend on 1, but corrects an incidentally found wart:
>>> find_basic_block calls fails too often.  Replace it with "modern"
>>> insn-to-basic-block cross-referencing.
>>>
>>> 3: Just an addendum to 2: removes an "if", where the condition is now
>>> always-true, dominated by a gcc_assert, and where the change in
>>> indentation was too ugly.
>>>
>>> 4: Corrects another incidentally found wart: for the last 15 years the
>>> code in resource.cc has only been called from within reorg.cc (and
>>> reorg.c), specifically not possibly before calling init_resource_info
>>> or after free_resource_info, so we can discard the code that tests
>>> certain allocated arrays for NULL.  I didn't even bother with a
>>> gcc_assert; besides some gen*-generated files, only reorg.cc includes
>>> resource.h (not to be confused with the system sys/resource.h).
>>> A grep says the #include resource.h can be removed from those gen*
>>> files and presumably from RESOURCE_H(!) as well.  Some Other Time.
>>> Also, removed a redundant "if (tinfo != NULL)" and moved the then-code
>>> into the previous then-clause.
>>>
>>>    resource.cc: Replace calls to find_basic_block with cfgrtl
>>>      BLOCK_FOR_INSN
>>>    resource.cc (mark_target_live_regs): Remove check for bb not found
>>>    resource.cc: Remove redundant conditionals
>>
>> I had to revert those last three patches due to PR
>> bootstrap/115284.  I hope to revisit once I have a means to
>> reproduce (and fix) the underlying bug.  It doesn't have to
>> be a bug with those changes per-se: IMHO the "improved"
>> lifetimes could just as well have uncovered a bug elsewhere
>> in reorg.  It's still on me to resolve that situation; done.
>> I'm just glad the cause was the incidental improvements and
>> not the original bug I wanted to fix.
>>
>> There appears to be only a single supported SPARC machine in
>> cfarm: cfarm216, and I currently can't reach it due to what
>> appears to be issues at my end.  I guess I'll either fix
>> that or breathe life into sparc-elf+sim.
> Or if you've got a reasonable server to use, QEMU might save you :-)
> 

Even better option.  The sh4/sh4eb-linux-gnu ports with 
execute/ieee/fp-cmp-5.c test.  That started execution failing at -O2 
with the first patch in the series and there are very clear assembly 
differences before/after your change.  Meaning you can probably look at 
them with just a cross compile and compare the before/after.


Jeff

Re: Reverted recent patches to resource.cc

[Hans-Peter Nilsson]

> Date: Sat, 8 Jun 2024 11:10:21 -0600
> From: Jeff Law <jeffreyalaw@gmail.com>

> >>>    resource.cc: Replace calls to find_basic_block with cfgrtl
> >>>      BLOCK_FOR_INSN
> >>>    resource.cc (mark_target_live_regs): Remove check for bb not found
> >>>    resource.cc: Remove redundant conditionals
> >>
> >> I had to revert those last three patches due to PR
> >> bootstrap/115284.  I hope to revisit once I have a means to
> >> reproduce (and fix) the underlying bug.  It doesn't have to
> >> be a bug with those changes per-se: IMHO the "improved"
> >> lifetimes could just as well have uncovered a bug elsewhere
> >> in reorg.  It's still on me to resolve that situation; done.
> >> I'm just glad the cause was the incidental improvements and
> >> not the original bug I wanted to fix.
> >>
> >> There appears to be only a single supported SPARC machine in
> >> cfarm: cfarm216, and I currently can't reach it due to what
> >> appears to be issues at my end.  I guess I'll either fix
> >> that or breathe life into sparc-elf+sim.
> > Or if you've got a reasonable server to use, QEMU might save you :-)
> > 
> 
> Even better option.  The sh4/sh4eb-linux-gnu ports with 
> execute/ieee/fp-cmp-5.c test.  That started execution failing at -O2 
> with the first patch in the series and there are very clear assembly 
> differences before/after your change.  Meaning you can probably look at 
> them with just a cross compile and compare the before/after.

Interesting, thanks for this.  (I'd expect assembly
differences, but only for slightly improved performance.)

Not sure when I'll revisit the underlying problem at
bootstrap/115284 though, perhaps not this week.  (For
context, for the gallery, for the record: the bootstrap
problem there is solved.)

brgds, H-P