Merge lp:~cltang/gcc-linaro/lp-748138-cfgrtl-fix-4_5 into lp:gcc-linaro/4.5

[Chung-Lin Tang]

LP:748148 was caused by the shrink wrap changes, a case where -fno-shrink-wrap cannot help to avoid.

This patch fixes this. Was approved internally by Bernd.

[Linaro Toolchain Builder (cbuild)]

cbuild has taken a snapshot of this branch at r99510 and queued it for build.

The snapshot is available at:
 http://ex.seabright.co.nz/snapshots/gcc-linaro-4.5+bzr99510~cltang~lp-748138-cfgrtl-fix-4_5.tar.xdelta3.xz

and will be built on the following builders:
 a9-builder i686 x86_64

You can track the build queue at:
 http://ex.seabright.co.nz/helpers/scheduler

cbuild-snapshot: gcc-linaro-4.5+bzr99510~cltang~lp-748138-cfgrtl-fix-4_5
cbuild-ancestor: lp:gcc-linaro+bzr99509
cbuild-state: check

[Linaro Toolchain Builder (cbuild)]

cbuild successfully built this on i686-lucid-cbuild114-scorpius-i686r1.

The build results are available at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.5+bzr99510~cltang~lp-748138-cfgrtl-fix-4_5/logs/i686-lucid-cbuild114-scorpius-i686r1

The test suite results were unchanged compared to the branch point lp:gcc-linaro+bzr99509.

The full testsuite results are at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.5+bzr99510~cltang~lp-748138-cfgrtl-fix-4_5/logs/i686-lucid-cbuild114-scorpius-i686r1/gcc-testsuite.txt

cbuild-checked: i686-lucid-cbuild114-scorpius-i686r1

[Linaro Toolchain Builder (cbuild)]

cbuild successfully built this on armv7l-maverick-cbuild114-ursa4-cortexa9r1.

The build results are available at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.5+bzr99510~cltang~lp-748138-cfgrtl-fix-4_5/logs/armv7l-maverick-cbuild114-ursa4-cortexa9r1

The test suite results were unchanged compared to the branch point lp:gcc-linaro+bzr99509.

The full testsuite results are at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.5+bzr99510~cltang~lp-748138-cfgrtl-fix-4_5/logs/armv7l-maverick-cbuild114-ursa4-cortexa9r1/gcc-testsuite.txt

cbuild-checked: armv7l-maverick-cbuild114-ursa4-cortexa9r1

[Ulrich Weigand (uweigand)]

I'm not sure this change is the correct fix.

Originally, the code in question attempts to redirect a branch to some new target, which might be the exit block. This special case is used to insert direct (possibly conditional) return instructions on such targets that support this, and fails on other targets.

With your patch, the routine will now *always* fail when trying to redirect a branch to the exit block. This is not a correctness bug, but it might presumably lead to worse performance of generated code on platforms that would have supported direct returns.

The immediate cause of the internal compiler error is an API change. It used to be the case that you had to pass NULL_RTX as argument to redirect_jump to indicate that the branch should be redirected to the exit block. After the shrink-wrap changes, it is *still* possible to redirect a branch to exit, but this instead indicated by passing the appropriate return insn (e.g. a ret_rtx or a simple_return_rtx).

In some other places where redirect_jump used to be called with NULL_RTX, the shrink-wrap patch changed this to instead pass ret_rtx. See e.g. this change in dead_or_predictable:

      if (new_dest == EXIT_BLOCK_PTR)
        new_dest_label = ret_rtx;
      else
        new_dest_label = block_label (new_dest);

(followed by a call to redirect_jump (... new_dest_label ...)).

I would have thought we'd want a similar change in those cfgrtl.c locations. I must admit, though, that I'm not completely sure which type of return insn is actually the correct one; why is it ret_rtx (and not, say, simple_return_rtx)? Maybe Bernd can provide some additional information here ...

review: Needs Information

