[PR,middle-end/59127] Fix Ada bootstrap on x86_64-unknown-linux-gnu

This patch fixes two issues, the most important issue is the related to 
the Ada build failures on the trunk.

When non-call-exceptions is on, most memory references potentially 
throw.  As a result those statements end basic blocks.  This causes
checking failures when the __builtin_trap is placed immediately after 
the memory reference because we find the memory reference in the middle 
of a basic block.

While I think we could support this with some more work, it just doesn't 
seem worth the effort.  It certainly isn't something that's occurring 
with any regularity AFAICT when buliding the Ada compiler/runtime system.

It's easiest to just disallow optimization when the statement that 
triggers undefined behaviour ends a block -- with the exception of 
GIMPLE_RETURN.  That captures the key issue, namely that the code is not 
currently prepared to have the trap in a separate block from the 
statement triggering undefined behaviour.

I didn't make a testcase for this failure because it triggers during 
bootstrapping Ada and my Ada-fu has severely eroded since the 80s when I 
was forced to learn Ada.

The second issue is when a block has outgoing abnormal edges, out of an 
abundance of caution, we should simply not apply the optimization.  That 
may be a bit too cautious, but it's clearly the safe thing to do.  I 
don't have a testcase for this.

In addition to the usual bootstrap & regression test on 
x86_64-unknown-linux-gnu, this patch fixes the Ada bootstrap on 
x86_64-unknown-linux-gnu and shows no regressions in the Ada test suite 
when compared to a compiler with the optimization totally disabled.  ie, 
a poor mans regression test for Ada given that Ada wasn't bootstrapping 
w/o this patch.

Applied to the trunk.

Jeff

ps.  Obviously the next thing to verify is that Ada is bootstrapping on 
Itanic, but there's other potential issues on Itanic that might get in 
the way.
* basic-block.h (has_abnormal_outgoing_edge_p): Moved here from...
	* tree-inline.c (has_abnormal_outgoing_edge_p): Remove.
	* gimple-ssa-isolate-paths.c: Include tree-cfg.h.
	(find_implicit_erroneous_behaviour): If a block has abnormal outgoing
	edges, then ignore it.  If the statement exhibiting erroneous
	behaviour ends basic blocks, with the exception of GIMPLE_RETURNs,
	then we can not optimize.
	(find_explicit_erroneous_behaviour): Likewise.

On Thu, Nov 14, 2013 at 9:58 PM, Jeff Law <law@redhat.com> wrote:
>
> This patch fixes two issues, the most important issue is the related to the
> Ada build failures on the trunk.
>
> When non-call-exceptions is on, most memory references potentially throw.
> As a result those statements end basic blocks.  This causes
> checking failures when the __builtin_trap is placed immediately after the
> memory reference because we find the memory reference in the middle of a
> basic block.
>
> While I think we could support this with some more work, it just doesn't
> seem worth the effort.  It certainly isn't something that's occurring with
> any regularity AFAICT when buliding the Ada compiler/runtime system.
>
> It's easiest to just disallow optimization when the statement that triggers
> undefined behaviour ends a block -- with the exception of GIMPLE_RETURN.

Hmm, don't you simply want to look for abnormal or eh outgoing edges?
That catches all stmt-ends-bb and doesn't catch GIMPLE_RETURN.

Richard.

