Mostly trivial formatting comments, but I think there's still a couple
of substantive points too.
jema...@gnu.org (Jose E. Marchesi) writes:
> +/* Return the builtin code corresponding to the kernel helper builtin
> + __builtin_NAME, or 0 if the name doesn't correspond to a kernel
> + helper builtin. */
> +
> +static inline int
> +bpf_helper_code (const char *name)
> +{
> + int i;
> +
> + for (i = 1; i < BPF_BUILTIN_HELPER_MAX; ++i)
> + {
> + if (strcmp (name, bpf_helper_names[i]) == 0)
> + return i;
> + }
Redundant braces, usual GCC style is to leave them out.
> +/* Return an RTX representing the place where a function returns or
> + receives a value of data type RET_TYPE, a tree node representing a
> + data type. */
> +
> +static rtx
> +bpf_function_value (const_tree ret_type,
> + const_tree fntype_or_decl ATTRIBUTE_UNUSED,
This *is* used. :-) I only noticed because...
> + bool outgoing ATTRIBUTE_UNUSED)
> +{
> + enum machine_mode mode;
> + int unsignedp;
> +
> + mode = TYPE_MODE (ret_type);
> + if (INTEGRAL_TYPE_P (ret_type))
> + mode = promote_function_mode (ret_type, mode, &unsignedp,
> fntype_or_decl, 1);
...long line.
> +/* Return the initial difference between the specified pair of
> + registers. The registers that can figure in FROM, and TO, are
> + specified by ELIMINABLE_REGS in bpf.h.
> +
> + This function is used in the definition of
> + INITIAL_ELIMINATION_OFFSET in bpf.h */
> +
> +HOST_WIDE_INT
> +bpf_initial_elimination_offset (int from,
> + int to)
Odd line split.
> +{
> + HOST_WIDE_INT ret;
> +
> + if (from == ARG_POINTER_REGNUM && to == STACK_POINTER_REGNUM)
> + {
> + ret = (cfun->machine->local_vars_size
> + + cfun->machine->callee_saved_reg_size);
> + }
Redundant braces.
> +/* Return true if X (a RTX) is a legitimate memory address on the
> + target machine for a memory operand of mode MODE. */
> +
> +static bool
> +bpf_legitimate_address_p (machine_mode mode ATTRIBUTE_UNUSED,
> + rtx x,
> + bool strict)
> +{
> + switch (GET_CODE (x))
> + {
> + case LABEL_REF:
> + case SYMBOL_REF:
> + case CONST:
> + /* These are assumed to fit in 32-bit, because the kernel
> + imposes a limit to the size of eBPF programs. */
> + return true;
> + break;
Usual style is not to break after a return. Same for rest of file.
> +/* 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)
> +{
> + unsigned num_bytes
> + = (arg.type
> + ? int_size_in_bytes (arg.type) : GET_MODE_SIZE (arg.mode));
arg.type_size_in_bytes ()
> +
> + /* Pass aggregates and values bigger than 5 words by reference.
> + Everything else is passed by copy. */
> + return ((arg.type && AGGREGATE_TYPE_P (arg.type))
arg.aggregate_type_p ()
> + || (num_bytes > 8*5));
> +}
> +
> +#undef TARGET_PASS_BY_REFERENCE
> +#define TARGET_PASS_BY_REFERENCE bpf_pass_by_reference
> +
> +/* Return a RTX indicating whether a function argument is passed in a
> + register and if so, which register. */
> +
> +static rtx
> +bpf_function_arg (cumulative_args_t ca, const function_arg_info &arg)
> +{
> + CUMULATIVE_ARGS *cum = get_cumulative_args (ca);
> +
> + if (*cum < 5)
> + return gen_rtx_REG (arg.mode, *cum + 1);
> + else
> + /* An error will be emitted for this in
> + bpf_function_arg_advance. */
> + return NULL_RTX;
> +}
> +
> +#undef TARGET_FUNCTION_ARG
> +#define TARGET_FUNCTION_ARG bpf_function_arg
> +
> +/* 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);
> +
> + if (*cum > 4)
> + error ("too many function arguments for eBPF");
> + else
> + {
> + unsigned num_bytes
> + = (arg.type
> + ? int_size_in_bytes (arg.type) : GET_MODE_SIZE (arg.mode));
arg.type_size_in_bytes ()
> + unsigned num_words
> + = CEIL (num_bytes, UNITS_PER_WORD);
> +
> + *cum += num_words;
> + }
I think my previous comment still stands here. *cum > 4 won't raise
an error for 4 DI arguments followed by a TI argument, which requires
6 registers in total. It'd also be good to avoid repeating the error
message within a single argument list.
Something like:
*cum <= 5 && *cum + num_words > 5
might be better, so that you only report an error on the argument that
tries to include r6.
> +/* Output the assembly code for a constructor. Since eBPF doesn't
> + support indirect calls, constructors are not supported. */
> +
> +static void
> +bpf_output_constructor (rtx symbol, int priority ATTRIBUTE_UNUSED)
> +{
> + tree decl = SYMBOL_REF_DECL (symbol);
> +
> + if (decl)
> + error_at (DECL_SOURCE_LOCATION (decl),
> + "no support for constructors");
Think sorry_at would be better, since this is an unsupported feature
rather than a user error. The message would need to be slightly reworded
to avoid the repeated "support".
> + else
> + fatal_insn ("no support for constructors", symbol);
fatal_insn isn't right for symbols -- should be a sorry ().
> +/* Output the assembly code for a destructor. Since eBPF doesn't
> + support indirect calls, destructors are not supported. */
> +
> +static void
> +bpf_output_destructor (rtx symbol, int priority ATTRIBUTE_UNUSED)
> +{
> + tree decl = SYMBOL_REF_DECL (symbol);
> +
> + if (decl)
> + error_at (DECL_SOURCE_LOCATION (decl),
> + "no support for destructors");
> + else
> + fatal_insn ("no support for destructors", symbol);
Same for these two.
> +/* Return the appropriate instruction to CALL to a function. TARGET
> + is a `mem' RTX denoting the address of the called function.
> +
> + The main purposes of this function are:
> + - To reject indirect CALL instructions, which are not supported by
> + eBPF.
> + - To recognize calls to kernel helper functions and emit the
> + corresponding CALL N instruction.
> +
> + This function is called from the expansion of the 'call' pattern in
> + bpf.md. */
> +
> +const char *
> +bpf_output_call (rtx target)
> +{
> + rtx op, xops[1];
> +
> + op = XEXP (target, 0);
> + switch (GET_CODE (op))
> + {
> + case CONST_INT:
> + output_asm_insn ("call\t%0", &op);
> + break;
> + case SYMBOL_REF:
> + {
> + const char *function_name = XSTR (op, 0);
> + int code;
> +
> + if (strncmp (function_name, "__builtin_bpf_helper_", 21) == 0
> + && ((code = bpf_helper_code (function_name + 21)) != 0))
> + {
> + xops[0] = GEN_INT (code);
> + output_asm_insn ("call\t%0", xops);
> + }
> + else
> + output_asm_insn ("call\t%0", &op);
Not a problem, but it seems odd to use xops for one call and &op for
the other two.
> +/* An initializer containing the contents of the register classes, as
> + integers which are bit masks. The Nth integer specifies the
> + contents of class N. The way the integer MASK is interpreted is
> + that register R is in the class if `MASK & (1 << R)' is 1.
> +
> + In eBPF all the hard registers are considered general-purpose
> + integer registers. */
> +#define REG_CLASS_CONTENTS \
> +{ \
> + 0x00000000, /* NO_REGS */ \
> + 0x00000fff, /* GR_REGS */ \
> + 0x00000fff, /* ALL_REGS */ \
> +}
Now that the two classes are unconditionally the same, it'd be better to
drop the GR_REGS class and just use GENERAL_REGS (#defined to ALL_REGS)
instead.
> +;; Note that subtractions of constants become additions, so there is
> +;; no need to handle immediate operands in the subMODE3 insns.
> +
> +(define_insn "sub<AM:mode>3"
> + [(set (match_operand:AM 0 "register_operand" "=r")
> + (plus:AM (match_operand:AM 1 "register_operand" " 0")
> + (match_operand:AM 2 "register_operand" " r")))]
Should be "minus" rather than "plus".
> + "1"
Just "" is more usual.
> + "sub<msuffix>\t%0,%2"
> + [(set_attr "type" "<mtype>")])
> +
> +;;; Negation
> +(define_insn "neg<AM:mode>2"
> + [(set (match_operand:AM 0 "register_operand" "=r")
> + (neg:AM (match_operand:AM 1 "register_operand" " 0")))]
> + ""
> + "neg<msuffix>\t%0"
> + [(set_attr "type" "<mtype>")])
> +
> +;;; Multiplication
> +(define_insn "mul<AM:mode>3"
> + [(set (match_operand:AM 0 "register_operand" "=r,r")
> + (mult:AM (match_operand:AM 1 "register_operand" " 0,0")
> + (match_operand:AM 2 "reg_or_imm_operand" " r,I")))]
> + ""
> + "mul<msuffix>\t%0,%2"
> + [(set_attr "type" "<mtype>")])
> +
> +(define_insn "mulsidi3"
> + [(set (match_operand:DI 0 "register_operand" "=r,r")
> + (sign_extend:DI
> + (mult:SI (match_operand:SI 1 "register_operand" "0,0")
> + (match_operand:SI 2 "reg_or_imm_operand" "r,I"))))]
> + ""
> + "mul32\t%0,%2"
> + [(set_attr "type" "alu32")])
Sorry, Segher was right and I was wrong: mulsidi3 is instead:
[(set (match_operand:DI 0 "register_operand" "=r,r")
(mult:DI
(sign_extend:DI (match_operand:SI 1 "register_operand" "0,0"))
(sign_extend:DI (match_operand:SI 2 "reg_or_imm_operand" "r,I"))))]
i.e. extend the operands rather than the result. So the define_insn
shouldn't be called mulsidi3 after all.
Are you sure this is a sign extension though? From a quick look at the
kernel sources, I got the impression it was a zero extension instead.
> +(define_expand "zero_extendsidi2"
> + [(set (match_operand:DI 0 "register_operand")
> + (zero_extend:DI (match_operand:SI 1 "reg_or_indirect_memory_operand")))]
> + ""
> +{
> + if (register_operand (operands[1], SImode))
> + {
> + operands[1] = gen_lowpart (DImode, operands[1]);
> + emit_insn (gen_ashldi3 (operands[0], operands[1], GEN_INT (32)));
> + emit_insn (gen_lshrdi3 (operands[0], operands[0], GEN_INT (32)));
> + DONE;
> + }
> +})
> +
> +(define_insn "*zero_extendsidi2"
> + [(set (match_operand:DI 0 "register_operand" "=r,r")
> + (zero_extend:DI (match_operand:SI 1 "reg_or_indirect_memory_operand"
> "0,m")))]
> + ""
> + "@
> + lsh\t%0,32\n\trsh\t%0,32
> + ldxw\t%0,%1"
> + [(set_attr "type" "alu,ldx")
> + (set_attr "length" "16,8")])
Sorry, should have noticed last time, but: you shouldn't need to handle
register operands here given the expander above. It's OK if you find it
improves code quality, but it'd be interesting to know why if so....
> +
> +;;; Sign-extension
> +
> +;; Sign-extending a 32-bit value into a 64-bit value is achieved using
> +;; shifting, with instructions generated by the expand below.
> +
> +(define_expand "extendsidi2"
> + [(set (match_operand:DI 0 "register_operand")
> + (sign_extend:DI (match_operand:SI 1 "register_operand")))]
> + ""
> +{
> + operands[1] = gen_lowpart (DImode, operands[1]);
> + emit_insn (gen_ashldi3 (operands[0], operands[1], GEN_INT (32)));
> + emit_insn (gen_ashrdi3 (operands[0], operands[0], GEN_INT (32)));
> + DONE;
> +})
> +
> +(define_insn "*extendsidi2"
> + [(set (match_operand:DI 0 "register_operand" "=r")
> + (sign_extend:DI (match_operand:SI 1 "register_operand" "0")))]
> + ""
> + "lsh\t%0,32\n\tarsh\t%0,32"
> + [(set_attr "type" "alu")
> + (set_attr "length" "16")])
Likewise this define_insn shouldn't be needed.
> + /* In cases where the moved entity is a constant address, we need to
> + emit an extra mov and modify the second operand to obtain
> + something like:
> +
> + lddw %T, %1
> + ldxw %0, [%T+0]
> +
> + Ditto for stores. */
> +
> + if (MEM_P (operands[1])
> + && CONSTANT_ADDRESS_P (XEXP (operands[1], 0)))
> + {
> + rtx tmp = gen_reg_rtx (DImode);
> +
> + emit_move_insn (tmp, XEXP (operands[1], 0));
> + operands[1] = gen_rtx_MEM (<AMM:MODE>mode, tmp);
> + }
> +
> + if (MEM_P (operands[0])
> + && CONSTANT_ADDRESS_P (XEXP (operands[0], 0)))
> + {
> + rtx tmp = gen_reg_rtx (DImode);
> +
> + emit_move_insn (tmp, XEXP (operands[0], 0));
> + operands[0] = gen_rtx_MEM (<AMM:MODE>mode, tmp);
> + }
In the covering note you said:
. Remove dubious code from the move expanders, by not accepting constant
addresses as legit. Rework handling of call instructions accordingly.
but it looks like they're still legitimate here and in
bpf_legitimate_address_p.
Not making them legitimate should also let you get rid of
indirect_memory_operand.
> +;; The eBPF jump instructions use 64-bit arithmetic when evaluating
> +;; the jump conditions. Therefore we use DI modes below.
> +
> +(define_expand "cbranchdi4"
> + [(set (pc)
> + (if_then_else (match_operator 0 "comparison_operator"
> + [(match_operand:DI 1 "register_operand")
> + (match_operand:DI 2 "reg_or_imm_operand")])
> + (label_ref (match_operand 3 "" ""))
> + (pc)))]
> + ""
> +{
> + if (!ordered_comparison_operator (operands[0], VOIDmode))
> + FAIL;
Indented too far.
Thanks,
Richard
