This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH V5 05/11] bpf: new GCC port


jemarch@gnu.org (Jose E. Marchesi) writes:
> +/* Return true if an argument at the position indicated by CUM should
> +   be passed by reference.  If the hook returns true, a copy of that
> +   argument is made in memory and a pointer to the argument is passed
> +   instead of the argument itself.  */
> +
> +static bool
> +bpf_pass_by_reference (cumulative_args_t cum ATTRIBUTE_UNUSED,
> +		       const function_arg_info &arg)
> +{
> +  poly_int64 mode_size = GET_MODE_SIZE (arg.mode);
> +  unsigned num_bytes
> +    = (arg.type ? arg.type_size_in_bytes() : mode_size);

Sorry, I meant replace the whole expression with arg.type_size_in_bytes ():

static bool
bpf_pass_by_reference (cumulative_args_t cum ATTRIBUTE_UNUSED,
		       const function_arg_info &arg)
{
  unsigned num_bytes = arg.type_size_in_bytes ();

> +
> +  /* Pass aggregates and values bigger than 5 words by reference.
> +     Everything else is passed by copy.  */
> +  return ((arg.type && arg.aggregate_type_p ())
> +	  || (num_bytes > 8*5));

Here too:

  return (arg.aggregate_type_p ()
	  || (num_bytes > 8*5));

(although it now fits easily on one line).

> +/* Update the summarizer variable pointed by CA to advance past an
> +   argument in the argument list.  */
> +
> +static void
> +bpf_function_arg_advance (cumulative_args_t ca,
> +			  const function_arg_info &arg)
> +{
> +  CUMULATIVE_ARGS *cum = get_cumulative_args (ca);
> +  poly_int64 mode_size = GET_MODE_SIZE (arg.mode);
> +  unsigned num_bytes = (arg.type
> +			? arg.type_size_in_bytes () : mode_size);

Same here.

> +  unsigned num_words
> +    = CEIL (num_bytes, UNITS_PER_WORD);

Nit: line break seems unnecessary.

> +
> +  if (*cum <= 5 && *cum + num_words > 5)
> +    error ("too many function arguments for eBPF");
> +  else
> +    *cum += num_words;

For this to work as intended (i.e. only report an error once), I think
you need to do "*cum += num_words;" even when reporting the error.

> +/* Return the appropriate instruction to CALL to a function.  TARGET
> +   is a RTX denoting the address of the called function.

an RTX

> +	if (GET_CODE (op0) == REG
> +	    && (GET_CODE (op1) == LABEL_REF
> +		|| GET_CODE (op1) == SYMBOL_REF
> +		|| GET_CODE (op1) == CONST_INT
> +		|| GET_CODE (op1) == CONST))
> +	  {
> +	    fprintf (file, "[%s+", reg_names[REGNO (op0)]);
> +	    output_addr_const (file, op1);
> +	    fputs ("]", file);
> +	  }
> +	else
> +	  fatal_insn ("invalid address in operand", addr);

Do you support all those codes on the rhs of a PLUS?  I thought it was
just CONST_INT.

> +(define_predicate "reg_or_memory_operand"
> +  (ior (match_operand 0 "register_operand")
> +       (match_operand 0 "memory_operand")))
> +
> +(define_predicate "mov_dst_operand"
> +  (ior (match_operand 0 "register_operand")
> +       (match_operand 0 "memory_operand")))

These are both equivalent to the standard nonimmediate_operand.
It'd be better to use that instead if possible.

Looks good otherwise.

Thanks,
Richard


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]