On Thu, Feb 06, 2020 at 01:15:25PM -0600, Segher Boessenkool wrote:
> On Thu, Jan 30, 2020 at 05:14:08PM +0100, Jakub Jelinek wrote:
> > Here is what I meant as the alternative, i.e. don't check any predicates,
> > just gen_add3_insn, if that fails, force rs into register and retry.
>
> I don't like gen_add3_insn here *at all*, as I said, but okay, you're
> only fixing existing code. But as long as it is there, this code will
> be a problem child.
gen_add3_insn is used 25 times elsewhere in the rs6000 backend when not
counting these 2 calls that were just slightly moved around by the patch.
> > And, add REG_FRAME_RELATED_EXPR note always when we haven't emitted a single
> > insn that has rtl exactly matching what we'd add the REG_FRAME_RELATED_EXPR
> > with (in that case, dwarf2cfi.c is able to figure it out by itself, no need
> > to waste compile time memory).
>
> I would say "just always emit that note", but that is what the patch
> does, already :-)
No, the patch doesn't emit it always, see below.
> > + rtx set;
> > + if (!NONJUMP_INSN_P (insn)
> > + || NEXT_INSN (insn)
> > + || (set = single_set (insn)) == NULL_RTX
> > + || SET_DEST (set) != end_addr
> > + || GET_CODE (SET_SRC (set)) != PLUS
> > + || XEXP (SET_SRC (set), 0) != stack_pointer_rtx
> > + || XEXP (SET_SRC (set), 1) != rs)
>
> Please don't have side effects in conditions. Two nested ifs would be
> fine here.
Ok, so like attached? There is an add_note boolean, so that the
insn = emit_insn (insn); doesn't have to be done 3 times.
> > + insn = emit_insn (insn);
> > + /* Describe the effect of INSN to the CFI engine, unless it
> > + is a single insn that describes it itself. */
> > add_reg_note (insn, REG_FRAME_RELATED_EXPR,
> > gen_rtx_SET (end_addr,
> > gen_rtx_PLUS (Pmode, stack_pointer_rtx,
> > rs)));
>
> So please fix the comment?
The point is not to add the REG_FRAME_RELATED_EXPR note in the most common case
where it would be just waste of compile time memory.
E.g.
(insn/f 17 16 18 2 (set (reg:DI 12 12)
(plus:DI (reg/f:DI 1 1)
(const_int -40960 [0xffffffffffff6000]))) "pr93122-2.c":9:1 -1
(nil))
doesn't need the note, the /f flag on the insn is all that is needed,
because the instruction is self-descriptive to dwarf2frame.c. The note
we'd add in that case would be
(set (reg:DI 12 12) (plus:DI (reg/f:DI 1 1) (const_int -40960)))
but that is the PATTERN of the insn already.
We need it in all other cases, when there is more than one insn doing that
or it isn't like that (end_addr = stack_pointer_rtx + rs).
So e.g. on the testcase in the patch,
(insn 17 16 18 2 (set (reg:DI 12 12)
(const_int -4294967296 [0xffffffff00000000])) "pr93122.c":9:1 -1
(nil))
(insn/f 18 17 19 2 (set (reg:DI 12 12)
(plus:DI (reg:DI 12 12)
(reg/f:DI 1 1))) "pr93122.c":9:1 -1
(expr_list:REG_FRAME_RELATED_EXPR (set (reg:DI 12 12)
(plus:DI (reg/f:DI 1 1)
(const_int -4294967296 [0xffffffff00000000])))
(nil)))
Only the last insn is /f marked and the effect of it is the one
written in the note.
2020-02-06 Jakub Jelinek <ja...@redhat.com>
PR target/93122
* config/rs6000/rs6000-logue.c
(rs6000_emit_probe_stack_range_stack_clash): Always use gen_add3_insn,
if it fails, move rs into end_addr and retry. Add
REG_FRAME_RELATED_EXPR note whenever it returns more than one insn or
the insn pattern doesn't describe well what exactly happens to
dwarf2cfi.c.
* gcc.target/powerpc/pr93122.c: New test.
--- gcc/config/rs6000/rs6000-logue.c.jj 2020-01-30 17:55:38.606339203 +0100
+++ gcc/config/rs6000/rs6000-logue.c 2020-02-06 20:36:21.511409319 +0100
@@ -1604,20 +1604,34 @@ rs6000_emit_probe_stack_range_stack_clas
rtx end_addr
= copy_reg ? gen_rtx_REG (Pmode, 0) : gen_rtx_REG (Pmode, 12);
rtx rs = GEN_INT (-rounded_size);
- rtx_insn *insn;
- if (add_operand (rs, Pmode))
- insn = emit_insn (gen_add3_insn (end_addr, stack_pointer_rtx, rs));
+ rtx_insn *insn = gen_add3_insn (end_addr, stack_pointer_rtx, rs);
+ if (insn == NULL)
+ {
+ emit_move_insn (end_addr, rs);
+ insn = gen_add3_insn (end_addr, end_addr, stack_pointer_rtx);
+ gcc_assert (insn);
+ }
+ bool add_note = false;
+ if (!NONJUMP_INSN_P (insn) || NEXT_INSN (insn))
+ add_note = true;
else
{
- emit_move_insn (end_addr, GEN_INT (-rounded_size));
- insn = emit_insn (gen_add3_insn (end_addr, end_addr,
- stack_pointer_rtx));
- /* Describe the effect of INSN to the CFI engine. */
- add_reg_note (insn, REG_FRAME_RELATED_EXPR,
- gen_rtx_SET (end_addr,
- gen_rtx_PLUS (Pmode, stack_pointer_rtx,
- rs)));
+ rtx set = single_set (insn);
+ if (set == NULL_RTX
+ || SET_DEST (set) != end_addr
+ || GET_CODE (SET_SRC (set)) != PLUS
+ || XEXP (SET_SRC (set), 0) != stack_pointer_rtx
+ || XEXP (SET_SRC (set), 1) != rs)
+ add_note = true;
}
+ insn = emit_insn (insn);
+ if (add_note)
+ /* Describe the effect of INSN to the CFI engine, unless it
+ is a single insn that describes it itself. */
+ add_reg_note (insn, REG_FRAME_RELATED_EXPR,
+ gen_rtx_SET (end_addr,
+ gen_rtx_PLUS (Pmode, stack_pointer_rtx,
+ rs)));
RTX_FRAME_RELATED_P (insn) = 1;
/* Emit the loop. */
--- gcc/testsuite/gcc.target/powerpc/pr93122.c.jj 2020-02-06
20:29:58.985147528 +0100
+++ gcc/testsuite/gcc.target/powerpc/pr93122.c 2020-02-06 20:29:58.985147528
+0100
@@ -0,0 +1,12 @@
+/* PR target/93122 */
+/* { dg-do compile { target lp64 } } */
+/* { dg-options "-fstack-clash-protection -mprefixed-addr -mfuture" } */
+
+void bar (char *);
+
+void
+foo (void)
+{
+ char s[4294967296];
+ bar (s);
+}
Jakub
