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


    > +/* 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 ();

I found it weird, but didn't look at the implementation of the arg
methods... too bad I didnt :)
    
    > +
    > +  /* 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.
    
Right.

    > +/* 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.
   
It is just CONST_INT.  I removed a cpp constant and it had the
previously legal values.  Fixing.
 
    > +(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.

Ok.
    
    Looks good otherwise.
    
    Richard


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