On 10/04/18 08:37, Uros Bizjak wrote:
> On Tue, Apr 10, 2018 at 12:26 AM, Jeff Law <[email protected]> wrote:
>> On 04/05/2018 08:20 AM, Kyrill Tkachov wrote:
>>> Hi all,
>>>
>>> In this PR the expansion code emits an invalid memory address for the
>>> stack probe, which the backend fails to recognise.
>>> The address is created explicitly in
>>> anti_adjust_stack_and_probe_stack_clash in explow.c and passed down to
>>> gen_probe_stack
>>> without any validation in emit_stack_probe.
>>>
>>> This patch fixes the ICE by calling validize_mem on the memory location
>>> before passing it down to the target.
>>> Jakub pointed out that we also want to create valid addresses for the
>>> probe_stack_address case, so this patch
>>> creates an expand operand and legitimizes it before passing it down to
>>> the probe_stack_address expander.
>>>
>>> This patch passes bootstrap and testing on arm-none-linux-gnueabihf and
>>> aarch64-none-linux-gnu
>>> and ppc64le-redhat-linux on gcc112 in the compile farm.
>>>
>>> Is this ok for trunk?
>>>
>>> Thanks,
>>> Kyrill
>>>
>>> P.S. Uros, the alpha probe_stack expander in alpha.md seems incompatible
>>> with the way the probe_stack name is
>>> used in the midend. It accepts a const_int operand that is used as an
>>> offset from the stack pointer, rather than accepting
>>> a full memory operand like other targets. Do you think it's better to
>>> rename the probe_stack pattern there to something
>>> that doesn't conflict with the name the midend assumes?
>>>
>>> 2018-04-05 Kyrylo Tkachov <[email protected]>
>>>
>>> PR target/85173
>>> * explow.c (emit_stack_probe): Call validize_mem on memory location
>>> before passing it to gen_probe_stack. Create address operand and
>>> legitimize it for the probe_stack_address case.
>>>
>>> 2018-04-05 Kyrylo Tkachov <[email protected]>
>>>
>>> PR target/85173
>>> * gcc.target/arm/pr85173.c: New test.
>> Alpha should be fixed -- the docs clearly state that the operand is "the
>> memory reference in the stack that needs to be probed". Just passing in
>> the offset seems wrong.
>
> This pattern has to be renamed to not clash with the standard pattern name.
>
> I'm testing the attached patch.
>
Ugh! Two different APIs, one called gen_stack_probe and one
gen_probe_stack? That has to be a recipe for disaster!
R.
> Uros.
>
>
> a.diff.txt
>
>
> diff --git a/gcc/config/alpha/alpha.c b/gcc/config/alpha/alpha.c
> index a039f045262c..3222cb716b63 100644
> --- a/gcc/config/alpha/alpha.c
> +++ b/gcc/config/alpha/alpha.c
> @@ -7771,13 +7771,13 @@ alpha_expand_prologue (void)
> int probed;
>
> for (probed = 4096; probed < probed_size; probed += 8192)
> - emit_insn (gen_probe_stack (GEN_INT (-probed)));
> + emit_insn (gen_stack_probe (GEN_INT (-probed)));
>
> /* We only have to do this probe if we aren't saving registers or
> if we are probing beyond the frame because of -fstack-check. */
> if ((sa_size == 0 && probed_size > probed - 4096)
> || flag_stack_check || flag_stack_clash_protection)
> - emit_insn (gen_probe_stack (GEN_INT (-probed_size)));
> + emit_insn (gen_stack_probe (GEN_INT (-probed_size)));
> }
>
> if (frame_size != 0)
> diff --git a/gcc/config/alpha/alpha.md b/gcc/config/alpha/alpha.md
> index 5d82e5a4adf7..6b99fce643b4 100644
> --- a/gcc/config/alpha/alpha.md
> +++ b/gcc/config/alpha/alpha.md
> @@ -4851,7 +4851,7 @@
>
>
> ;; Subroutine of stack space allocation. Perform a stack probe.
> -(define_expand "probe_stack"
> +(define_expand "stack_probe"
> [(set (match_dup 1) (match_operand:DI 0 "const_int_operand"))]
> ""
> {
> @@ -4886,12 +4886,12 @@
>
> int probed = 4096;
>
> - emit_insn (gen_probe_stack (GEN_INT (- probed)));
> + emit_insn (gen_stack_probe (GEN_INT (- probed)));
> while (probed + 8192 < INTVAL (operands[1]))
> - emit_insn (gen_probe_stack (GEN_INT (- (probed += 8192))));
> + emit_insn (gen_stack_probe (GEN_INT (- (probed += 8192))));
>
> if (probed + 4096 < INTVAL (operands[1]))
> - emit_insn (gen_probe_stack (GEN_INT (- INTVAL(operands[1]))));
> + emit_insn (gen_stack_probe (GEN_INT (- INTVAL(operands[1]))));
> }
>
> operands[1] = GEN_INT (- INTVAL (operands[1]));
>
