Hi,
When I was investigating unsigned int vec_init issue on Power,
I happened to find there seems something we can enhance in how
combine pass invalidate last_set (set last_set_invalid nonzero).
Currently we have the check:
if (!insn
|| (value && rsp->last_set_table_tick >= label_tick_ebb_start))
rsp->last_set_invalid = 1;
which means if we want to record some value for some reg and
this reg got refered before in a valid scope, we invalidate the
set of reg (last_set_invalid to 1). It avoids to find the wrong
set for one reg reference, such as the case like:
... op regX // this regX could find wrong last_set below
regX = ... // if we think this set is valid
... op regX
But because of retry's existence, the last_set_table_tick could
be set by some later reference insns, but we see it's set due
to retry on the set (for that reg) insn again, such as:
insn 1
insn 2
regX = ... --> (a)
... op regX --> (b)
insn 3
// assume all in the same BB.
Assuming we combine 1, 2 -> 3 sucessfully and replace them as two
(3 insns -> 2 insns), retrying from insn1 or insn2 again:
it will scan insn (a) again, the below condition holds for regX:
(value && rsp->last_set_table_tick >= label_tick_ebb_start)
it will mark this set as invalid set. But actually the
last_set_table_tick here is set by insn (b) before retrying, so it
should be safe to be taken as valid set.
This proposal is to check whether the last_set_table safely happens
after the current set, make the set still valid if so.
Bootstrapped/regtested on powerpc64le-linux-gnu (P9),
aarch64-linux-gnu and x86_64-pc-linux-gnu.
Full SPEC2017 building shows this patch gets more sucessful combines
from 1902208 to 1902243 (trivial though).
Any comments are highly appreciated!
BR,
Kewen
-----
gcc/ChangeLog:
* combine.c (struct reg_stat_type): New member
last_set_table_luid.
(update_table_tick): Add one argument for insn luid and
set last_set_table_luid with it.
(record_value_for_reg): Adjust the condition to set
last_set_invalid nonzero.
diff --git a/gcc/combine.c b/gcc/combine.c
index 6fb2fa82c3f..2f45a0ad733 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -202,6 +202,10 @@ struct reg_stat_type {
int last_set_table_tick;
+ /* Record the luid of the insn whose expression involving register n. */
+
+ int last_set_table_luid;
+
/* Record the value of label_tick when the value for register n is placed in
last_set_value. */
@@ -480,7 +484,7 @@ static rtx gen_lowpart_for_combine (machine_mode, rtx);
static enum rtx_code simplify_compare_const (enum rtx_code, machine_mode,
rtx, rtx *);
static enum rtx_code simplify_comparison (enum rtx_code, rtx *, rtx *);
-static void update_table_tick (rtx);
+static void update_table_tick (rtx, int);
static void record_value_for_reg (rtx, rtx_insn *, rtx);
static void check_promoted_subreg (rtx_insn *, rtx);
static void record_dead_and_set_regs_1 (rtx, const_rtx, void *);
@@ -13228,7 +13232,7 @@ count_rtxs (rtx x)
for each register mentioned. Similar to mention_regs in cse.c */
static void
-update_table_tick (rtx x)
+update_table_tick (rtx x, int insn_luid)
{
enum rtx_code code = GET_CODE (x);
const char *fmt = GET_RTX_FORMAT (code);
@@ -13243,7 +13247,21 @@ update_table_tick (rtx x)
for (r = regno; r < endregno; r++)
{
reg_stat_type *rsp = ®_stat[r];
- rsp->last_set_table_tick = label_tick;
+ if (rsp->last_set_table_tick >= label_tick_ebb_start)
+ {
+ /* Later references should not have lower ticks. */
+ gcc_assert (label_tick >= rsp->last_set_table_tick);
+ /* Should pick up the lowest luid if the references
+ are in the same block. */
+ if (label_tick == rsp->last_set_table_tick
+ && rsp->last_set_table_luid > insn_luid)
+ rsp->last_set_table_luid = insn_luid;
+ }
+ else
+ {
+ rsp->last_set_table_tick = label_tick;
+ rsp->last_set_table_luid = insn_luid;
+ }
}
return;
@@ -13279,16 +13297,17 @@ update_table_tick (rtx x)
if (ARITHMETIC_P (x0)
&& (x1 == XEXP (x0, 0) || x1 == XEXP (x0, 1)))
{
- update_table_tick (XEXP (x0, x1 == XEXP (x0, 0) ? 1 : 0));
+ update_table_tick (XEXP (x0, x1 == XEXP (x0, 0) ? 1 : 0),
+ insn_luid);
break;
}
}
- update_table_tick (XEXP (x, i));
+ update_table_tick (XEXP (x, i), insn_luid);
}
else if (fmt[i] == 'E')
for (j = 0; j < XVECLEN (x, i); j++)
- update_table_tick (XVECEXP (x, i, j));
+ update_table_tick (XVECEXP (x, i, j), insn_luid);
}
/* Record that REG is set to VALUE in insn INSN. If VALUE is zero, we
@@ -13359,7 +13378,10 @@ record_value_for_reg (rtx reg, rtx_insn *insn, rtx
value)
/* Mark registers that are being referenced in this value. */
if (value)
- update_table_tick (value);
+ {
+ gcc_assert (insn);
+ update_table_tick (value, DF_INSN_LUID (insn));
+ }
/* Now update the status of each register being set.
If someone is using this register in this block, set this register
@@ -13372,8 +13394,11 @@ record_value_for_reg (rtx reg, rtx_insn *insn, rtx
value)
{
rsp = ®_stat[i];
rsp->last_set_label = label_tick;
+ gcc_assert (label_tick >= rsp->last_set_table_tick);
if (!insn
- || (value && rsp->last_set_table_tick >= label_tick_ebb_start))
+ || (value && rsp->last_set_table_tick >= label_tick_ebb_start
+ && !(label_tick == rsp->last_set_table_tick
+ && DF_INSN_LUID (insn) < rsp->last_set_table_luid)))
rsp->last_set_invalid = 1;
else
rsp->last_set_invalid = 0;
