Re: [PATCH] emit-rtl.c: Allow splitting of RTX_FRAME_RELATED_P insns?
Sorry for the slow reply.
Senthil Kumar Selvaraj writes:
> 2020-08-13 Senthil Kumar Selvaraj
>
> gcc/ChangeLog:
>
> * emit-rtl.c (try_split): Call copy_frame_info_to_split_insn
> to split certain RTX_FRAME_RELATED_P insns.
> * recog.c (copy_frame_info_to_split_insn): New function.
> (peep2_attempt): Split copying of frame related info of
> RTX_FRAME_RELATED_P insns into above function and call it.
> * recog.h (copy_frame_info_to_split_insn): Declare it.
>
> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index f9b0e9714d9..3706f0a03fd 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -3822,10 +3822,6 @@ try_split (rtx pat, rtx_insn *trial, int last)
>int njumps = 0;
>rtx_insn *call_insn = NULL;
>
> - /* We're not good at redistributing frame information. */
> - if (RTX_FRAME_RELATED_P (trial))
> -return trial;
> -
>if (any_condjump_p (trial)
>&& (note = find_reg_note (trial, REG_BR_PROB, 0)))
> split_branch_probability
> @@ -3842,6 +3838,7 @@ try_split (rtx pat, rtx_insn *trial, int last)
>if (!seq)
> return trial;
>
> + int split_insn_count = 0;
>/* Avoid infinite loop if any insn of the result matches
> the original pattern. */
>insn_last = seq;
> @@ -3850,11 +3847,25 @@ try_split (rtx pat, rtx_insn *trial, int last)
>if (INSN_P (insn_last)
> && rtx_equal_p (PATTERN (insn_last), pat))
> return trial;
> + split_insn_count++;
>if (!NEXT_INSN (insn_last))
> break;
>insn_last = NEXT_INSN (insn_last);
> }
>
> + /* We're not good at redistributing frame information if
> + the split occurs before reload or if it results in more
> + than one insn. */
> + if (RTX_FRAME_RELATED_P (trial))
> +{
> + if (!reload_completed || split_insn_count != 1)
> +return trial;
> +
> + rtx_insn *new_insn = seq;
> + rtx_insn *old_insn = trial;
> + copy_frame_info_to_split_insn (old_insn, new_insn);
> +}
> +
>/* We will be adding the new sequence to the function. The splitters
> may have introduced invalid RTL sharing, so unshare the sequence now.
> */
>unshare_all_rtl_in_chain (seq);
> diff --git a/gcc/recog.c b/gcc/recog.c
> index 25f19b1b1cf..e024597f9d7 100644
> --- a/gcc/recog.c
> +++ b/gcc/recog.c
> @@ -3277,6 +3277,78 @@ peep2_reinit_state (regset live)
>COPY_REG_SET (peep2_insn_data[MAX_INSNS_PER_PEEP2].live_before, live);
> }
>
> +/* Copies frame related info of an insn (old_insn) to the single
> + insn (new_insn) that was obtained by splitting old_insn. */
By convention, old_insn and new_insn should be in caps, since they
refer to parameter names.
OK otherwise, thanks.
Richard
Re: [PATCH] emit-rtl.c: Allow splitting of RTX_FRAME_RELATED_P insns?
Richard Sandiford writes:
> Senthil Kumar via Gcc-patches writes:
>> Hi,
>>
>> I'm working on converting the AVR backend to MODE_CC, following
>> the steps described for case #2 in the CC0 transition wiki page,
>> and I've implemented the first three bullet
>> points (https://github.com/saaadhu/gcc-avr-cc0/tree/avr-cc0-squashed). With
>> the below patch, there are zero regressions (for mega and xmega
>> subarchs) compared to the current mainline, as of yesterday.
>>
>> The wiki suggests using post-reload splitters, so that's the
>> direction I took, but I ran into an issue where split_insn
>> bails out early if RTX_FRAME_RELATED_P is true - this means
>> that splits for REG_CC clobbering insns with
>> RTX_FRAME_RELATED_P will never execute, resulting in a
>> could-not-split insn ICE in the final stage.
>>
>> I see that the recog.c:peep2_attempt allows splitting of a
>> RTX_FRAME_RELATED_P insn, provided the result of the split is a
>> single insn. Would it be ok to modify try_split also to
>> allow those kinds of insns (tentative patch attached, code
>> copied over from peep2_attempt, only setting old and new_insn)? Or is there
>> a different approach to fix this?
>
> I agree there's no obvious reason why splitting to a single insn
> should be rejected but a peephole2 to a single instruction should be OK.
> And reusing the existing, tried-and-tested code is the way to go.
>
> But could you split the code out of peep2_attempt into a subroutine
> (probably still in recog.c) and reuse it in try_split?
How does the below patch look? Bootstrapped and reg tested on
x86_64-linux.
>
> BTW, just to check: is your email address in MAINTAINERS still correct?
It was out-of-date, yes - updated now.
Regards
Senthil
2020-08-13 Senthil Kumar Selvaraj
gcc/ChangeLog:
* emit-rtl.c (try_split): Call copy_frame_info_to_split_insn
to split certain RTX_FRAME_RELATED_P insns.
* recog.c (copy_frame_info_to_split_insn): New function.
(peep2_attempt): Split copying of frame related info of
RTX_FRAME_RELATED_P insns into above function and call it.
* recog.h (copy_frame_info_to_split_insn): Declare it.
diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index f9b0e9714d9..3706f0a03fd 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -3822,10 +3822,6 @@ try_split (rtx pat, rtx_insn *trial, int last)
int njumps = 0;
rtx_insn *call_insn = NULL;
- /* We're not good at redistributing frame information. */
- if (RTX_FRAME_RELATED_P (trial))
-return trial;
-
if (any_condjump_p (trial)
&& (note = find_reg_note (trial, REG_BR_PROB, 0)))
split_branch_probability
@@ -3842,6 +3838,7 @@ try_split (rtx pat, rtx_insn *trial, int last)
if (!seq)
return trial;
+ int split_insn_count = 0;
/* Avoid infinite loop if any insn of the result matches
the original pattern. */
insn_last = seq;
@@ -3850,11 +3847,25 @@ try_split (rtx pat, rtx_insn *trial, int last)
if (INSN_P (insn_last)
&& rtx_equal_p (PATTERN (insn_last), pat))
return trial;
+ split_insn_count++;
if (!NEXT_INSN (insn_last))
break;
insn_last = NEXT_INSN (insn_last);
}
+ /* We're not good at redistributing frame information if
+ the split occurs before reload or if it results in more
+ than one insn. */
+ if (RTX_FRAME_RELATED_P (trial))
+{
+ if (!reload_completed || split_insn_count != 1)
+return trial;
+
+ rtx_insn *new_insn = seq;
+ rtx_insn *old_insn = trial;
+ copy_frame_info_to_split_insn (old_insn, new_insn);
+}
+
/* We will be adding the new sequence to the function. The splitters
may have introduced invalid RTL sharing, so unshare the sequence now. */
unshare_all_rtl_in_chain (seq);
diff --git a/gcc/recog.c b/gcc/recog.c
index 25f19b1b1cf..e024597f9d7 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -3277,6 +3277,78 @@ peep2_reinit_state (regset live)
COPY_REG_SET (peep2_insn_data[MAX_INSNS_PER_PEEP2].live_before, live);
}
+/* Copies frame related info of an insn (old_insn) to the single
+ insn (new_insn) that was obtained by splitting old_insn. */
+
+void
+copy_frame_info_to_split_insn (rtx_insn *old_insn, rtx_insn *new_insn)
+{
+ bool any_note = false;
+ rtx note;
+
+ if (!RTX_FRAME_RELATED_P (old_insn))
+return;
+
+ RTX_FRAME_RELATED_P (new_insn) = 1;
+
+ /* Allow the backend to fill in a note during the split. */
+ for (note = REG_NOTES (new_insn); note ; note = XEXP (note, 1))
+switch (REG_NOTE_KIND (note))
+ {
+ case REG_FRAME_RELATED_EXPR:
+ case REG_CFA_DEF_CFA:
+ case REG_CFA_ADJUST_CFA:
+ case REG_CFA_OFFSET:
+ case REG_CFA_REGISTER:
+ case REG_CFA_EXPRESSION:
+ case REG_CFA_RESTORE:
+ case REG_CFA_SET_VDRAP:
+any_note = true;
+break;
+ default:
+break;
+ }
+
+ /* If the backend didn't supply a note, copy
Re: [PATCH] emit-rtl.c: Allow splitting of RTX_FRAME_RELATED_P insns?
On Tue, Aug 11, 2020 at 07:59:44AM +0100, Richard Sandiford wrote: > >> I agree there's no obvious reason why splitting to a single insn > >> should be rejected but a peephole2 to a single instruction should be OK. > >> And reusing the existing, tried-and-tested code is the way to go. > > > > The only obvious difference is that the splitters run many times, while > > peep2 runs only once, very late. If you make this only do stuff for > > reload_completed splitters, that difference is gone as well. > > Yeah, but I was talking specifically about RTX_FRAME_RELATED_P stuff, > rather than in general, and RTX_FRAME_RELATED_P insns shouldn't exist > until prologue/epilogue generation. Yeah, good points. > The reference to “single insn” > was because both passes would still reject splitting/peepholing an > RTX_FRAME_RELATED_P insn to multiple insns. Is that the only thing RTX_FRAME_RELATED_P is actually useful for now, btw? (I do get that it is handy to have the simple cases in the prologue automatically figured out, but that is at best a nicety). What is actually bad about splitting FRAME_RELATED insns, anyway? I can think of many things that could go wrong, but all of those can go wrong with 1-1 splits as well. Maybe this all just works because not very many 1-1 splits are used in practice? So many questions, feel free to ignore all :-) Segher
Re: [PATCH] emit-rtl.c: Allow splitting of RTX_FRAME_RELATED_P insns?
Segher Boessenkool writes: > On Mon, Aug 10, 2020 at 05:16:15PM +0100, Richard Sandiford wrote: >> Senthil Kumar via Gcc-patches writes: >> > The wiki suggests using post-reload splitters, so that's the >> > direction I took, but I ran into an issue where split_insn >> > bails out early if RTX_FRAME_RELATED_P is true - this means >> > that splits for REG_CC clobbering insns with >> > RTX_FRAME_RELATED_P will never execute, resulting in a >> > could-not-split insn ICE in the final stage. >> > >> > I see that the recog.c:peep2_attempt allows splitting of a >> > RTX_FRAME_RELATED_P insn, provided the result of the split is a >> > single insn. Would it be ok to modify try_split also to >> > allow those kinds of insns (tentative patch attached, code >> > copied over from peep2_attempt, only setting old and new_insn)? Or is >> > there >> > a different approach to fix this? >> >> I agree there's no obvious reason why splitting to a single insn >> should be rejected but a peephole2 to a single instruction should be OK. >> And reusing the existing, tried-and-tested code is the way to go. > > The only obvious difference is that the splitters run many times, while > peep2 runs only once, very late. If you make this only do stuff for > reload_completed splitters, that difference is gone as well. Yeah, but I was talking specifically about RTX_FRAME_RELATED_P stuff, rather than in general, and RTX_FRAME_RELATED_P insns shouldn't exist until prologue/epilogue generation. The reference to “single insn” was because both passes would still reject splitting/peepholing an RTX_FRAME_RELATED_P insn to multiple insns. Thanks, Richard
Re: [PATCH] emit-rtl.c: Allow splitting of RTX_FRAME_RELATED_P insns?
On Mon, Aug 10, 2020 at 05:16:15PM +0100, Richard Sandiford wrote: > Senthil Kumar via Gcc-patches writes: > > The wiki suggests using post-reload splitters, so that's the > > direction I took, but I ran into an issue where split_insn > > bails out early if RTX_FRAME_RELATED_P is true - this means > > that splits for REG_CC clobbering insns with > > RTX_FRAME_RELATED_P will never execute, resulting in a > > could-not-split insn ICE in the final stage. > > > > I see that the recog.c:peep2_attempt allows splitting of a > > RTX_FRAME_RELATED_P insn, provided the result of the split is a > > single insn. Would it be ok to modify try_split also to > > allow those kinds of insns (tentative patch attached, code > > copied over from peep2_attempt, only setting old and new_insn)? Or is > > there > > a different approach to fix this? > > I agree there's no obvious reason why splitting to a single insn > should be rejected but a peephole2 to a single instruction should be OK. > And reusing the existing, tried-and-tested code is the way to go. The only obvious difference is that the splitters run many times, while peep2 runs only once, very late. If you make this only do stuff for reload_completed splitters, that difference is gone as well. > But could you split the code out of peep2_attempt into a subroutine > (probably still in recog.c) and reuse it in try_split? Yes please :-) Segher
Re: [PATCH] emit-rtl.c: Allow splitting of RTX_FRAME_RELATED_P insns?
Senthil Kumar via Gcc-patches writes: > Hi, > > I'm working on converting the AVR backend to MODE_CC, following > the steps described for case #2 in the CC0 transition wiki page, > and I've implemented the first three bullet > points (https://github.com/saaadhu/gcc-avr-cc0/tree/avr-cc0-squashed). With > the below patch, there are zero regressions (for mega and xmega > subarchs) compared to the current mainline, as of yesterday. > > The wiki suggests using post-reload splitters, so that's the > direction I took, but I ran into an issue where split_insn > bails out early if RTX_FRAME_RELATED_P is true - this means > that splits for REG_CC clobbering insns with > RTX_FRAME_RELATED_P will never execute, resulting in a > could-not-split insn ICE in the final stage. > > I see that the recog.c:peep2_attempt allows splitting of a > RTX_FRAME_RELATED_P insn, provided the result of the split is a > single insn. Would it be ok to modify try_split also to > allow those kinds of insns (tentative patch attached, code > copied over from peep2_attempt, only setting old and new_insn)? Or is there > a different approach to fix this? I agree there's no obvious reason why splitting to a single insn should be rejected but a peephole2 to a single instruction should be OK. And reusing the existing, tried-and-tested code is the way to go. But could you split the code out of peep2_attempt into a subroutine (probably still in recog.c) and reuse it in try_split? BTW, just to check: is your email address in MAINTAINERS still correct? Thanks, Richard
[PATCH] emit-rtl.c: Allow splitting of RTX_FRAME_RELATED_P insns?
Hi,
I'm working on converting the AVR backend to MODE_CC, following
the steps described for case #2 in the CC0 transition wiki page,
and I've implemented the first three bullet
points (https://github.com/saaadhu/gcc-avr-cc0/tree/avr-cc0-squashed). With
the below patch, there are zero regressions (for mega and xmega
subarchs) compared to the current mainline, as of yesterday.
The wiki suggests using post-reload splitters, so that's the
direction I took, but I ran into an issue where split_insn
bails out early if RTX_FRAME_RELATED_P is true - this means
that splits for REG_CC clobbering insns with
RTX_FRAME_RELATED_P will never execute, resulting in a
could-not-split insn ICE in the final stage.
I see that the recog.c:peep2_attempt allows splitting of a
RTX_FRAME_RELATED_P insn, provided the result of the split is a
single insn. Would it be ok to modify try_split also to
allow those kinds of insns (tentative patch attached, code
copied over from peep2_attempt, only setting old and new_insn)? Or is there
a different approach to fix this?
Regards
Senthil
gcc/ChangeLog:
* emit-rtl.c (try_split): Allow splitting
certain RTX_FRAME_RELATED_P insns.
diff --git gcc/emit-rtl.c gcc/emit-rtl.c
index f9b0e9714d9..7cf5704cf14 100644
--- gcc/emit-rtl.c
+++ gcc/emit-rtl.c
@@ -3822,10 +3822,6 @@ try_split (rtx pat, rtx_insn *trial, int last)
int njumps = 0;
rtx_insn *call_insn = NULL;
- /* We're not good at redistributing frame information. */
- if (RTX_FRAME_RELATED_P (trial))
-return trial;
-
if (any_condjump_p (trial)
&& (note = find_reg_note (trial, REG_BR_PROB, 0)))
split_branch_probability
@@ -3842,6 +3838,7 @@ try_split (rtx pat, rtx_insn *trial, int last)
if (!seq)
return trial;
+ int split_insn_count = 0;
/* Avoid infinite loop if any insn of the result matches
the original pattern. */
insn_last = seq;
@@ -3850,11 +3847,87 @@ try_split (rtx pat, rtx_insn *trial, int last)
if (INSN_P (insn_last)
&& rtx_equal_p (PATTERN (insn_last), pat))
return trial;
+ split_insn_count++;
if (!NEXT_INSN (insn_last))
break;
insn_last = NEXT_INSN (insn_last);
}
+ /* We're not good at redistributing frame information if
+ the split results in more than one insn */
+ if (RTX_FRAME_RELATED_P (trial))
+{
+ if (split_insn_count != 1)
+return trial;
+
+ // Copy from recog.c:peep2_attempt
+ bool any_note = false;
+ rtx note;
+ rtx_insn *new_insn = seq;
+ rtx_insn *old_insn = trial;
+
+ /* We have a 1-1 replacement. Copy over any frame-related info. */
+ RTX_FRAME_RELATED_P (new_insn) = 1;
+
+ /* Allow the backend to fill in a note during the split. */
+ for (note = REG_NOTES (new_insn); note ; note = XEXP (note, 1))
+switch (REG_NOTE_KIND (note))
+ {
+case REG_FRAME_RELATED_EXPR:
+case REG_CFA_DEF_CFA:
+case REG_CFA_ADJUST_CFA:
+case REG_CFA_OFFSET:
+case REG_CFA_REGISTER:
+case REG_CFA_EXPRESSION:
+case REG_CFA_RESTORE:
+case REG_CFA_SET_VDRAP:
+ any_note = true;
+ break;
+default:
+ break;
+ }
+
+ /* If the backend didn't supply a note, copy one over. */
+ if (!any_note)
+for (note = REG_NOTES (old_insn); note ; note = XEXP (note, 1))
+ switch (REG_NOTE_KIND (note))
+{
+ case REG_FRAME_RELATED_EXPR:
+ case REG_CFA_DEF_CFA:
+ case REG_CFA_ADJUST_CFA:
+ case REG_CFA_OFFSET:
+ case REG_CFA_REGISTER:
+ case REG_CFA_EXPRESSION:
+ case REG_CFA_RESTORE:
+ case REG_CFA_SET_VDRAP:
+add_reg_note (new_insn, REG_NOTE_KIND (note), XEXP (note, 0));
+any_note = true;
+ break;
+ default:
+break;
+}
+
+ /* If there still isn't a note, make sure the unwind info sees the
+ same expression as before the split. */
+ if (!any_note)
+{
+ rtx old_set, new_set;
+
+ /* The old insn had better have been simple, or annotated. */
+ old_set = single_set (old_insn);
+ gcc_assert (old_set != NULL);
+
+ new_set = single_set (new_insn);
+ if (!new_set || !rtx_equal_p (new_set, old_set))
+add_reg_note (new_insn, REG_FRAME_RELATED_EXPR, old_set);
+}
+
+ /* Copy prologue/epilogue status. This is required in order to keep
+ proper placement of EPILOGUE_BEG and the DW_CFA_remember_state. */
+ maybe_copy_prologue_epilogue_insn (old_insn, new_insn);
+ }
+
+
/* We will be adding the new sequence to the function. The splitters
may have introduced invalid RTL sharing, so unshare the sequence now. */
unshare_all_rtl_in_chain
