Re: [tuples] Tuples accessors patch

[Aldy Hernandez <aldyh at redhat dot com>]

Hi Chris.

BTW, your mailer is wrapping parts of the patch weirdly when you quote.
Would it be possible to fix this?

I know Diego already approved this, but would you mind correcting these?

> -  GS_RETURN_OPERAND_RETVAL (p) = retval;
> +  gs_return_set_retval(p, retval);

Space after function name.

> +  p = ggc_alloc_cleared (sizeof (struct gimple_statement_call)
> +                         + sizeof (tree) * (nargs - 1));

Thanks for fixing my parenthesis thinko.

> +  GS_CODE (p) = GS_CALL;
> +  GS_SUBCODE_FLAGS (p) = 0;
> +  gs_call_set_nargs (p, nargs);
> +  gs_call_set_fn (p, func);

Diego, I don't mind all the gs_*_set_* inline functions, I assume it was
your idea ;-), but should we not also provide  gs_set_code() and
gs_set_subcode_flags() as well?  I don't know; what do you think?

>     case GSS_ASSIGN_UNARY_REG:
>       p = ggc_alloc_cleared (sizeof (struct
> gimple_statement_assign_unary_reg));
>       GS_CODE (p) = GS_ASSIGN;
> -      GS_ASSIGN_UNARY_REG_LHS (p) = lhs;
> +      gs_assign_set_lhs (p, lhs);
>       if (IS_EXPR_CODE_CLASS (TREE_CODE_CLASS (TREE_CODE (rhs))))
> -	GS_ASSIGN_UNARY_REG_RHS (p) = TREE_OPERAND (rhs, 0);
> +	gs_assign_set_rhs (p, TREE_OPERAND (rhs, 0));
>       else
> -	GS_ASSIGN_UNARY_REG_RHS (p) = rhs;
> +	gs_assign_set_rhs (p, rhs);
>       GS_SUBCODE_FLAGS (p) = TREE_CODE (rhs);
>       break;

The function gs_assign_set_operand() looks at GS_SUBCODE_FLAGS to
determine where to put operands.  In the above code you are calling
gs_assign_set* before you set GS_SUBCODE_FLAGS, which has been cleared
by ggc_alloc_cleared.

>     case GSS_ASSIGN_UNARY_MEM:
> @@ -82,11 +115,11 @@
>     case GSS_ASSIGN_UNARY_MEM:
>       p = ggc_alloc_cleared (sizeof (struct
> gimple_statement_assign_unary_mem));
>       GS_CODE (p) = GS_ASSIGN;
> -      GS_ASSIGN_UNARY_MEM_LHS (p) = lhs;
> +      gs_assign_set_lhs (p, lhs);
>       if (IS_EXPR_CODE_CLASS (TREE_CODE_CLASS (TREE_CODE (rhs))))
> -	GS_ASSIGN_UNARY_MEM_RHS (p) = TREE_OPERAND (rhs, 0);
> +        gs_assign_set_rhs (p, TREE_OPERAND (rhs, 0));
>       else
> -	GS_ASSIGN_UNARY_MEM_RHS (p) = rhs;
> +        gs_assign_set_rhs (p, rhs);
> 
>       GS_SUBCODE_FLAGS (p) = TREE_CODE (rhs);

Similarly here.

> +/* Construct a GS_COND statement.  Compare using PRED the LHS and the RHS,
> +   if true goto T_LABEL, else goto F_LABEL.  */

"If true..." should be a separate sentence.

Perhaps we should describe the arguments better.  Maybe like this:

/* Construct a GS_COND statement.
   PRED is the condition.
   T_LABEL is the label to jump to if the condition is true.
   F_LABEL is the label to jump to otherwise.  */

LHS and RHS are obvious.

> +/* Construct a GS_ASM statement.
> +
> +   STRING is the assembly code.
> +   NI is the number of register inputs.
> +   NO is the number of register outputs.
> +   NC is the number of clobbered registers.
> +   ...s are trees for each input, output and clobber.  */

s/s are/ are/