> That captures the key issue, namely that the code is not currently prepared
> to have the trap in a separate block from the statement triggering undefined
> behaviour.
>
> I didn't make a testcase for this failure because it triggers during
> bootstrapping Ada and my Ada-fu has severely eroded since the 80s when I was
> forced to learn Ada.
>
> The second issue is when a block has outgoing abnormal edges, out of an
> abundance of caution, we should simply not apply the optimization.  That may
> be a bit too cautious, but it's clearly the safe thing to do.  I don't have
> a testcase for this.
>
> In addition to the usual bootstrap & regression test on
> x86_64-unknown-linux-gnu, this patch fixes the Ada bootstrap on
> x86_64-unknown-linux-gnu and shows no regressions in the Ada test suite when
> compared to a compiler with the optimization totally disabled.  ie, a poor
> mans regression test for Ada given that Ada wasn't bootstrapping w/o this
> patch.
>
> Applied to the trunk.
>
> Jeff
>
> ps.  Obviously the next thing to verify is that Ada is bootstrapping on
> Itanic, but there's other potential issues on Itanic that might get in the
> way.
>
>
>         * basic-block.h (has_abnormal_outgoing_edge_p): Moved here from...
>         * tree-inline.c (has_abnormal_outgoing_edge_p): Remove.
>         * gimple-ssa-isolate-paths.c: Include tree-cfg.h.
>         (find_implicit_erroneous_behaviour): If a block has abnormal
> outgoing
>         edges, then ignore it.  If the statement exhibiting erroneous
>         behaviour ends basic blocks, with the exception of GIMPLE_RETURNs,
>         then we can not optimize.
>         (find_explicit_erroneous_behaviour): Likewise.
>
>
> diff --git a/gcc/basic-block.h b/gcc/basic-block.h
> index 9c28f14..b7e3b50 100644
> --- a/gcc/basic-block.h
> +++ b/gcc/basic-block.h
> @@ -1008,4 +1008,19 @@ inverse_probability (int prob1)
>    check_probability (prob1);
>    return REG_BR_PROB_BASE - prob1;
>  }
> +
> +/* Return true if BB has at least one abnormal outgoing edge.  */
> +
> +static inline bool
> +has_abnormal_outgoing_edge_p (basic_block bb)
> +{
> +  edge e;
> +  edge_iterator ei;
> +
> +  FOR_EACH_EDGE (e, ei, bb->succs)
> +    if (e->flags & EDGE_ABNORMAL)
> +      return true;
> +
> +  return false;
> +}
>  #endif /* GCC_BASIC_BLOCK_H */
> diff --git a/gcc/gimple-ssa-isolate-paths.c b/gcc/gimple-ssa-isolate-paths.c
> index 108b98e..66c13f4 100644
> --- a/gcc/gimple-ssa-isolate-paths.c
> +++ b/gcc/gimple-ssa-isolate-paths.c
> @@ -37,6 +37,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "ssa-iterators.h"
>  #include "cfgloop.h"
>  #include "tree-pass.h"
> +#include "tree-cfg.h"
>
>
>  static bool cfg_altered;
> @@ -215,6 +216,17 @@ find_implicit_erroneous_behaviour (void)
>      {
>        gimple_stmt_iterator si;
>
> +      /* Out of an abundance of caution, do not isolate paths to a
> +        block where the block has any abnormal outgoing edges.
> +
> +        We might be able to relax this in the future.  We have to detect
> +        when we have to split the block with the NULL dereference and
> +        the trap we insert.  We have to preserve abnormal edges out
> +        of the isolated block which in turn means updating PHIs at
> +        the targets of those abnormal outgoing edges.  */
> +      if (has_abnormal_outgoing_edge_p (bb))
> +       continue;
> +
>        /* First look for a PHI which sets a pointer to NULL and which
>          is then dereferenced within BB.  This is somewhat overly
>          conservative, but probably catches most of the interesting
> @@ -256,8 +268,15 @@ find_implicit_erroneous_behaviour (void)
>                 {
>                   /* We only care about uses in BB.  Catching cases in
>                      in other blocks would require more complex path
> -                    isolation code.  */
> -                 if (gimple_bb (use_stmt) != bb)
> +                    isolation code.
> +
> +                    If the statement must end a block and is not a
> +                    GIMPLE_RETURN, then additional work would be
> +                    necessary to isolate the path.  Just punt it for
> +                    now.  */
> +                 if (gimple_bb (use_stmt) != bb
> +                     || (stmt_ends_bb_p (use_stmt)
> +                         && gimple_code (use_stmt) != GIMPLE_RETURN))
>                     continue;
>
>                   if (infer_nonnull_range (use_stmt, lhs))
> @@ -289,6 +308,17 @@ find_explicit_erroneous_behaviour (void)
>      {
>        gimple_stmt_iterator si;
>
> +      /* Out of an abundance of caution, do not isolate paths to a
> +        block where the block has any abnormal outgoing edges.
> +
> +        We might be able to relax this in the future.  We have to detect
> +        when we have to split the block with the NULL dereference and
> +        the trap we insert.  We have to preserve abnormal edges out
> +        of the isolated block which in turn means updating PHIs at
> +        the targets of those abnormal outgoing edges.  */
> +      if (has_abnormal_outgoing_edge_p (bb))
> +       continue;
> +
>        /* Now look at the statements in the block and see if any of
>          them explicitly dereference a NULL pointer.  This happens
>          because of jump threading and constant propagation.  */
> @@ -299,7 +329,8 @@ find_explicit_erroneous_behaviour (void)
>           /* By passing null_pointer_node, we can use infer_nonnull_range
>              to detect explicit NULL pointer dereferences and other uses
>              where a non-NULL value is required.  */
> -         if (infer_nonnull_range (stmt, null_pointer_node))
> +         if ((!stmt_ends_bb_p (stmt) || gimple_code (stmt) ==
> GIMPLE_RETURN)
> +             && infer_nonnull_range (stmt, null_pointer_node))
>             {
>               insert_trap_and_remove_trailing_statements (&si,
>
> null_pointer_node);
> diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> index fe5c0cb..08c4541 100644
> --- a/gcc/tree-inline.c
> +++ b/gcc/tree-inline.c
> @@ -4504,21 +4504,6 @@ fold_marked_statements (int first, struct
> pointer_set_t *statements)
>        }
>  }
>
> -/* Return true if BB has at least one abnormal outgoing edge.  */
> -
> -static inline bool
> -has_abnormal_outgoing_edge_p (basic_block bb)
> -{
> -  edge e;
> -  edge_iterator ei;
> -
> -  FOR_EACH_EDGE (e, ei, bb->succs)
> -    if (e->flags & EDGE_ABNORMAL)
> -      return true;
> -
> -  return false;
> -}
> -
>  /* Expand calls to inline functions in the body of FN.  */
>
>  unsigned int
>

On 11/15/13 02:36, Richard Biener wrote:
> On Thu, Nov 14, 2013 at 9:58 PM, Jeff Law <law@redhat.com> wrote:
>>
>> This patch fixes two issues, the most important issue is the related to the
>> Ada build failures on the trunk.
>>
>> When non-call-exceptions is on, most memory references potentially throw.
>> As a result those statements end basic blocks.  This causes
>> checking failures when the __builtin_trap is placed immediately after the
>> memory reference because we find the memory reference in the middle of a
>> basic block.
>>
>> While I think we could support this with some more work, it just doesn't
>> seem worth the effort.  It certainly isn't something that's occurring with
>> any regularity AFAICT when buliding the Ada compiler/runtime system.
>>
>> It's easiest to just disallow optimization when the statement that triggers
>> undefined behaviour ends a block -- with the exception of GIMPLE_RETURN.
>
> Hmm, don't you simply want to look for abnormal or eh outgoing edges?
> That catches all stmt-ends-bb and doesn't catch GIMPLE_RETURN.
Yea, that's better.  I've got a patch spinning now.

jeff