This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [ARM] Fix PR middle-end/65958
- From: Richard Earnshaw <Richard dot Earnshaw at foss dot arm dot com>
- To: Eric Botcazou <ebotcazou at adacore dot com>
- Cc: gcc-patches at gcc dot gnu dot org, Ramana Radhakrishnan <ramana dot radhakrishnan at foss dot arm dot com>
- Date: Thu, 3 Dec 2015 11:08:42 +0000
- Subject: Re: [ARM] Fix PR middle-end/65958
- Authentication-results: sourceware.org; auth=none
- References: <1478566 dot ZKXszbaoG4 at polaris> <9319219 dot YanzbaT3s8 at polaris> <5638F077 dot 5050105 at foss dot arm dot com> <11225412 dot 5B49tQ9fvN at polaris>
Sorry for the delay, very busy on other things these days...
On 16/11/15 20:00, Eric Botcazou wrote:
>> More comments inline.
>
> Revised version attached, which addresses all your comments and in
particular
> removes the
>
> +#if PROBE_INTERVAL > 4096
> +#error Cannot use indexed addressing mode for stack probing
> +#endif
>
> compile-time assertion. It generates the same code for PROBE_INTERVAL
== 4096
> as before and it generates code that can be assembled for 8192.
>
> Tested on Aarch64/Linux, OK for the mainline?
>
> +#define PROBE_INTERVAL (1 << STACK_CHECK_PROBE_INTERVAL_EXP)
> +
> +/* We use the 12-bit shifted immediate arithmetic instructions so values
> + must be multiple of (1 << 12), i.e. 4096. */
> +#if (PROBE_INTERVAL % 4096) != 0
I can understand this restriction, but...
> + /* See the same assertion on PROBE_INTERVAL above. */
> + gcc_assert ((first % 4096) == 0);
... why isn't this a test that FIRST is aligned to PROBE_INTERVAL?
> + /* See if we have a constant small number of probes to generate.
If so,
> + that's the easy case. */
> + if (size <= PROBE_INTERVAL)
> + {
> + const HOST_WIDE_INT base = ROUND_UP (size, 4096);
> + emit_set_insn (reg1,
blank line between declarations and code. Also, can we come up with a
suitable define for 4096 here that expresses the context and then use
that consistently through the remainder of this function?
> +(define_insn "probe_stack_range"
> + [(set (match_operand:DI 0 "register_operand" "=r")
> + (unspec_volatile:DI [(match_operand:DI 1 "register_operand" "0")
> + (match_operand:DI 2 "register_operand" "r")]
> + UNSPEC_PROBE_STACK_RANGE))]
I think this should really use PTRmode, so that it's ILP32 ready (I'm
not going to ask you to make sure that works though, since I suspect
there are still other issues to resolve with ILP32 at this time).
R.