[Chung-Lin Tang (cltang)]

I had such concerns too at the start, about the case where new_bb ==
EXIT_BLOCK_PTR (and passing to redirect_jump() a NULL bb) returns true.

However, Bernd explained that these call sites were all before reload,
and no ret_rtx or simple_return_rtx is available at this stage. So the
"new shrink-wrap style" is not applicable here.

Bernd did suggest that maybe we can do an improvement where a new empty
BB with a label is inserted before the exit block, allowing fallthru.

Another possible way that I just thought of, would be to change the
redirect_jump() assertion to gcc_assert (!reload_completed || nlabel !=
NULL_RTX).

CCing Bernd here.

Thanks,
Chung-Lin

On 2011/5/16 05:40 PM, Ulrich Weigand wrote:
> Review: Needs Information
> I'm not sure this change is the correct fix.
>
> Originally, the code in question attempts to redirect a branch to some new target, which might be the exit block. This special case is used to insert direct (possibly conditional) return instructions on such targets that support this, and fails on other targets.
>
> With your patch, the routine will now *always* fail when trying to redirect a branch to the exit block. This is not a correctness bug, but it might presumably lead to worse performance of generated code on platforms that would have supported direct returns.
>
> The immediate cause of the internal compiler error is an API change. It used to be the case that you had to pass NULL_RTX as argument to redirect_jump to indicate that the branch should be redirected to the exit block. After the shrink-wrap changes, it is *still* possible to redirect a branch to exit, but this instead indicated by passing the appropriate return insn (e.g. a ret_rtx or a simple_return_rtx).
>
> In some other places where redirect_jump used to be called with NULL_RTX, the shrink-wrap patch changed this to instead pass ret_rtx. See e.g. this change in dead_or_predictable:
>
> if (new_dest == EXIT_BLOCK_PTR)
> new_dest_label = ret_rtx;
> else
> new_dest_label = block_label (new_dest);
>
> (followed by a call to redirect_jump (... new_dest_label ...)).
>
> I would have thought we'd want a similar change in those cfgrtl.c locations. I must admit, though, that I'm not completely sure which type of return insn is actually the correct one; why is it ret_rtx (and not, say, simple_return_rtx)? Maybe Bernd can provide some additional information here ...
>

[Bernd Schmidt (bernds)]

On 05/16/2011 10:38 PM, Chung-Lin Tang wrote:
> I had such concerns too at the start, about the case where new_bb ==
> EXIT_BLOCK_PTR (and passing to redirect_jump() a NULL bb) returns true.
>
> However, Bernd explained that these call sites were all before reload,
> and no ret_rtx or simple_return_rtx is available at this stage. So the
> "new shrink-wrap style" is not applicable here.

More strictly speaking, the return and cond_return patterns are
unavailable before reload, so at least for the case that was crashing,
the attempt to generate them is pointless.

Bernd

[Chung-Lin Tang (cltang)]

On 2011/5/16 下午 07:03, Bernd Schmidt wrote:
> On 05/16/2011 10:38 PM, Chung-Lin Tang wrote:
>> I had such concerns too at the start, about the case where new_bb ==
>> EXIT_BLOCK_PTR (and passing to redirect_jump() a NULL bb) returns true.
>>
>> However, Bernd explained that these call sites were all before reload,
>> and no ret_rtx or simple_return_rtx is available at this stage. So the
>> "new shrink-wrap style" is not applicable here.
>
> More strictly speaking, the return and cond_return patterns are
> unavailable before reload, so at least for the case that was crashing,
> the attempt to generate them is pointless.
>
>
> Bernd

So Bernd, does reverting my current patch (it is already committed to CS
branches) and instead changing the redirect_jump() assertion to only
trigger after reload_completed make sense? I am beginning to think that
should be better...

Chung-Lin

[Bernd Schmidt (bernds)]

