Re: [PATCH GCC][3/6]New file computing regional register pressure on TREE SSA

["Bin.Cheng" <amker dot cheng at gmail dot com>]

On Tue, Jun 27, 2017 at 6:40 PM, Jeff Law <law@redhat.com> wrote:
> On 05/12/2017 05:28 AM, Bin Cheng wrote:
>> Hi,
>> This patch computes register pressure information on TREE SSA by a backward live
>> range data flow problem.  The major motivation is to estimate register pressure
>> for inner-most loop on TREE SSA, then other optimizations can use it.  So far the
>> information is used only in predcom later, but it could be useful to implement a
>> tree level scheduler in order to shrink live ranges.  Unfortunately the example
>> live range shrink pass I implemented doesn't have obvious impact on performance.
>> I think one reason is TER which effectively undoes its effect.  Maybe it will be
>> useful once TER/expanding is replaced with a better instruction selector, it is
>> not included in this patch.
>> One fact I need to mention is David proposed a similar patch long time ago at
>> https://gcc.gnu.org/ml/gcc-patches/2008-12/msg01261.html.  It tries to compute
>> register pressure information on tree ssa and shrink live ranges based on that
>> information.  Unfortunately the patch wasn't merged in the end.  There has been
>> quite changes in GCC implementation, I didn't use its code directly.  However,
>> I did read that patch and had it in mind when implementing this one.  If there
>> is any issue in this patch, it would be me that should be blamed.  I also sent
>> message to David about this patch and the possible relation with his.
>>
>> Bootstrap and test on x86_64 and AArch64.  Is it OK?
>>
>> Thanks,
>> bin
>>
>> 2017-05-10  Xinliang David Li  <davidxl@google.com>
>>           Bin Cheng  <bin.cheng@arm.com>
>>
>>       * Makefile.in (tree-ssa-regpressure.o): New object file.
>>       * tree-ssa-regpressure.c: New file.
>>       * tree-ssa-regpressure.h: New file.
> Any thoughts on tests, either end-to-end or unit testing?
>
> At a high level does this make more sense as a pass or as a function
> that is called by other passes?  I don't have a strong opinion here,
> just putting the question out there for discussion.
>
> You've got a live computation solver in here.  Is there some reason you
> don't use the existing life analysis code?   I'd prefer not have have
> another life analysis implementation if we can avoid it.  And if you
> were using that code, I think you can easily get the coalescing data
> you're using as well.
>
> I haven't gone through all the detail in the patch as I think we need to
> make sure we've got the design issues right first.  BUt there are a
> couple nits noted inline below.
>
>
Hi Jeff, Thanks very much for the review.  I plan to revisit this
after current tasks as well as study more benchmark cases for register
pressure.

Thanks,
bin
>
>
>
>
>>
>>
>> 0003-tree-ssa-regpressure-20170504.txt
>>
>>
>> From bf6e51ff68d87c372719de567d4de49d77744f77 Mon Sep 17 00:00:00 2001
>> From: Bin Cheng <binche01@e108451-lin.cambridge.arm.com>
>> Date: Mon, 8 May 2017 15:20:27 +0100
>> Subject: [PATCH 3/6] tree-ssa-regpressure-20170504.txt
>>
>> ---
>>  gcc/Makefile.in            |   1 +
>>  gcc/tree-ssa-regpressure.c | 829 +++++++++++++++++++++++++++++++++++++++++++++
>>  gcc/tree-ssa-regpressure.h |  21 ++
>>  3 files changed, 851 insertions(+)
>>  create mode 100644 gcc/tree-ssa-regpressure.c
>>  create mode 100644 gcc/tree-ssa-regpressure.h
>>
>> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
>> index 97259ac..abfd4bc 100644
>> --- a/gcc/Makefile.in
>> +++ b/gcc/Makefile.in
>> @@ -1534,6 +1534,7 @@ OBJS = \
>>       tree-ssa-pre.o \
>>       tree-ssa-propagate.o \
>>       tree-ssa-reassoc.o \
>> +     tree-ssa-regpressure.o \
>>       tree-ssa-sccvn.o \
>>       tree-ssa-scopedtables.o \
>>       tree-ssa-sink.o \
>> diff --git a/gcc/tree-ssa-regpressure.c b/gcc/tree-ssa-regpressure.c
>> new file mode 100644
>> index 0000000..ebc6576
>> --- /dev/null
>> +++ b/gcc/tree-ssa-regpressure.c
>> @@ -0,0 +1,829 @@
>> +/* Reg Pressure Model and Live Range Shrinking Optimization on TREE SSA.
>> +   Copyright (C) 2017 Free Software Foundation, Inc.
>> +
>> +This file is part of GCC.
>> +
>> +GCC is free software; you can redistribute it and/or modify it
>> +under the terms of the GNU General Public License as published by the
>> +Free Software Foundation; either version 3, or (at your option) any
>> +later version.
>> +
>> +GCC is distributed in the hope that it will be useful, but WITHOUT
>> +ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> +FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
>> +for more details.
>> +
>> +You should have received a copy of the GNU General Public License
>> +along with GCC; see the file COPYING3.  If not see
>> +<http://www.gnu.org/licenses/>.  */
>> +
>> +
>> +#include "config.h"
>> +#include "system.h"
>> +#include "coretypes.h"
>> +#include "backend.h"
>> +#include "rtl.h"
>> +#include "memmodel.h"
>> +#include "ira.h"
> So I suspect what we need from ira.h is fairly narrow.  Would it be
> possible to pull the externalized interfaces we need for register
> pressure at the gimple level into a new include file?
>
> My worry is that as we pull in ira.h (and rtl.h and who knows what else)
> into the gimple space we end up with a tangled mess of touching rtl
> things in gimple which we'd like to avoid.
>
> In fact, the first thing that jumped out at me when I scanned the patch
> was the use of N_REG_CLASSES.  That's really a target property.
>
> I must have mis-read the earlier patch to ira.c as I thought we were
> only really tracking 3 classes.  Integer, float and vector.   Do we
> really need to track pressure on all the classes to do a reasonable job?
>
>
> +
>> +/* Reg_alloc structure, coalesced from ssa variables.  */
>> +typedef struct reg_alloc
>> +{
>> +  /* ID of this reg_alloc.  */
>> +  unsigned id;
>> +  /* The type of ssa variables of this reg_alloc.  */
>> +  tree type;
>> +  /* Ssa variables coalesced to this reg_alloc.  */
> Use "SSA" rather than "Ssa".
>
>
>
>
>
>> +      for (bsi = gsi_last_bb (bb); !gsi_end_p (bsi); gsi_prev (&bsi))
>> +     {
>> +       stmt = gsi_stmt (bsi);
>> +       stmt_info = create_stmt_lr_info (region, stmt);
>> +       /* No need to compute live range information for debug stmt.  */
>> +       if (is_gimple_debug (stmt))
>> +         continue;
> ISTM you'd want to test for a debug stmt before creating the stmt_lr_info?
>
>
>> +
>> +/* Check if PRESSURE is high according to target information about available
>> +   registers.  */
>> +static inline bool
>> +high_reg_pressure (unsigned *pressure)
>> +{
>> +  int k;
>> +
>> +  gcc_assert (pressure != NULL);
>> +
>> +  for (k = 0; k < N_REG_CLASSES; k++)
>> +    if (pressure[k] > 8 && pressure[k] > target_avail_regs[k])
>> +      return true;
> Where does the magic "8" come from?  ISTM that anytime the pressure is
> greater than the available registers that the pressure is high,
> regardless of the absolute number of live objects.
>
>
> Jeff
>