On Thu, Nov 11, 2021 at 06:12:50PM +0000, Hafiz Abid Qadeer wrote:
> * dwarf2cfi.c (dw_stack_pointer_regnum): Change type to struct cfa_reg.
> (dw_frame_pointer_regnum): Likewise.
> (new_cfi_row): Use set_by_dwreg.
> (get_cfa_from_loc_descr): Use set_by_dwreg. Support register spans.
> handle DW_OP_bregx with DW_OP_breg{0-31}. Support DW_OP_lit*,
2 spaces instead of 1 before Support above.
> + cfa->reg.set_by_dwreg ((op == DW_OP_bregx)
> + ? (ptr->dw_loc_oprnd1.v.val_int) : (op - DW_OP_breg0));
Formatting. All those 3 () inner pairs are unnecessary, and it would be
nicer to use a temporary, like:
unsigned regno
= (op == DW_OP_bregx
? ptr->dw_loc_oprnd1.v.val_int : op - DW_OP_breg0);
cfa->reg.set_by_dwreg (regno);
> + unsigned int regno = (op == DW_OP_bregx)
> + ? (ptr->dw_loc_oprnd1.v.val_int) : (op - DW_OP_breg0);
With the ()s similarly, and also ? should be below op.
> + gcc_assert (regno == (cfa->reg.reg - 1));
Again, the inner () pair is unnecessary.
> + cfa->reg.span_width = (cfa->offset.to_constant () / 8);
And here the outer () pair.
> +
> +#if CHECKING_P
Please use if (CHECKING_P) instead of #if.
> + /* Ensure that the above assumption is accurate. */
> + for (unsigned int i = 0; i < result.span; i++)
> + {
> + gcc_assert (known_eq (GET_MODE_SIZE (GET_MODE (XVECEXP (span,
> + 0, i))),
> + result.span_width));
> + gcc_assert (REG_P (XVECEXP (span, 0, i)));
> + gcc_assert (dwf_regno (XVECEXP (span, 0, i)) == result.reg + i);
> + }
> +#endif
> + }
> +
> + return result;
> @@ -3135,7 +3267,8 @@ static unsigned int
> execute_dwarf2_frame (void)
> {
> /* Different HARD_FRAME_POINTER_REGNUM might coexist in the same file. */
> - dw_frame_pointer_regnum = DWARF_FRAME_REGNUM (HARD_FRAME_POINTER_REGNUM);
> + dw_frame_pointer_regnum = dwf_cfa_reg (gen_rtx_REG
> + (Pmode, HARD_FRAME_POINTER_REGNUM));
If at all possible, avoid function name on one line and ( with first
argument on another one. In the above case it can be easily avoided by
dw_frame_pointer_regnum
= dwf_cfa_reg (gen_rtx_REG (Pmode, HARD_FRAME_POINTER_REGNUM));
> +static void
> +build_breg_loc (struct dw_loc_descr_node **head, unsigned int regno)
> +{
> + if (regno <= 31)
> + add_loc_descr (head, new_loc_descr ((enum dwarf_location_atom)
> + (DW_OP_breg0 + regno), 0, 0));
Bad formatting, (DW_OP_breg0 should be below (enum
> +
> + /* deal with the remaining registers in the span. */
Capital letter on Deal
> + for (int i = (reg.span - 2); i >= 0; i--)
Please remove the redundant inner () pair.
> --- a/gcc/dwarf2out.h
> +++ b/gcc/dwarf2out.h
> @@ -119,6 +119,38 @@ struct GTY(()) dw_fde_node {
> };
>
>
> +/* This represents a register, in DWARF_FRAME_REGNUM space, for use in CFA
> + definitions and expressions.
> + Most architectures only need a single register number, but some (amdgcn)
> + have pointers that span multiple registers. DWARF permits arbitrary
> + register sets but existing use-cases only require contiguous register
> + sets, as represented here. */
> +struct GTY(()) cfa_reg {
> + unsigned int reg;
> + unsigned short span;
> + unsigned short span_width; /* A.K.A. register mode size. */
> +
> + cfa_reg& set_by_dwreg (unsigned int r)
> + {
> + reg = r;
> + span = 1;
> + span_width = 0; /* Unknown size (permitted when span == 1). */
> + return *this;
> + }
> +
> + bool operator== (const cfa_reg other) const
The normal C++ way would be (const cfa_reg &other), wouldn't it?
Or otherwise the const keyword doesn't make much sense.
> + {
> + return (reg == other.reg && span == other.span
> + && (span_width == other.span_width
> + || (span == 1
> + && (span_width == 0 || other.span_width == 0))));
> + }
> + bool operator!= (const cfa_reg other) const
Likewise.
Please add an empty line above operator!=
> + {
> + return !(*this == other);
> + }
> +};
> +
Otherwise LGTM.
Jakub
