On 04/23/2015 09:10 PM, Thomas Preud'homme wrote:
From: Jeff Law [mailto:l...@redhat.com]
Sent: Friday, April 24, 2015 10:59 AM
Hi Jeff,
+
+static bool
+cprop_reg_p (const_rtx x)
+{
+ return REG_P (x) && !HARD_REGISTER_P (x);
+}
How about instead this move to a more visible location (perhaps a macro
in regs.h or an inline function). Then as a followup, change the
various places that have this sequence to use that common definition
that exist outside of cprop.c.
According to Steven this was proposed in the past but was refused (see
end of [1]).
[1] https://gcc.gnu.org/ml/gcc-patches/2015-03/msg01066.html
Missed that message. Given we've already gone round and round on it,
let go with the patch as-is and deal with hard_register_p and
pseudo_register_p independently. No idea who objected to that, seems
like a no-brainer to me.
@@ -1191,7 +1192,7 @@ do_local_cprop (rtx x, rtx_insn *insn)
/* Rule out USE instructions and ASM statements as we don't want
to
change the hard registers mentioned. */
if (REG_P (x)
- && (REGNO (x) >= FIRST_PSEUDO_REGISTER
+ && (cprop_reg_p (x)
|| (GET_CODE (PATTERN (insn)) != USE
&& asm_noperands (PATTERN (insn)) < 0)))
Isn't the REG_P test now redundant?
I made the same mistake when reviewing that change and indeed it's not.
Note the opening parenthesis before cprop_reg_p that contains a bitwise
OR expression. So in the case where cprop_reg_p is false, REG_P still
needs to be true.
We could keep a check on FIRST_PSEUDO_REGISTER but the intent (checking
that the register is suitable for propagation) is clearer now, as pointed out by
Steven to me.
Ah. Nevermind then.
So revised review is "ok for the trunk" :-)
jeff
