[Committed] [PATCH, loop2_invariant, 1/2] Check only one register class
On 26 June 2014 05:30, Jeff Law wrote:
> On 06/11/14 04:05, Zhenqiang Chen wrote:
>>
>> On 10 June 2014 19:06, Steven Bosscher wrote:
>>>
>>> On Tue, Jun 10, 2014 at 11:22 AM, Zhenqiang Chen wrote:
Hi,
For loop2-invariant pass, when flag_ira_loop_pressure is enabled,
function gain_for_invariant checks the pressures of all register
classes. This does not make sense since one invariant might impact
only one register class.
The patch enhances functions get_inv_cost and gain_for_invariant to
check only the register pressure of the invariant if possible.
>>>
>>>
>>> This patch may work for targets with more-or-less orthogonal reg
>>> classes, but not if there is a lot of overlap between reg classes.
>>
>>
>> Yes. I need check the overlap between reg classes.
>>
>> Patch is updated to check all overlap reg classes by
>> reg_classes_intersect_p:
>
> So you need a new ChangeLog, of course :-)
>
>
>
>>
>> diff --git a/gcc/loop-invariant.c b/gcc/loop-invariant.c
>> index c76a2a0..6e43b49 100644
>> --- a/gcc/loop-invariant.c
>> +++ b/gcc/loop-invariant.c
>> @@ -1092,16 +1092,22 @@ get_pressure_class_and_nregs (rtx insn, int
>> *nregs)
>> }
>>
>> /* Calculates cost and number of registers needed for moving invariant
>> INV
>> - out of the loop and stores them to *COST and *REGS_NEEDED. */
>> + out of the loop and stores them to *COST and *REGS_NEEDED. *CL will
>> be
>> + the REG_CLASS of INV. Return
>> + 0: if INV is invalid.
>> + 1: if INV and its depends_on have same reg_class
>> + > 1: if INV and its depends_on have different reg_classes. */
>
> Nit/bikeshedding. I tend to prefer < 0, 0, > 0 (or -1, 0, 1) for
> tri-states. It's not a big deal though.
Thanks! Update the tri-state values to ( -1, 0, 1).
>
>>check_p = i < ira_pressure_classes_num;
>> +
>> + if ((dep_ret > 1) || ((dep_ret == 1) && (*cl != dep_cl)))
>> + {
>> + *cl = ALL_REGS;
>> + ret ++;
>
> Whitespace nit -- no space in this statement. use "ret++;"
>
> You should add a testcase if at all possible. Perhaps two, one which runs
> on an ARM variant and one for x86_64. The former because that's obviously
> what Linaro cares about, the latter for wider testing.
>
>
> Definitely add a ChangeLog entry, fix the whitespace nit & add testcase. OK
> with those fixes. Your choice on the tri-state values.
Updated with a testcase for X86-64 and committed @212135.
ChangeLog:
2014-06-30 Zhenqiang Chen
* loop-invariant.c (get_inv_cost): Handle register class.
(gain_for_invariant): Check the register pressure of the inv
and its overlapped register class, other than all.
testsuite/ChangeLog:
2014-06-30 Zhenqiang Chen
* ira-loop-pressure.c: New test.
diff --git a/gcc/loop-invariant.c b/gcc/loop-invariant.c
index 25e63e4..ef5c59b 100644
--- a/gcc/loop-invariant.c
+++ b/gcc/loop-invariant.c
@@ -1090,16 +1090,22 @@ get_pressure_class_and_nregs (rtx insn, int *nregs)
}
/* Calculates cost and number of registers needed for moving invariant INV
- out of the loop and stores them to *COST and *REGS_NEEDED. */
+ out of the loop and stores them to *COST and *REGS_NEEDED. *CL will be
+ the REG_CLASS of INV. Return
+ -1: if INV is invalid.
+ 0: if INV and its depends_on have same reg_class
+ 1: if INV and its depends_on have different reg_classes. */
-static void
-get_inv_cost (struct invariant *inv, int *comp_cost, unsigned *regs_needed)
+static int
+get_inv_cost (struct invariant *inv, int *comp_cost, unsigned *regs_needed,
+ enum reg_class *cl)
{
int i, acomp_cost;
unsigned aregs_needed[N_REG_CLASSES];
unsigned depno;
struct invariant *dep;
bitmap_iterator bi;
+ int ret = 1;
/* Find the representative of the class of the equivalent invariants. */
inv = invariants[inv->eqto];
@@ -1115,7 +1121,7 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
if (inv->move
|| inv->stamp == actual_stamp)
-return;
+return -1;
inv->stamp = actual_stamp;
if (! flag_ira_loop_pressure)
@@ -1127,6 +1133,8 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
pressure_class = get_pressure_class_and_nregs (inv->insn, &nregs);
regs_needed[pressure_class] += nregs;
+ *cl = pressure_class;
+ ret = 0;
}
if (!inv->cheap_address
@@ -1167,6 +1175,8 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
EXECUTE_IF_SET_IN_BITMAP (inv->depends_on, 0, depno, bi)
{
bool check_p;
+ enum reg_class dep_cl = ALL_REGS;
+ int dep_ret;
dep = invariants[depno];
@@ -1174,7 +1184,7 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
if (dep->move)
continue;
- get_inv_cost (dep, &acomp_cost, aregs_needed);
+ dep_ret = get_inv_cost (dep, &acomp_cost, aregs_needed,
Re: [PATCH, loop2_invariant, 1/2] Check only one register class
On 06/11/14 04:05, Zhenqiang Chen wrote:
On 10 June 2014 19:06, Steven Bosscher wrote:
On Tue, Jun 10, 2014 at 11:22 AM, Zhenqiang Chen wrote:
Hi,
For loop2-invariant pass, when flag_ira_loop_pressure is enabled,
function gain_for_invariant checks the pressures of all register
classes. This does not make sense since one invariant might impact
only one register class.
The patch enhances functions get_inv_cost and gain_for_invariant to
check only the register pressure of the invariant if possible.
This patch may work for targets with more-or-less orthogonal reg
classes, but not if there is a lot of overlap between reg classes.
Yes. I need check the overlap between reg classes.
Patch is updated to check all overlap reg classes by reg_classes_intersect_p:
So you need a new ChangeLog, of course :-)
diff --git a/gcc/loop-invariant.c b/gcc/loop-invariant.c
index c76a2a0..6e43b49 100644
--- a/gcc/loop-invariant.c
+++ b/gcc/loop-invariant.c
@@ -1092,16 +1092,22 @@ get_pressure_class_and_nregs (rtx insn, int *nregs)
}
/* Calculates cost and number of registers needed for moving invariant INV
- out of the loop and stores them to *COST and *REGS_NEEDED. */
+ out of the loop and stores them to *COST and *REGS_NEEDED. *CL will be
+ the REG_CLASS of INV. Return
+ 0: if INV is invalid.
+ 1: if INV and its depends_on have same reg_class
+ > 1: if INV and its depends_on have different reg_classes. */
Nit/bikeshedding. I tend to prefer < 0, 0, > 0 (or -1, 0, 1) for
tri-states. It's not a big deal though.
check_p = i < ira_pressure_classes_num;
+
+ if ((dep_ret > 1) || ((dep_ret == 1) && (*cl != dep_cl)))
+ {
+ *cl = ALL_REGS;
+ ret ++;
Whitespace nit -- no space in this statement. use "ret++;"
You should add a testcase if at all possible. Perhaps two, one which
runs on an ARM variant and one for x86_64. The former because that's
obviously what Linaro cares about, the latter for wider testing.
Definitely add a ChangeLog entry, fix the whitespace nit & add testcase.
OK with those fixes. Your choice on the tri-state values.
jeff
Re: [PATCH, loop2_invariant, 1/2] Check only one register class
On 18 June 2014 05:49, Jeff Law wrote: > On 06/11/14 04:05, Zhenqiang Chen wrote: >> >> On 10 June 2014 19:06, Steven Bosscher wrote: >>> >>> On Tue, Jun 10, 2014 at 11:22 AM, Zhenqiang Chen wrote: Hi, For loop2-invariant pass, when flag_ira_loop_pressure is enabled, function gain_for_invariant checks the pressures of all register classes. This does not make sense since one invariant might impact only one register class. The patch enhances functions get_inv_cost and gain_for_invariant to check only the register pressure of the invariant if possible. >>> >>> >>> This patch may work for targets with more-or-less orthogonal reg >>> classes, but not if there is a lot of overlap between reg classes. >> >> >> Yes. I need check the overlap between reg classes. >> >> Patch is updated to check all overlap reg classes by >> reg_classes_intersect_p: > > Just so I'm sure I know what you're trying to do. > > You want to map the pseudo back to its likely class(es) then look at how > those classes (and only those classes) would be impacted from a register > pressure standpoint if the pseudo was hoisted as an invariant? Yes. > This is primarily achieved by returning the class of the invariant, then > filtering out any non-intersecting classes in gain_for_invariant, right? Yes. This is what I want to do since I found some invariant which register class is NO_REGS (memory write) or SSE_REGS is blocked by GENERAL_REGS' register pressure. Thanks! -Zhenqiang
Re: [PATCH, loop2_invariant, 1/2] Check only one register class
On 06/11/14 04:05, Zhenqiang Chen wrote: On 10 June 2014 19:06, Steven Bosscher wrote: On Tue, Jun 10, 2014 at 11:22 AM, Zhenqiang Chen wrote: Hi, For loop2-invariant pass, when flag_ira_loop_pressure is enabled, function gain_for_invariant checks the pressures of all register classes. This does not make sense since one invariant might impact only one register class. The patch enhances functions get_inv_cost and gain_for_invariant to check only the register pressure of the invariant if possible. This patch may work for targets with more-or-less orthogonal reg classes, but not if there is a lot of overlap between reg classes. Yes. I need check the overlap between reg classes. Patch is updated to check all overlap reg classes by reg_classes_intersect_p: Just so I'm sure I know what you're trying to do. You want to map the pseudo back to its likely class(es) then look at how those classes (and only those classes) would be impacted from a register pressure standpoint if the pseudo was hoisted as an invariant? This is primarily achieved by returning the class of the invariant, then filtering out any non-intersecting classes in gain_for_invariant, right? jeff
Re: [PATCH, loop2_invariant, 1/2] Check only one register class
On 10 June 2014 19:06, Steven Bosscher wrote:
> On Tue, Jun 10, 2014 at 11:22 AM, Zhenqiang Chen wrote:
>> Hi,
>>
>> For loop2-invariant pass, when flag_ira_loop_pressure is enabled,
>> function gain_for_invariant checks the pressures of all register
>> classes. This does not make sense since one invariant might impact
>> only one register class.
>>
>> The patch enhances functions get_inv_cost and gain_for_invariant to
>> check only the register pressure of the invariant if possible.
>
> This patch may work for targets with more-or-less orthogonal reg
> classes, but not if there is a lot of overlap between reg classes.
Yes. I need check the overlap between reg classes.
Patch is updated to check all overlap reg classes by reg_classes_intersect_p:
diff --git a/gcc/loop-invariant.c b/gcc/loop-invariant.c
index c76a2a0..6e43b49 100644
--- a/gcc/loop-invariant.c
+++ b/gcc/loop-invariant.c
@@ -1092,16 +1092,22 @@ get_pressure_class_and_nregs (rtx insn, int *nregs)
}
/* Calculates cost and number of registers needed for moving invariant INV
- out of the loop and stores them to *COST and *REGS_NEEDED. */
+ out of the loop and stores them to *COST and *REGS_NEEDED. *CL will be
+ the REG_CLASS of INV. Return
+ 0: if INV is invalid.
+ 1: if INV and its depends_on have same reg_class
+ > 1: if INV and its depends_on have different reg_classes. */
-static void
-get_inv_cost (struct invariant *inv, int *comp_cost, unsigned *regs_needed)
+static int
+get_inv_cost (struct invariant *inv, int *comp_cost, unsigned *regs_needed,
+ enum reg_class *cl)
{
int i, acomp_cost;
unsigned aregs_needed[N_REG_CLASSES];
unsigned depno;
struct invariant *dep;
bitmap_iterator bi;
+ int ret = 2;
/* Find the representative of the class of the equivalent invariants. */
inv = invariants[inv->eqto];
@@ -1117,7 +1123,7 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
if (inv->move
|| inv->stamp == actual_stamp)
-return;
+return 0;
inv->stamp = actual_stamp;
if (! flag_ira_loop_pressure)
@@ -1129,6 +1135,8 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
pressure_class = get_pressure_class_and_nregs (inv->insn, &nregs);
regs_needed[pressure_class] += nregs;
+ *cl = pressure_class;
+ ret = 1;
}
if (!inv->cheap_address
@@ -1169,6 +1177,8 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
EXECUTE_IF_SET_IN_BITMAP (inv->depends_on, 0, depno, bi)
{
bool check_p;
+ enum reg_class dep_cl = ALL_REGS;
+ int dep_ret;
dep = invariants[depno];
@@ -1176,7 +1186,7 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
if (dep->move)
continue;
- get_inv_cost (dep, &acomp_cost, aregs_needed);
+ dep_ret = get_inv_cost (dep, &acomp_cost, aregs_needed, &dep_cl);
if (! flag_ira_loop_pressure)
check_p = aregs_needed[0] != 0;
@@ -1186,6 +1196,12 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
if (aregs_needed[ira_pressure_classes[i]] != 0)
break;
check_p = i < ira_pressure_classes_num;
+
+ if ((dep_ret > 1) || ((dep_ret == 1) && (*cl != dep_cl)))
+ {
+ *cl = ALL_REGS;
+ ret ++;
+ }
}
if (check_p
/* We need to check always_executed, since if the original value of
@@ -1219,6 +1235,7 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
}
(*comp_cost) += acomp_cost;
}
+ return ret;
}
/* Calculates gain for eliminating invariant INV. REGS_USED is the number
@@ -1233,10 +1250,12 @@ gain_for_invariant (struct invariant *inv,
unsigned *regs_needed,
bool speed, bool call_p)
{
int comp_cost, size_cost;
+ enum reg_class cl;
+ int ret;
actual_stamp++;
- get_inv_cost (inv, &comp_cost, regs_needed);
+ ret = get_inv_cost (inv, &comp_cost, regs_needed, &cl);
if (! flag_ira_loop_pressure)
{
@@ -1245,6 +1264,11 @@ gain_for_invariant (struct invariant *inv,
unsigned *regs_needed,
- estimate_reg_pressure_cost (new_regs[0],
regs_used, speed, call_p));
}
+ else if (ret == 0)
+return -1;
+ else if ((ret == 1) && (cl == NO_REGS))
+/* Hoist it anyway since it does not impact register pressure. */
+return 1;
else
{
int i;
@@ -1253,6 +1277,10 @@ gain_for_invariant (struct invariant *inv,
unsigned *regs_needed,
for (i = 0; i < ira_pressure_classes_num; i++)
{
pressure_class = ira_pressure_classes[i];
+
+ if (!reg_classes_intersect_p (pressure_class, cl))
+ continue;
+
if ((int) new_regs[pressure_class]
+ (int) regs_needed[pressure_class]
+ LOOP_DATA (curr_loop)->max_reg_pres
Re: [PATCH, loop2_invariant, 1/2] Check only one register class
On Tue, Jun 10, 2014 at 11:22 AM, Zhenqiang Chen wrote: > Hi, > > For loop2-invariant pass, when flag_ira_loop_pressure is enabled, > function gain_for_invariant checks the pressures of all register > classes. This does not make sense since one invariant might impact > only one register class. > > The patch enhances functions get_inv_cost and gain_for_invariant to > check only the register pressure of the invariant if possible. This patch may work for targets with more-or-less orthogonal reg classes, but not if there is a lot of overlap between reg classes. So I don't think this approach is OK. Ciao! Steven
[PATCH, loop2_invariant, 1/2] Check only one register class
Hi,
For loop2-invariant pass, when flag_ira_loop_pressure is enabled,
function gain_for_invariant checks the pressures of all register
classes. This does not make sense since one invariant might impact
only one register class.
The patch enhances functions get_inv_cost and gain_for_invariant to
check only the register pressure of the invariant if possible.
Bootstrap and no make check regression on X86-64.
Bootstrap and no make check regression on X86-64 with
flag_ira_loop_pressure = true.
OK for trunk?
Thanks!
-Zhenqiang
ChangeLog:
2014-06-10 Zhenqiang Chen
* loop-invariant.c (get_inv_cost): Handle register class.
(gain_for_invariant): Check the register pressure of the inv,
other than all.
diff --git a/gcc/loop-invariant.c b/gcc/loop-invariant.c
index 100a2c1..e822bb6 100644
--- a/gcc/loop-invariant.c
+++ b/gcc/loop-invariant.c
@@ -1058,16 +1058,22 @@ get_pressure_class_and_nregs (rtx insn, int *nregs)
}
/* Calculates cost and number of registers needed for moving invariant INV
- out of the loop and stores them to *COST and *REGS_NEEDED. */
+ out of the loop and stores them to *COST and *REGS_NEEDED. *CL will be
+ the REG_CLASS of INV. Return
+ 0: if INV is invalid.
+ 1: if INV and its depends_on have same reg_class
+ > 1: if INV and its depends_on have different reg_classes. */
-static void
-get_inv_cost (struct invariant *inv, int *comp_cost, unsigned *regs_needed)
+static int
+get_inv_cost (struct invariant *inv, int *comp_cost, unsigned *regs_needed,
+ enum reg_class *cl)
{
int i, acomp_cost;
unsigned aregs_needed[N_REG_CLASSES];
unsigned depno;
struct invariant *dep;
bitmap_iterator bi;
+ int ret = 2;
/* Find the representative of the class of the equivalent invariants. */
inv = invariants[inv->eqto];
@@ -1083,7 +1089,7 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
if (inv->move
|| inv->stamp == actual_stamp)
-return;
+return 0;
inv->stamp = actual_stamp;
if (! flag_ira_loop_pressure)
@@ -1095,6 +1101,8 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
pressure_class = get_pressure_class_and_nregs (inv->insn, &nregs);
regs_needed[pressure_class] += nregs;
+ *cl = pressure_class;
+ ret = 1;
}
if (!inv->cheap_address
@@ -1135,10 +1143,12 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
EXECUTE_IF_SET_IN_BITMAP (inv->depends_on, 0, depno, bi)
{
bool check_p;
+ enum reg_class dep_cl = NO_REGS;
+ int dep_ret;
dep = invariants[depno];
- get_inv_cost (dep, &acomp_cost, aregs_needed);
+ dep_ret = get_inv_cost (dep, &acomp_cost, aregs_needed, &dep_cl);
if (! flag_ira_loop_pressure)
check_p = aregs_needed[0] != 0;
@@ -1148,6 +1158,11 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
if (aregs_needed[ira_pressure_classes[i]] != 0)
break;
check_p = i < ira_pressure_classes_num;
+
+ if (dep_ret > 1)
+ ret += dep_ret;
+ else if ((dep_ret == 1) && (*cl != dep_cl))
+ ret++;
}
if (check_p
/* We need to check always_executed, since if the original value of
@@ -1181,6 +1196,7 @@ get_inv_cost (struct invariant *inv, int
*comp_cost, unsigned *regs_needed)
}
(*comp_cost) += acomp_cost;
}
+ return ret;
}
/* Calculates gain for eliminating invariant INV. REGS_USED is the number
@@ -1195,10 +1211,12 @@ gain_for_invariant (struct invariant *inv,
unsigned *regs_needed,
bool speed, bool call_p)
{
int comp_cost, size_cost;
+ enum reg_class cl;
+ int ret;
actual_stamp++;
- get_inv_cost (inv, &comp_cost, regs_needed);
+ ret = get_inv_cost (inv, &comp_cost, regs_needed, &cl);
if (! flag_ira_loop_pressure)
{
@@ -1207,6 +1225,24 @@ gain_for_invariant (struct invariant *inv,
unsigned *regs_needed,
- estimate_reg_pressure_cost (new_regs[0],
regs_used, speed, call_p));
}
+ else if (ret == 0)
+return -1;
+ else if (ret == 1)
+{
+ /* Hoist it anyway since it does not impact register pressure. */
+ if (cl == NO_REGS)
+ return 1;
+
+ size_cost = (int) new_regs[cl]
++ (int) regs_needed[cl]
++ LOOP_DATA (curr_loop)->max_reg_pressure[cl]
++ IRA_LOOP_RESERVED_REGS
+- ira_class_hard_regs_num[cl];
+ if (size_cost > 0)
+ return -1;
+ else
+ size_cost = 0;
+}
else
{
int i;
