Hi!
On Fri, Dec 20, 2019 at 06:55:53PM -0500, Michael Meissner wrote:
> * config/rs6000/rs6000.c (rs6000_reg_to_addr_mask): New helper
> function to identify the address mask of a hard register.
Do this as a separate patch please. That refactoring is pre-approved.
Please explain in the function comment what an "address mask" is. Or
better yet, don't call it a "mask", it isn't a mask?
Also various of the names here still have "reload" in it, which doesn't
really make much sense.
rs6000_mode_to_addressing_flags? And a reg_to for this new one?
Something like that.
> + /* For references to local static variables, try to fold a constant offset
> + into the address. */
> + else if (pcrel_local_address (addr, Pmode) && CONST_INT_P (element_offset))
> + {
> + if (GET_CODE (addr) == CONST)
> + addr = XEXP (addr, 0);
> +
> + if (GET_CODE (addr) == PLUS)
> + {
> + rtx op0 = XEXP (addr, 0);
> + rtx op1 = XEXP (addr, 1);
> + if (CONST_INT_P (op1))
> + {
> + HOST_WIDE_INT offset
> + = INTVAL (XEXP (addr, 1)) + INTVAL (element_offset);
> +
> + if (offset == 0)
> + new_addr = op0;
> +
> + else if (SIGNED_INTEGER_34BIT_P (offset))
> + {
> + rtx plus = gen_rtx_PLUS (Pmode, op0, GEN_INT (offset));
> + new_addr = gen_rtx_CONST (Pmode, plus);
> + }
> +
> + else
> + {
> + emit_move_insn (base_tmp, addr);
> + new_addr = gen_rtx_PLUS (Pmode, base_tmp, element_offset);
> + }
> + }
> + else
> + {
> + emit_move_insn (base_tmp, addr);
> + new_addr = gen_rtx_PLUS (Pmode, base_tmp, element_offset);
> + }
> + }
> +
> + else
> + {
> + rtx plus = gen_rtx_PLUS (Pmode, addr, element_offset);
> + new_addr = gen_rtx_CONST (Pmode, plus);
> + }
> + }
This adds four new if's and four new else's, indented three deep. Please
write it as a separate function? Something like "pcrel_adjust_address",
adding an extra offset?
Or not pcrel perhaps, you can do other addresses in the same routine?
Either way, adjust the address is useful more often than for extracts,
and is a much more general thing to do. So please split that out from
the existing code, as a separate patch again, and then add to that?
> + /* If we have a PC-relative address, check if offsetable loads are
> + allowed. */
> + else if (pcrel_local_address (new_addr, Pmode))
> + {
> + addr_mask_type addr_mask
> + = rs6000_reg_to_addr_mask (scalar_reg, scalar_mode);
> +
> + valid_addr_p = (addr_mask & RELOAD_REG_OFFSET) != 0;
> + }
That comment could be better, too? (And two letters "t" in offsettable).
Thanks,
Segher