> +
> +gimple
> +gs_build_asm (const char* string, unsigned ni, unsigned no, unsigned 
> nc,...)
> +{
> +  gimple p;
> +  unsigned i;
> +  va_list ap;
> +
> +  p = ggc_alloc_cleared (sizeof (struct gimple_statement_asm)
> +                         + sizeof (tree) * (ni + no + nc) - 1);

This should be "(ni + no + nc - 1)".

> +  gs_asm_set_ni (p, ni);
> +  gs_asm_set_no (p, no);
> +  gs_asm_set_nc (p, nc);

These names are too cryptic.  Could you change them to:

	gs_asm_set_ninputs
	gs_asm_set_noutputs
	gs_asm_set_nclobbered

> +  gs_asm_set_string (p, string);
> +
> +  va_start (ap, nc);
> +  for (i = 0; i < ni; i++)
> +    gs_asm_set_in_op (p, i, va_arg (ap, tree));
> +  for (i = 0; i < no; i++)
> +    gs_asm_set_out_op (p, i, va_arg (ap, tree));
> +  for (i = 0; i < nc; i++)
> +    gs_asm_set_clobber_op (p, i, va_arg (ap, tree));

For consistency, can we call these:

	gs_asm_set_input_op
	gs_asm_set_output_op

> +/* Construct a GS_EH_FILTER statement.
> +
> +  TYPES is the filter's types.

s/is/are

> +  FAILURE is the filters failure action.  */

s/filters/filter's

> +   CATCH, does this try catch?
> +   FINALLY, does this try have a finally?  */

To make these obvious, call these  CATCH_P and FINALLY_P.

> +  if(catch)

Space after if.

> +  if(finally)

Same.

> +/* Construct a GS_PHI statement, with a CAPACITY, a RESULT, and NARGS
> +   phi_arg_d * arguments from ...  */

Can you describe what the arguments are, not just with their names?  If
you don't know, just leave them out of the description.

> +gimple
> +gs_build_phi (unsigned capacity, unsigned nargs, tree result, ...)
> +{
> +  gimple p;
> +  unsigned int i;
> +  va_list va;
> +  p = ggc_alloc_cleared (sizeof (struct gimple_statement_phi)
> +      + (sizeof(struct phi_arg_d) * (nargs - 1)) );

Space after sizeof.

> +      struct phi_arg_d * phid =

Remove space after "*"

> +/* Construct a GS_RESX statement, with a REGION.  */

Describe region.

> +/* Construct a GS_SWITCH statement: for INDEX with the NLABLES labels
> +   in ..., excluding the DEFAULT_LABEL.  */

Describe arguments like this:

/* Construct a GS_SWITCH statement.
   NLABELS is the number of labels.
   INDEX is the switch's index.
   DEFAULT_LABEL is the default label.
   The rest of the arguments are the labels.  */

Please describe arguments for the rest of the functions, where
appropriate.

> +/* Construct a GS_OMP_PARRELLE statement for BODY, with CLAUSES, a child

Should be GS_OMP_PARALLEL.

> +#define GS_TRY_CATCH 1 << 0
> +#define GS_TRY_FINALLY 1 << 1

Add a comment to say these apply to GS_SUBCODE_FLAGS.

> -  tree default_label;
>   tree GTY ((length ("%h.nlabels + 1"))) labels[1];

Why did we decide to put default_label in the op[] array?  Don't we
always have a default label?

> +/* Predicate for conds. */
> +enum gs_cond {
> +  COND_LT, COND_GT, COND_LE, COND_GE, COND_EQ, COND_NE

Prepend GS_ to these conditions to avoid confusion with other things in
the compiler.

> +static inline void
> +gs_assign_set_operand (gimple gs, int opno, tree op)

This functions needs a comment describing its purpose and its arguments.

> +gs_assign_set_lhs (gimple gs, tree lhs)

These not as much, cause they're more obvious.

> +static inline tree *
> +gs_call_fn (gimple gs)
> +{
> +  GS_CHECK (gs, GS_CALL);
> +  return &(gs->gs_call.fn);
> +}

In most of the accessors returning trees you have them returning
pointers to trees.  I assume this is because you followed my code that
did something similar, but then provided a GS_CALL_FN macro doing the
pointer magic so you could set to GS_CALL_FN.  Since you are now
providing gs_*_set_*() functions, there is no need for the macros, and
no need to return the address of these operands.

Please adjust your patch throughout.

> +static inline void
> +gs_bind_set_body (gimple gs, struct gs_sequence * gss)

gs_sequence pointers are to be specified as "gs_seq".

> +  gs->gs_bind.body.first = gss->first;
> +  gs->gs_bind.body.last = gss->last;

Use GS_SEQ_FIRST and GS_SEQ_LAST.

> +static inline tree *
> +gs_asm_in_op (gimple gs, unsigned int index)
> +{
> +  GS_CHECK (gs, GS_ASM);
> +  gcc_assert(index <= gs->gs_asm.ni);

Space after "(".

> +static inline void
> +gs_asm_set_in_op (gimple gs, unsigned int index, tree in_op)
> +{
> +  GS_CHECK (gs, GS_ASM);
> +  gcc_assert(index <= gs->gs_asm.ni);

Same.

> +  gs->gs_asm.op[index] = in_op;
> +}
> +
> +static inline tree *
> +gs_asm_out_op (gimple gs, unsigned int index)
> +{
> +  GS_CHECK (gs, GS_ASM);
> +  gcc_assert(index <= gs->gs_asm.no);

Same.

> +/* GS_OMP_CRITICAL accessors. */

Comment out of place.  Should be before the gs_omp_critical* functions.

> +static inline void
> +gs_omp_set_body (gimple gs, struct gs_sequence body)
> +{
> +  gs->omp.body.first = body.first;
> +  gs->omp.body.last = body.last;
> +}

Use GS_SEQ_{FIRST,LAST} here.  Something like

	GS_SEQ_FIRST (&gs->omp.body) = GS_SEQ_FIRST (&body);
	etc

> +static inline void
> +gs_omp_for_set_pre_body (gimple gs, struct gs_sequence pre_body)
> +{
> +  GS_CHECK (gs, GS_OMP_FOR);
> +  gs->gs_omp_for.pre_body.first = pre_body.first;
> +  gs->gs_omp_for.pre_body.last = pre_body.last;

Use GS_SEQ_{FIRST,LAST}.

> +/* get or set the OMP_FOR_COND stored in the subcode flags */

Uppercase.

> +static inline void
> +gs_assign_omp_for_cond (gimple gs, enum gs_cond cond)
> +{
> +  GS_CHECK(gs, GS_OMP_FOR);
> +  GS_SUBCODE_FLAGS(gs) =  cond;

Space after GS_SUBCODE_FLAGS.

> +/* Get the string representation of a gs_cond P.  */

s/ P//

Chris, could you make these changes, and test them, then resubmit the
patch?

Thanks.
Aldy