Re: [PATCH] Fix ACATS failures in GCC 3.4

[Roger Sayle <roger at eyesopen dot com>]

Hi Eric,
> I don't think this is the correct fix (at least alone) since it only sweeps
> under the rug the problem of the dependency of the signedness of the
> comparison upon the operands.  I think that, from an Ada viewpoint,
> my fix is safer.

The type system used by "fold" has been notoriously flexible/forgiving in
the past, and bugs such as changing the type of the tree passed to fold
often have gone undetected for years.  I know this poor type safety has
been a concern for Ada for years.  However, there's been an effort to
clean much of this up, which simplifies life for the tree-ssa folks and
the poor type safety has also been a concern for the Ada folks.

The rule for MIN_EXPR and MAX_EXPR is (should be?) that the tree type of
their operands should/must match that of the operator.  However, I agree
that it is better defensive programming to also modify the RTL expanders
to consistently take the unsignedp flag from the operator's type.  Hence
if it ever encounters "suspect" trees in future, it will atleast behave
consistently.  To paraphrase Richard Henderson: "Why fix a bug only once
when it can be fixed twice?".

The sweeping under the rug argument is the wrong way around.  Only
tweaking the MIN_EXPR and MAX_EXPR RTL expanders would be papering over
the bug in the constant folder.  However, I'll preapprove a patch that
does both.

As mentioned by Richard Kenner, we should probably also remove the
RSHIFT case if there are no testsuite regressions, but this is certainly
more risky and probably not appropriate for the 3.4 branch.  If you
prefer we can leave your existing workaround on the branch, and cure
the underlying problems swapping/unsignedp/RSHIFT on mainline.


> > Of course, you're probably right that there's also an issue with
> > the MAX_EXPR and MIN_EXPR RTL expanders, and I'm now also curious
> > why the nested NOPs above aren't being folded :>
>
> The chain of NOPs is as follows:
>
> <minus_expr 0x404be900
>     type <integer_type 0x4017cbd0 long int sizetype SI
>
>  arg 0 <nop_expr 0x404c067c type <integer_type 0x4017cbd0 long int>
>
>   arg 0 <nop_expr 0x404c05dc type <integer_type 0x4017c438 integer>
>
>    <var_decl 0x404bf72c R17b
>     type <integer_type 0x404bf654 p__index___XDLU_0__100
>         type <integer_type 0x4017c438 integer SI


Ok, this looks like a separate issue (harmless bug), possibly in the Ada
front-end.  This tree only needs one NOP and should probably read just:

  arg 0 <nop_expr 0x404c067c type <integer_type 0x4017cbd0 long int>
    <var_decl 0x404bf72c R17b
      type <integer_type 0x404bf654 p__index___XDLU_0__100
        type <integer_type 0x4017c438 integer SI

As all three types are SImode, the VAR_DECL can be cast immediately to
"long int".  I looks like fold isn't being called on the (nop (nop ...))
tree.  A quick peek in ada/utils.c reveals several places that contain
"expr = build1 (NOP_EXPR, type, expr);" instead of
"expr = fold (build1 (NOP_EXPR, type, expr));", and I suspect that its
one of these that leaves the nested NOPs around.


This is completely unrelated to the ACATS failures, however, and just
results in slightly larger trees being generated by the gnat front-end.

Roger
--