{PING] [PATCH] Sign extension elimination

[Roger Sayle]

On Fri, 14 Apr 2006, Mircea Namolaru wrote:
> Still not reviewed:
> http://gcc.gnu.org/ml/gcc-patches/2006-02/msg01994.html.

Sorry for the exceptionally long delay.  There was some discussion
on IRC that perhaps following RTH's initial comments/review, he
was somehow unhappy with the proposed approach or assuming the
role of patch reviewer.  My interpretation of his silence is
that he's no longer unhappy, and my background reading including
http://www.trl.ibm.com/projects/jit/paper/sxt.pdf leads me to
believe this approach is not unreasonable.  For example, I now
understand enough to be happy with the recog change which was
a previous concern of mine.

I'm not sure if you've been following the huge amount of off-list
traffic on extending this infrastructure to support x86 created
sign extensions.  Presumably your ping implies you'd prefer to
get the functionality into the tree and tweaked from there.

Fortunately, Mark has also commented that he's keen to get this
patch into 4.2, as it was submitted long before stage 3.


This patch is OK for mainline, with a few cosmetic changes:


> void
> see_main (void);

The function see_main should be made static to avoid the need for
this extra prototype.  This function isn't needed outside this file,
and is defined before it is used.


> #define ENTRY_EI(ENTRY) ((struct see_entry_extra_info *)(ENTRY)->extra_info)
A space between the cast and the (ENTRY).

> rtx set = NULL;
> ...
> set = single_set (extension);

The patch is particularly paranoid about initializing local variables
(perhaps an IBM Haifa coding style?).  There's no need to initialize
*every* local variable, and in cases such as the one above you can
combine with declaration with the immediately following assignment.
This will also reduce the number of lines using " rtx rhs1, rhs2;"
when appropriate.


> if ((GET_CODE (rhs) != SIGN_EXTEND) && (GET_CODE (rhs) != ZERO_EXTEND))
>    return NULL;

You don't need the extra parenthesis around the inequalities.
Several examples.

>  return (curr_prop1->regno == curr_prop2->regno);
Again unneccessary parenthesis.
Several examples.


>  if (r && REG_P (r))
>    return REGNO (r);
>  else
>    {
>      gcc_assert (r && INSN_P (r));
>      set = single_set (r);
>      gcc_assert (set);
>      lhs = SET_DEST (set);
>      return REGNO (lhs);
>    }

Marginally better style, is to avoid the else clause and continue
the remainder of the function in the outermost scope.  Though this
is admittedly a borderline nit-pick.

>     (*slot_pre_exp)->bitmap_index =
>	(htab_elements (see_pre_extension_hash) - 1);
Another example, of unnecessary parenthesis.

>  first_ei = (struct see_entry_extra_info *)first->extra_info;
>  second_ei = (struct see_entry_extra_info *)second->extra_info;
Space after the casts.


>    case RELEVANT_USE:
>      switch (second_ei->relevancy)
>	{
>	case RELEVANT_USE:
>	  return false;
>	case EXTENDED_DEF:
>	  first_ei->relevancy = second_ei->relevancy;
>	  first_ei->source_mode_signed = second_ei->source_mode_signed;
>	  first_ei->source_mode_unsigned = second_ei->source_mode_unsigned;
>	  return false;
>	case SIGN_EXTENDED_DEF:
>	case ZERO_EXTENDED_DEF:
>	  first_ei->relevancy = second_ei->relevancy;
>	  first_ei->source_mode = second_ei->source_mode;
>	  return false;
>	default:
>	  gcc_unreachable ();
>	}
>    case SIGN_EXTENDED_DEF:

Another real borderline case, and I'm sorry for nit-picking, but
my personal preference with code such as the above is to use
"break;" in the reachable bodies of this function, and place a
single "return false;" after the switch.  This makes it clearer
that control doesn't fall through into the SIGN_EXTENDED_DEF.

>   see_pre_extension_hash =
>     htab_create (10, hash_descriptor_pre_extension, eq_descriptor_pre_extension,
>		   hash_del_pre_extension);

Strange choice of where to break the source line.  More commonly the
htab_create is placed on te same line as the assignment, with the
arguments listed on separate lines, broken at the commas and indented
just past the open paren.

>	      if ((type == DEF_EXTENSION)
>		  && reg_mentioned_p (source_extension_reg, SET_DEST (sub)))
More unnecessary parenthesis arounf the initial equality.
A few more examples.

>     if (stn)
>	{
>	  switch (type)
>	    {
>	    ...
>	    }
>	}

No need for the braces around a single statement.


>  for (i = 0; i < uses_num; i++)
>    {
>      union_defs (df, DF_USES_GET (df, i), def_entry, use_entry,
>		  see_update_leader_extra_info);
>    }

Likewise.

>	case (SIGN_EXTEND):
>	case (ZERO_EXTEND):

Another novel use of unecessary parenthesis.  As the joke goes
"An A.I. programmer can write LISP in any language" :-)


Given these are all trivial style issues that don't affect the
patches correctness, feel free to patch the well tested version
that you have to mainline, and then commit the clean-ups as a
follow-up patch.  This'll make it easier to diagnose if you
accidentally break something fixing the style, vs. a more
fundamental technical problem that was previously missed.


Fingers crossed this doesn't break anything.  I promise to
review fallout patches much more quickly.  Sorry again for
the delay it's never perfectly clear who is responsible for
reviewing such large changes, and in cases like this where
thee contributor has more technical expertise over the subject
matter than the reviewer, a certain amount is based on trust.

Thanks again for you patience,

Roger
--