On Tue, Jun 27, 2017 at 6:40 PM, Jeff Law <l...@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 <davi...@google.com> >> Bin Cheng <bin.ch...@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 <binch...@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 >
