Re: [PATCH, RFC]: Next stage1, refactoring: propagating rtx subclasses

[Richard Sandiford <rdsandiford at googlemail dot com>]

Thanks for looking at this.

Mikhail Maltsev <maltsevm@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