On 05/16/2011 11:29 PM, Chung-Lin Tang wrote:
> On 2011/5/16 下午 07:03, Bernd Schmidt wrote:
>> On 05/16/2011 10:38 PM, Chung-Lin Tang wrote:
>>> I had such concerns too at the start, about the case where new_bb ==
>>> EXIT_BLOCK_PTR (and passing to redirect_jump() a NULL bb) returns true.
>>>
>>> However, Bernd explained that these call sites were all before reload,
>>> and no ret_rtx or simple_return_rtx is available at this stage. So the
>>> "new shrink-wrap style" is not applicable here.
>>
>> More strictly speaking, the return and cond_return patterns are
>> unavailable before reload, so at least for the case that was crashing,
>> the attempt to generate them is pointless.
>>
>>
>> Bernd
>
> So Bernd, does reverting my current patch (it is already committed to CS
> branches) and instead changing the redirect_jump() assertion to only
> trigger after reload_completed make sense? I am beginning to think that
> should be better...

No, because from the target == EXIT_BLOCK_PTR condition we can't tell
whether we want a return or a simple_return. You could change the
function to also accept the new jump label as an argument and modify the
callers, but I don't know whether that's really worthwhile.

Bernd

[Chung-Lin Tang (cltang)]

On 2011/5/16 07:35 PM, Bernd Schmidt wrote:
> On 05/16/2011 11:29 PM, Chung-Lin Tang wrote:
>> On 2011/5/16 下午 07:03, Bernd Schmidt wrote:
>>> On 05/16/2011 10:38 PM, Chung-Lin Tang wrote:
>>>> I had such concerns too at the start, about the case where new_bb ==
>>>> EXIT_BLOCK_PTR (and passing to redirect_jump() a NULL bb) returns true.
>>>>
>>>> However, Bernd explained that these call sites were all before reload,
>>>> and no ret_rtx or simple_return_rtx is available at this stage. So the
>>>> "new shrink-wrap style" is not applicable here.
>>>
>>> More strictly speaking, the return and cond_return patterns are
>>> unavailable before reload, so at least for the case that was crashing,
>>> the attempt to generate them is pointless.
>>>
>>>
>>> Bernd
>>
>> So Bernd, does reverting my current patch (it is already committed to CS
>> branches) and instead changing the redirect_jump() assertion to only
>> trigger after reload_completed make sense? I am beginning to think that
>> should be better...
>
> No, because from the target == EXIT_BLOCK_PTR condition we can't tell
> whether we want a return or a simple_return. You could change the
> function to also accept the new jump label as an argument and modify the
> callers, but I don't know whether that's really worthwhile.

I might be misunderstanding something here, as I'm a little unclear
about this part, but from the context above, I would think that this
return/simple_return determination is not needed before reload? (or
before prologue/epilogue construction?)

[Bernd Schmidt (bernds)]

On 05/16/2011 11:40 PM, Chung-Lin Tang wrote:
> I might be misunderstanding something here, as I'm a little unclear
> about this part, but from the context above, I would think that this
> return/simple_return determination is not needed before reload? (or
> before prologue/epilogue construction?)

It's not needed in the sense that you shouldn't try (and e.g. the ARM
port rejects attempts) to construct a return or a simple_return pattern
before prologue/epilogue generation.

Bernd

[Ulrich Weigand (uweigand)]

> On 05/16/2011 11:40 PM, Chung-Lin Tang wrote:
> > I might be misunderstanding something here, as I'm a little unclear
> > about this part, but from the context above, I would think that this
> > return/simple_return determination is not needed before reload? (or
> > before prologue/epilogue construction?)
>
> It's not needed in the sense that you shouldn't try (and e.g. the ARM
> port rejects attempts) to construct a return or a simple_return pattern
> before prologue/epilogue generation.

I agree that before prologue/epilogue generation, it is useless to try
constructing a return pattern via redirect_jump.

