Thanks for looking at this.
Mikhail Maltsev <malts...@gmail.com> writes:
> 2. Not all of these "type promotions" can be done by just looking at
> function callers and callees (and some functions will only be generated
> during the build of some rare architecture) and checks already done in
> them. In a couple of cases I referred to comments and my general
> understanding of code semantics. In one case this actually caused a
> regression (in the patch it is fixed, of course), because of somewhat
> misleading comment (see "live_label_rtx" function added in patch for
> details) The question is - are such changes OK for refactoring (or it
> should strictly preserve semantics)?
FWIW I think the split between label_rtx and live_label_rtx is good,
but I think we should give them different names. The first one is
returning only a position in the instruction stream, the second is
returning a jump target. I think we should rename both of them to
make that distinction clearer.
> @@ -2099,9 +2107,9 @@ fix_crossing_conditional_branches (void)
> emit_label (new_label);
>
> gcc_assert (GET_CODE (old_label) == LABEL_REF);
> - old_label = JUMP_LABEL (old_jump);
> - new_jump = emit_jump_insn (gen_jump (old_label));
> - JUMP_LABEL (new_jump) = old_label;
> + old_label_insn = JUMP_LABEL_AS_INSN (old_jump);
> + new_jump = emit_jump_insn (gen_jump (old_label_insn));
> + JUMP_LABEL (new_jump) = old_label_insn;
>
> last_bb = EXIT_BLOCK_PTR_FOR_FN (cfun)->prev_bb;
> new_bb = create_basic_block (new_label, new_jump, last_bb);
I think the eventual aim would be to have rtx_jump_insn member functions
that get and set the jump label as an rtx_insn, with JUMP_LABEL_AS_INSN
being a stepping stone towards that. In cases like this it might make
more sense to ensure old_jump has the right type (rtx_jump_insn) and go
straight to the member functions, rather than switching to JUMP_LABEL_AS_INSN
now and then having to rewrite it later.
> @@ -1014,8 +1023,9 @@ do_compare_rtx_and_jump (rtx op0, rtx op1, enum
> rtx_code code, int unsignedp,
> {
> if (CONSTANT_P (tem))
> {
> - rtx label = (tem == const0_rtx || tem == CONST0_RTX (mode))
> - ? if_false_label : if_true_label;
> + rtx_code_label *label = (tem == const0_rtx
> + || tem == CONST0_RTX (mode)) ?
> + if_false_label : if_true_label;
> if (label)
> emit_jump (label);
> return;
Formatting nit, but the line break should be before "?" rather than after.
> diff --git a/gcc/is-a.h b/gcc/is-a.h
> index 58917eb..4fb9dde 100644
> --- a/gcc/is-a.h
> +++ b/gcc/is-a.h
> @@ -46,6 +46,11 @@ TYPE as_a <TYPE> (pointer)
>
> do_something_with (as_a <cgraph_node *> *ptr);
>
> +TYPE assert_as_a <TYPE> (pointer)
> +
> + Like as_a <TYPE> (pointer), but uses assertion, which is enabled even in
> + non-checking (release) build.
> +
> TYPE safe_as_a <TYPE> (pointer)
>
> Like as_a <TYPE> (pointer), but where pointer could be NULL. This
> @@ -193,6 +198,17 @@ as_a (U *p)
> return is_a_helper <T>::cast (p);
> }
>
> +/* Same as above, but checks the condition even in release build. */
> +
> +template <typename T, typename U>
> +inline T
> +assert_as_a (U *p)
> +{
> + gcc_assert (is_a <T> (p));
> + return is_a_helper <T>::cast (p);
> +}
> +
> +
> /* Similar to as_a<>, but where the pointer can be NULL, even if
> is_a_helper<T> doesn't check for NULL. */
This preserves the behaviour of the original code but I'm not sure
it's worth it. I doubt the distinction between:
gcc_assert (JUMP_P (x));
and:
gcc_checking_assert (JUMP_P (x));
was ever very scientific. Seems like we should take this refactoring as
an opportunity to make the checking more consistent.
> @@ -5069,18 +5164,15 @@ split_if_necessary (int regno, machine_mode mode,
> {
> bool res = false;
> int i, nregs = 1;
> - rtx next_usage_insns;
> + rtx_usage_list *next_usage_insns;
>
> if (regno < FIRST_PSEUDO_REGISTER)
> nregs = hard_regno_nregs[regno][mode];
> for (i = 0; i < nregs; i++)
> if (usage_insns[regno + i].check == curr_usage_insns_check
> - && (next_usage_insns = usage_insns[regno + i].insns) != NULL_RTX
> + && (next_usage_insns = usage_insns[regno + i].insns) != NULL
> /* To avoid processing the register twice or more. */
> - && ((GET_CODE (next_usage_insns) != INSN_LIST
> - && INSN_UID (next_usage_insns) < max_uid)
> - || (GET_CODE (next_usage_insns) == INSN_LIST
> - && (INSN_UID (XEXP (next_usage_insns, 0)) < max_uid)))
> + && (INSN_UID (next_usage_insns->insn ()) < max_uid)
> && need_for_split_p (potential_reload_hard_regs, regno + i)
> && split_reg (before_p, regno + i, insn, next_usage_insns))
> res = true;
No need for the brackets in the last condition now that it fits on
a single line.
> @@ -4501,6 +4500,107 @@ static int calls_num;
> USAGE_INSNS. */
> static int curr_usage_insns_check;
>
> +namespace
> +{
> +
> +class rtx_usage_list GTY(()) : public rtx_def
> +{
> +public:
> + /* This class represents an element in a singly-linked list, which:
> + 1. Ends with non-debug INSN
> + 2. May contain several INSN_LIST nodes with DEBUG_INSNs attached to them
> +
> + I.e.: INSN_LIST--> INSN_LIST-->..--> INSN
> + | |
> + DEBUG_INSN DEBUG_INSN
> +
> + See struct usage_insns for description of how it is used. */
> +
> + /* Get next node of the list. */
> + rtx_usage_list *next () const;
> +
> + /* Get the current instruction. */
> + rtx_insn *insn ();
> +
> + /* Check, if current INSN is debug info. */
> + bool debug_p () const;
> +
> + /* Add debug information to the chain. */
> + rtx_usage_list *push_front (rtx_debug_insn *debug_insn);
> +};
> +
> +/* If current node is an INSN return it. Otherwise it as an INSN_LIST node,
> + in this case return the attached INSN. */
> +
> +rtx_insn *
> +rtx_usage_list::insn ()
> +{
> + if (rtx_insn *as_insn = dyn_cast <rtx_insn *> (this))
> + return as_insn;
> + return safe_as_a <rtx_debug_insn *> (XEXP (this, 0));
> +}
> +
> +/* Get next node. */
> +
> +rtx_usage_list *
> +rtx_usage_list::next () const
> +{
> + return reinterpret_cast <rtx_usage_list *> (XEXP (this, 1));
> +}
> +
> +/* Check, if current INSN is debug info. */
> +
> +bool
> +rtx_usage_list::debug_p () const
> +{
> + return is_a <const rtx_insn_list *> (this);
> +}
> +
> +/* Add debug information to the chain. */
> +
> +rtx_usage_list *
> +rtx_usage_list::push_front (rtx_debug_insn *debug_insn)
> +{
> + /* ??? Maybe it would be better to store DEBUG_INSNs in a separate
> + homogeneous list (or vec) and use another pointer for actual INSN?
> + Then we won't have to traverse the list and some checks will also
> + become simpler. */
> + return reinterpret_cast <rtx_usage_list *>
> + (gen_rtx_INSN_LIST (VOIDmode,
> + debug_insn, this));
> +}
> +
> +} // anon namespace
> +
> +/* Helpers for as-a casts. */
> +
> +template <>
> +template <>
> +inline bool
> +is_a_helper <rtx_insn_list *>::test (rtx_usage_list *list)
> +{
> + return list->code == INSN_LIST;
> +}
> +
> +template <>
> +template <>
> +inline bool
> +is_a_helper <const rtx_insn_list *>::test (const rtx_usage_list *list)
> +{
> + return list->code == INSN_LIST;
> +}
> +
> +/* rtx_usage_list is either an INSN_LIST node or an INSN (no other
> + options). Therefore, this check is valid. */
> +
> +template <>
> +template <>
> +inline bool
> +is_a_helper <rtx_insn *>::test (rtx_usage_list *list)
> +{
> + return list->code != INSN_LIST;
> +}
> +
> /* Info about last usage of registers in EBB to do inheritance/split
> transformation. Inheritance transformation is done from a spilled
> pseudo and split transformations from a hard register or a pseudo
That seems pretty heavy-weight for LRA-local code. Also, the long-term
plan is for INSN_LIST and rtx_insn to be in separate hierarchies,
at which point we'd have no alias-safe way to distinguish them.
usage_insns isn't a GC structure and isn't live across a GC collection,
so I don't think we need these lists to be rtxes at all. Also:
/* Return first non-debug insn in list USAGE_INSNS. */
static rtx_insn *
skip_usage_debug_insns (rtx usage_insns)
{
rtx insn;
/* Skip debug insns. */
for (insn = usage_insns;
insn != NULL_RTX && GET_CODE (insn) == INSN_LIST;
insn = XEXP (insn, 1))
;
return safe_as_a <rtx_insn *> (insn);
}
suggests that having the nondebug insn last is a problem. Any
correctness decisions should be based on the nondebug insn and
it's inefficient to have to skip all the debug insns before
doing that.
So I think we should change the way this list is represented.
Maybe we could use something like a vec (perhaps too expensive to allocate,
reallocate and deallocate for each register) or a simple obstack-based
linked list. Either of those would be more space-efficient than
INSN_LIST and would avoid the rtx garbage after the pass has finished.
FWIW the patch looked good to me otherwise.
Thanks,
Richard
