* PR 48826: NOTE_INSN_CALL_ARG_LOCATION vs. define_split
@ 2011-05-23 2:07 Richard Sandiford
2011-05-23 9:30 ` Eric Botcazou
0 siblings, 1 reply; 6+ messages in thread
From: Richard Sandiford @ 2011-05-23 2:07 UTC (permalink / raw)
To: gcc-patches
In PR 44826, we split a pre-reload call into another call followed
by a load of GP. The problem is that we're running the split pass after
var-tracking, and the original call had a NOTE_INSN_CALL_ARG_LOCATION
attached to it. We need to move the note to the new call, just like we
already move other properties of the call.
This shows up on 32-bit MIPS GNU/Linux at -O -g, because the first
post-epilogue scheduling pass is after var tracking. It doesn't
show up on -O2 and above because an earlier pass does the split.
I think the bug could in principle trigger on other targets with
call-clobbered GPs (e.g. some variants of Alpha, ia64 and PA).
However, most other ports split on reload_completed, whereas MIPS
does it on epilogue_completed, so perhaps it really is only MIPS.
Tested on mips-linux-gnu and x86_64-linux-gnu. OK to install?
Richard
gcc/
PR rtl-optimization/48826
* emit-rtl.c (try_split): When splitting a call that is followed
by a NOTE_INSN_CALL_ARG_LOCATION, move the note after the new call.
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c 2011-05-22 17:13:06.000000000 +0100
+++ gcc/emit-rtl.c 2011-05-22 20:37:09.000000000 +0100
@@ -3476,11 +3476,27 @@ try_split (rtx pat, rtx trial, int last)
for (insn = insn_last; insn ; insn = PREV_INSN (insn))
if (CALL_P (insn))
{
+ rtx next;
rtx *p = &CALL_INSN_FUNCTION_USAGE (insn);
while (*p)
p = &XEXP (*p, 1);
*p = CALL_INSN_FUNCTION_USAGE (trial);
SIBLING_CALL_P (insn) = SIBLING_CALL_P (trial);
+ /* If the new call is the last instruction in the sequence,
+ it will effectively replace the old call in-situ. Otherwise
+ we must move any following NOTE_INSN_CALL_ARG_LOCATION note
+ so that it comes immediately after the new call. */
+ if (NEXT_INSN (insn))
+ {
+ next = NEXT_INSN (trial);
+ if (next
+ && NOTE_P (next)
+ && NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
+ {
+ remove_insn (next);
+ add_insn_after (next, insn, NULL);
+ }
+ }
}
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PR 48826: NOTE_INSN_CALL_ARG_LOCATION vs. define_split
2011-05-23 2:07 PR 48826: NOTE_INSN_CALL_ARG_LOCATION vs. define_split Richard Sandiford
@ 2011-05-23 9:30 ` Eric Botcazou
2011-05-23 20:16 ` Richard Sandiford
0 siblings, 1 reply; 6+ messages in thread
From: Eric Botcazou @ 2011-05-23 9:30 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> PR rtl-optimization/48826
> * emit-rtl.c (try_split): When splitting a call that is followed
> by a NOTE_INSN_CALL_ARG_LOCATION, move the note after the new call.
OK if you move up the comment and merge it in the comment of the block. And it
would be nice to add the missing blurb about the SIBLING_CALL_P flag. TIA.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PR 48826: NOTE_INSN_CALL_ARG_LOCATION vs. define_split
2011-05-23 9:30 ` Eric Botcazou
@ 2011-05-23 20:16 ` Richard Sandiford
2011-05-23 22:46 ` Eric Botcazou
2011-05-29 1:07 ` Richard Sandiford
0 siblings, 2 replies; 6+ messages in thread
From: Richard Sandiford @ 2011-05-23 20:16 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
Eric Botcazou <ebotcazou@adacore.com> writes:
>> PR rtl-optimization/48826
>> * emit-rtl.c (try_split): When splitting a call that is followed
>> by a NOTE_INSN_CALL_ARG_LOCATION, move the note after the new call.
>
> OK if you move up the comment and merge it in the comment of the
> block. And it would be nice to add the missing blurb about the
> SIBLING_CALL_P flag. TIA.
Doh, good catch.
In the end, it felt a bit awkward stringing together three essentially
separate bits of info into one comment, so I instead generalised the
block comment and added specific comments above each section.
It sounded like you were more concerned with having the comments staying
in sync than the actual specifics, so I went ahead and installed the
patch below. Please let me know if you'd prefer something different
though.
Thanks,
Richard
gcc/
PR rtl-optimization/48826
* emit-rtl.c (try_split): When splitting a call that is followed
by a NOTE_INSN_CALL_ARG_LOCATION, move the note after the new call.
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c 2011-05-22 22:19:08.000000000 +0100
+++ gcc/emit-rtl.c 2011-05-23 18:48:23.000000000 +0100
@@ -3470,17 +3470,40 @@ try_split (rtx pat, rtx trial, int last)
}
/* If we are splitting a CALL_INSN, look for the CALL_INSN
- in SEQ and copy our CALL_INSN_FUNCTION_USAGE to it. */
+ in SEQ and copy any additional information across. */
if (CALL_P (trial))
{
for (insn = insn_last; insn ; insn = PREV_INSN (insn))
if (CALL_P (insn))
{
- rtx *p = &CALL_INSN_FUNCTION_USAGE (insn);
+ rtx next, *p;
+
+ /* Add the old CALL_INSN_FUNCTION_USAGE to whatever the
+ target may have explicitly specified. */
+ p = &CALL_INSN_FUNCTION_USAGE (insn);
while (*p)
p = &XEXP (*p, 1);
*p = CALL_INSN_FUNCTION_USAGE (trial);
+
+ /* If the old call was a sibling call, the new one must
+ be too. */
SIBLING_CALL_P (insn) = SIBLING_CALL_P (trial);
+
+ /* If the new call is the last instruction in the sequence,
+ it will effectively replace the old call in-situ. Otherwise
+ we must move any following NOTE_INSN_CALL_ARG_LOCATION note
+ so that it comes immediately after the new call. */
+ if (NEXT_INSN (insn))
+ {
+ next = NEXT_INSN (trial);
+ if (next
+ && NOTE_P (next)
+ && NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
+ {
+ remove_insn (next);
+ add_insn_after (next, insn, NULL);
+ }
+ }
}
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PR 48826: NOTE_INSN_CALL_ARG_LOCATION vs. define_split
2011-05-23 20:16 ` Richard Sandiford
@ 2011-05-23 22:46 ` Eric Botcazou
2011-05-29 1:07 ` Richard Sandiford
1 sibling, 0 replies; 6+ messages in thread
From: Eric Botcazou @ 2011-05-23 22:46 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> It sounded like you were more concerned with having the comments staying
> in sync than the actual specifics, so I went ahead and installed the
> patch below.
That's fine, thanks.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PR 48826: NOTE_INSN_CALL_ARG_LOCATION vs. define_split
2011-05-23 20:16 ` Richard Sandiford
2011-05-23 22:46 ` Eric Botcazou
@ 2011-05-29 1:07 ` Richard Sandiford
2011-05-29 14:31 ` Eric Botcazou
1 sibling, 1 reply; 6+ messages in thread
From: Richard Sandiford @ 2011-05-29 1:07 UTC (permalink / raw)
To: Eric Botcazou; +Cc: gcc-patches
Richard Sandiford <rdsandiford@googlemail.com> writes:
> gcc/
> PR rtl-optimization/48826
> * emit-rtl.c (try_split): When splitting a call that is followed
> by a NOTE_INSN_CALL_ARG_LOCATION, move the note after the new call.
It turns out that this isn't enough. In the attached testcase,
we have a:
(var_location pc (nil))
note between the call and the CALL_ARG_LOCATION. A quick look
at the var-tracking flow suggests that this is legitimate,
so we need to look at every note before the next real insn,
rather than just looking at the first.
I assume the ARM, SH and S390 reorg code would also need to cope
with this case.
If this usage isn't legitimate, and the CALL_ARG_LOCATION is supposed
to come directly after the call, then it would be great if someone more
familiar with the var-tracking code could have a look at it.
Otherwise, patch bootstrapped & regression tested on x86_64-linux-gnu.
Also tested on mips-linux-gnu. OK to install?
Richard
gcc/
* emit-rtl.c (try_split): Use a loop to search for
NOTE_INSN_CALL_ARG_LOCATIONs.
gcc/testsuite/
From Ryan Mansfield
* gcc.dg/pr48826.c: New test.
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c 2011-05-28 17:08:48.000000000 +0100
+++ gcc/emit-rtl.c 2011-05-28 18:23:28.000000000 +0100
@@ -3494,16 +3494,15 @@ try_split (rtx pat, rtx trial, int last)
we must move any following NOTE_INSN_CALL_ARG_LOCATION note
so that it comes immediately after the new call. */
if (NEXT_INSN (insn))
- {
- next = NEXT_INSN (trial);
- if (next
- && NOTE_P (next)
- && NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
+ for (next = NEXT_INSN (trial);
+ next && NOTE_P (next);
+ next = NEXT_INSN (next))
+ if (NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
{
remove_insn (next);
add_insn_after (next, insn, NULL);
+ break;
}
- }
}
}
Index: gcc/testsuite/gcc.dg/pr48826.c
===================================================================
--- /dev/null 2011-05-28 09:03:52.070295797 +0100
+++ gcc/testsuite/gcc.dg/pr48826.c 2011-05-28 18:12:00.000000000 +0100
@@ -0,0 +1,10 @@
+/* { dg-options "-O -g -w" } */
+
+void bar (int *);
+
+void
+foo ()
+{
+ int *const pc = __builtin_return_address (0);
+ bar (pc);
+}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PR 48826: NOTE_INSN_CALL_ARG_LOCATION vs. define_split
2011-05-29 1:07 ` Richard Sandiford
@ 2011-05-29 14:31 ` Eric Botcazou
0 siblings, 0 replies; 6+ messages in thread
From: Eric Botcazou @ 2011-05-29 14:31 UTC (permalink / raw)
To: Richard Sandiford; +Cc: gcc-patches
> If this usage isn't legitimate, and the CALL_ARG_LOCATION is supposed
> to come directly after the call, then it would be great if someone more
> familiar with the var-tracking code could have a look at it.
"Be liberal in what you accept" says the common wisdom.
> gcc/
> * emit-rtl.c (try_split): Use a loop to search for
> NOTE_INSN_CALL_ARG_LOCATIONs.
>
> gcc/testsuite/
> From Ryan Mansfield
> * gcc.dg/pr48826.c: New test.
OK, thanks.
--
Eric Botcazou
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-05-29 8:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-23 2:07 PR 48826: NOTE_INSN_CALL_ARG_LOCATION vs. define_split Richard Sandiford
2011-05-23 9:30 ` Eric Botcazou
2011-05-23 20:16 ` Richard Sandiford
2011-05-23 22:46 ` Eric Botcazou
2011-05-29 1:07 ` Richard Sandiford
2011-05-29 14:31 ` Eric Botcazou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).