In fact, it seems that most cases of direct/conditional return
instructions are created *during* prologue/epilogue generation itself,
in thread_prologue_and_epilogue_insns, which was adapted for the
shrink-wrap case.

So the only remaining cases are those where the pre-shrink-wrap code
created a direct/conditional return, but only *after* prologue/
epilogue generation (e.g. via a redirect_jump during post-reload
if-conversion or scheduling).

I don't have a good feeling how frequent that case ever occurred, and
whether it would be worthwhile to fix. However, since those cases would
have caused an internal compiler error with the shrink-wrap compiler,
they do seem to be quite rare or else we'd have noticed them ...

So, given this discussion, my call would be to accept the patch as-is
for the Linaro GCC 4.5 compiler. Once Bernd brings the shrink-wrap
patch upstream, I'm assuming there will be review discussions on that
point as well -- and if in the end some other solution to this problem
goes upstream, we can always go back and update our branch as well.

Original patch is OK for Linaro GCC 4.5. (As discussed in our call,
please apply only after Andrew has created the release.)

review: Approve

[Chung-Lin Tang (cltang)]

On 2011/5/16 下午 08:33, Ulrich Weigand wrote:
> Review: Approve
>> On 05/16/2011 11:40 PM, Chung-Lin Tang wrote:
>>> I might be misunderstanding something here, as I'm a little unclear
>>> about this part, but from the context above, I would think that this
>>> return/simple_return determination is not needed before reload? (or
>>> before prologue/epilogue construction?)
>>
>> It's not needed in the sense that you shouldn't try (and e.g. the ARM
>> port rejects attempts) to construct a return or a simple_return pattern
>> before prologue/epilogue generation.
>
> I agree that before prologue/epilogue generation, it is useless to try
> constructing a return pattern via redirect_jump.
>
> In fact, it seems that most cases of direct/conditional return
> instructions are created *during* prologue/epilogue generation itself,
> in thread_prologue_and_epilogue_insns, which was adapted for the
> shrink-wrap case.
>
> So the only remaining cases are those where the pre-shrink-wrap code
> created a direct/conditional return, but only *after* prologue/
> epilogue generation (e.g. via a redirect_jump during post-reload
> if-conversion or scheduling).
>
> I don't have a good feeling how frequent that case ever occurred, and
> whether it would be worthwhile to fix. However, since those cases would
> have caused an internal compiler error with the shrink-wrap compiler,
> they do seem to be quite rare or else we'd have noticed them ...
>
> So, given this discussion, my call would be to accept the patch as-is
> for the Linaro GCC 4.5 compiler. Once Bernd brings the shrink-wrap
> patch upstream, I'm assuming there will be review discussions on that
> point as well -- and if in the end some other solution to this problem
> goes upstream, we can always go back and update our branch as well.
>
> Original patch is OK for Linaro GCC 4.5. (As discussed in our call,
> please apply only after Andrew has created the release.)
>

I wasn't on the call. However this is an ICE case, are you sure you want
to apply it one release later?

Chung-Lin

[Ulrich Weigand (uweigand)]

> On 2011/5/16 下午 08:33, Ulrich Weigand wrote:
> > Original patch is OK for Linaro GCC 4.5. (As discussed in our call,
> > please apply only after Andrew has created the release.)
>
> I wasn't on the call. However this is an ICE case, are you sure you want
> to apply it one release later?

Well, in the call Michael said he didn't want to see any additional patches for this release.

However, I think we were under the impression that this patch is shrink-wrap related and therefore not release-critical ... but it seems actually the ICE happens even if shrink-wrap is disabled, so I guess it would be good to have.

Michael/Andrew -- this is your call: should this patch still go into this week's release?

[Linaro Toolchain Builder (cbuild)]

cbuild successfully built this on x86_64-maverick-cbuild114-crucis-x86_64r1.

The build results are available at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.5+bzr99510~cltang~lp-748138-cfgrtl-fix-4_5/logs/x86_64-maverick-cbuild114-crucis-x86_64r1

