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.
Uros.
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]));
