Re: Request for code review - (ZEE patch : Redundant Zero extension elimination)

[Sriraman Tallam <tmsriram at google dot com>]

I will make these changes. Thanks.

-Sriraman.

On Fri, Apr 30, 2010 at 1:42 PM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On Fri, Apr 30, 2010 at 11:06:58AM -0700, Sriraman Tallam wrote:
>> ? ? I am reopening this. I have attached the new ZEE patch generated
>> for GCC 4.6.0. Again, this patch is x86_64 specific now. I can modify
>> it to be a generic one if necessary. I would appreciate any feed-back
>> on how this can be taken forward.
>
> A few style comments on the patch; I do not have approval powers.
>
>> Index: config/i386/implicit-zee.c
>> ===================================================================
>> --- config/i386/implicit-zee.c ? ? ? ?(revision 0)
>> +++ config/i386/implicit-zee.c ? ? ? ?(revision 0)
>> +/* This says if a register is newly created for the purpose of
>> + ? zero-extension. */
>
> Two spaces after a period in comments, please. ?Here and many other
> places throughout the file.
>
>> +static int
>> +is_set_with_extension_DI (rtx *expr, void *data)
>> +{
>> + ?/* Looking only for patterns of the type :
>> + ? ? SET (REG:DI X) (ZERO_EXTEND (REG:SI x))
>> + ? */
>> +
>> + ?if (GET_CODE (*expr) == SET
>> + ? ? ?&& GET_MODE (SET_DEST (*expr)) == DImode
>> + ? ? ?&& GET_CODE (SET_DEST (*expr)) == REG)
>> + ? ?{
>> + ? ? ?if (GET_CODE (SET_SRC (*expr)) == ZERO_EXTEND
>> + ? ? ? ? ?&& GET_CODE (XEXP (SET_SRC (*expr),0)) == REG
>> + ? ? ? ? ?&& GET_MODE (XEXP (SET_SRC (*expr),0)) == SImode
>> + ? ? ? ? ?&& REGNO (SET_DEST (*expr)) == REGNO (XEXP (SET_SRC (*expr),0)))
>> + ? ? ? ? ? ?{
>
> Please merge these into a single condition.
>
>> +combine_set_zero_extend (rtx curr_insn, rtx *orig_set, rtx newreg)
>> +{
>> ...
>> + ? ? ? ? ?if (INTVAL (orig_src) >= 0 && INTVAL (orig_src) <= 0x7fffffff)
>> + ? ? ? ? ? ?{
>> + ? ? ? ? ? ? ?new_set = gen_rtx_SET (VOIDmode, newreg, orig_src);
>> + ? ? ? ? ? ?}
>
> GCC convention is to omit the braces when there's only a single
> statement. ?Please fix all other occurrences in the file.
>
>> +static int
>> +make_defs_and_copies_lists (rtx zero_extend_insn, rtx set_pat,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?VEC (rtx,heap) **defs_list,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ?VEC (rtx,heap) **copies_list)
>> +{
>> + ?bool *is_insn_visited;
>> + ?VEC (rtx,heap) *work_list;
>> + ?rtx srcreg, copy_reg_1, copy_reg_2;
>> + ?rtx def_insn;
>> + ?int n_defs = 0;
>> + ?int vec_index = 0;
>> + ?int n_worklist = 0;
>> + ?int i, is_copy;
>> +
>> + ?srcreg = XEXP (SET_SRC (set_pat), 0);
>> + ?work_list = VEC_alloc (rtx, heap, 8);
>> +
>> + ?is_insn_visited = XNEWVEC (bool, max_insn_uid);
>> +
>> + ?for (i = 0; i < max_insn_uid; i++)
>> + ? ?is_insn_visited[i] = false;
>
> It might be better to move this work to...
>
>> + ?/* Initialize the Work List */
>> + ?n_worklist = get_defs (zero_extend_insn, srcreg, &work_list);
>> +
>> + ?if (n_worklist == 0)
>> + ? ?{
>> + ? ? ?VEC_free (rtx, heap, work_list);
>> + ? ? ?XDELETEVEC (is_insn_visited);
>> + ? ? ?/* The number of defs being equal to zero can only imply that all of its
>> + ? ? ? ? definitions have been previously merged. */
>> + ? ? ?return 2;
>> + ? ?}
>
> ...here, now that you know is_insn_visited will be used.
>
>> + ? ? ? ? ?/* Perform transitive closure here */
>> +
>> + ? ? ? ? ?n_defs = 0;
>> + ? ? ? ? ?n_defs = get_defs (def_insn, copy_reg_1, &work_list);
>
> The first assignment is redundant.
>
>> + ? ? ? ? ?n_defs = 0;
>> + ? ? ? ? ?n_defs = get_defs (def_insn, copy_reg_2, &work_list);
>
> Likewise.
>
>> +merge_def_and_ze (rtx def_insn)
>> +{
>> ...
>> + ? ? ?if (!combine_set_zero_extend (def_insn, sub_rtx, setreg))
>> + ? ? ? ?{
>> + ? ? ? ? ?return false;
>> + ? ? ? ?}
>> + ? ? ?else
>> + ? ? ? ?{
>> + ? ? ? ? ?return true;
>> + ? ? ? ?}
>
> Just returning the result of combine_set_zero_extend would work here.
>
> -Nathan
>