The test suite results changed compared to the branch point lp:gcc-linaro+bzr99509:
 -FAIL: gcc.c-torture/compile/limits-structnest.c -O2 -flto (test for excess errors)
 -FAIL: gcc.c-torture/compile/limits-structnest.c -O2 -fwhopr (test for excess errors)
 -FAIL: gcc.c-torture/compile/limits-structnest.c -O2 (test for excess errors)
 -FAIL: gcc.c-torture/compile/limits-structnest.c -O3 -fomit-frame-pointer (test for excess errors)
 -FAIL: gcc.c-torture/compile/limits-structnest.c -O3 -g (test for excess errors)
 -FAIL: gcc.c-torture/compile/limits-structnest.c -Os (test for excess errors)
 +PASS: gcc.c-torture/compile/limits-structnest.c -O2 -flto (test for excess errors)
 +PASS: gcc.c-torture/compile/limits-structnest.c -O2 -fwhopr (test for excess errors)
 +PASS: gcc.c-torture/compile/limits-structnest.c -O2 (test for excess errors)
 +PASS: gcc.c-torture/compile/limits-structnest.c -O3 -fomit-frame-pointer (test for excess errors)
 +PASS: gcc.c-torture/compile/limits-structnest.c -O3 -g (test for excess errors)
 +PASS: gcc.c-torture/compile/limits-structnest.c -Os (test for excess errors)

The full testsuite results are at:
 http://ex.seabright.co.nz/build/gcc-linaro-4.5+bzr99510~cltang~lp-748138-cfgrtl-fix-4_5/logs/x86_64-maverick-cbuild114-crucis-x86_64r1/gcc-testsuite.txt

cbuild-checked: x86_64-maverick-cbuild114-crucis-x86_64r1

=== modified file 'ChangeLog.linaro'
--- ChangeLog.linaro	2011-05-11 15:51:33 +0000
+++ ChangeLog.linaro	2011-05-12 09:17:24 +0000
@@ -1,3 +1,12 @@
[phone]  Chung-Lin Tang  <[e-mail]>
+
+	LP:748138
+
+	gcc/
+	* cfgrtl.c (try_redirect_by_replacing_jump): Treat EXIT_BLOCK_PTR case
+	separately before call to redirect_jump(). Add assertion.
+	(patch_jump_insn): Same.
+
 2011-05-11  Ramana Radhakrishnan  <[e-mail]>
 
 	Backport from mainline

=== modified file 'gcc/cfgrtl.c'
--- gcc/cfgrtl.c	2011-02-08 10:51:58 +0000
+++ gcc/cfgrtl.c	2011-05-12 09:17:24 +0000
@@ -835,11 +835,10 @@
       if (dump_file)
 	fprintf (dump_file, "Redirecting jump %i from %i to %i.\n",
 		 INSN_UID (insn), e->dest->index, target->index);
-      if (!redirect_jump (insn, block_label (target), 0))
-	{
-	  gcc_assert (target == EXIT_BLOCK_PTR);
-	  return NULL;
-	}
+      if (target == EXIT_BLOCK_PTR)
+	return NULL;
+      if (! redirect_jump (insn, block_label (target), 0))
+	gcc_unreachable ();
     }
 
   /* Cannot do anything for target exit block.  */
@@ -1019,11 +1018,10 @@
 	  /* If the substitution doesn't succeed, die.  This can happen
 	     if the back end emitted unrecognizable instructions or if
 	     target is exit block on some arches.  */
-	  if (!redirect_jump (insn, block_label (new_bb), 0))
-	    {
-	      gcc_assert (new_bb == EXIT_BLOCK_PTR);
-	      return false;
-	    }
+	  if (new_bb == EXIT_BLOCK_PTR)
+	    return false;
+	  if (! redirect_jump (insn, block_label (new_bb), 0))
+	    gcc_unreachable ();
 	}
     }
   return true;