[PATCH][LTO] Re-set boolean_type_node in free_lang_data

[PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Richard Guenther]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

The following fixes ICEs in convert_move that happen because Fortrans idea of
boolean_type_node doesn't match up with lto1s.  It's a hack added alongside
the other hacks, and shouldn't change anything for C/C++.

But I'll take this as a chance to bootstrap lto bootstrapping/testing.  Well, as
soon as I can get back to a fast machine.

Ok for lto if that works?

Thanks,
Richard.

2009-07-10  Richard Guenther  <rguenther@suse.de>

        * tree.c (free_lang_data): Re-set boolean_type_node to what
        it will be for lto1.

[-- Attachment #2: fix-lto-fortran-4 --]
[-- Type: application/octet-stream, Size: 1336 bytes --]

2009-07-10  Richard Guenther  <rguenther@suse.de>

	* tree.c (free_lang_data): Re-set boolean_type_node to what
	it will be for lto1.

Index: gcc/tree.c
===================================================================
--- gcc/tree.c	(revision 149476)
+++ gcc/tree.c	(working copy)
@@ -4744,6 +4744,22 @@ free_lang_data (void)
      a variant copy of ptr_type_node for front-end purposes.  */
   fileptr_type_node = ptr_type_node;
 
+  /* FIXME lto.  This is a hack.  boolean_type_node is set to
+     a frontend-specific mode by Fortran and Java at least.  */
+  if (TREE_CODE (boolean_type_node) != BOOLEAN_TYPE
+      || (TYPE_MODE (boolean_type_node)
+	  != mode_for_size (BOOL_TYPE_SIZE, MODE_INT, 0))
+      || TYPE_PRECISION (boolean_type_node) != 1
+      || !TYPE_UNSIGNED (boolean_type_node))
+    {
+      boolean_type_node = make_unsigned_type (BOOL_TYPE_SIZE);
+      TREE_SET_CODE (boolean_type_node, BOOLEAN_TYPE);
+      TYPE_MAX_VALUE (boolean_type_node) = build_int_cst (boolean_type_node, 1);
+      TYPE_PRECISION (boolean_type_node) = 1;
+      boolean_false_node = TYPE_MIN_VALUE (boolean_type_node);
+      boolean_true_node = TYPE_MAX_VALUE (boolean_type_node);
+    }
+
   /* Reset some langhooks.  */
   lang_hooks.callgraph.analyze_expr = NULL;
   lang_hooks.types_compatible_p = gimple_types_compatible_p;

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Diego Novillo]

On Fri, Jul 10, 2009 at 15:25, Richard
Guenther<richard.guenther@gmail.com> wrote:
> The following fixes ICEs in convert_move that happen because Fortrans idea of
> boolean_type_node doesn't match up with lto1s.  It's a hack added alongside
> the other hacks, and shouldn't change anything for C/C++.

Note that at some point we'll want to make these "hacks" into
agreed-upon changes needed to transition from the FE type system into
gimple's.  In some cases it will be a simple validation on the change
(as in, "yes, this is how it's supposed to transition").


> Ok for lto if that works?

Yes, thanks.


Diego.

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Eric Botcazou]

> 2009-07-10  Richard Guenther  <rguenther@suse.de>
>
>         * tree.c (free_lang_data): Re-set boolean_type_node to what
>         it will be for lto1.

This silently got merged at some point with the FIXME removed.  This will 
break Ada in subtle ways in non-LTO mode; I suspect Java is also affected 
when BOOL_TYPE_SIZE is not 8.  Can't this be done properly in lto1?

-- 
Eric Botcazou

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Richard Guenther]

On Mon, Oct 12, 2009 at 12:00 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> 2009-07-10  Richard Guenther  <rguenther@suse.de>
>>
>>         * tree.c (free_lang_data): Re-set boolean_type_node to what
>>         it will be for lto1.
>
> This silently got merged at some point with the FIXME removed.  This will
> break Ada in subtle ways in non-LTO mode; I suspect Java is also affected
> when BOOL_TYPE_SIZE is not 8.  Can't this be done properly in lto1?

It is done properly in lto1.  Can you expand on why this will break
Ada in subtle ways?  Note that after free_lang_data the frontend
isn't involved in anything anymore.

Richard.

> --
> Eric Botcazou
>

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Eric Botcazou]

> It is done properly in lto1.

Then why doing it in non-LTO mode as well?

> Can you expand on why this will break Ada in subtle ways?  Note that after
> free_lang_data the frontend isn't involved in anything anymore.

This changes the precision of boolean_type_node, which is 8 in Ada with the 
corresponding TYPE_MAX_VALUE because of invalid values so that tests like

  if (x > 1)

are not optimized away.

-- 
Eric Botcazou

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Richard Guenther]

On Mon, Oct 12, 2009 at 12:30 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> It is done properly in lto1.
>
> Then why doing it in non-LTO mode as well?

Because the LTO streamer pre-loads its cache with common
tree nodes (including boolean_type_node).  The change is
to make those match up, otherwise you'll stream out a
different boolean_type_node than you stream in, generating
wrong-code (thus, I would have guessed this fix would _fix_
Ada instead of breaking it).

>> Can you expand on why this will break Ada in subtle ways?  Note that after
>> free_lang_data the frontend isn't involved in anything anymore.
>
> This changes the precision of boolean_type_node, which is 8 in Ada with the
> corresponding TYPE_MAX_VALUE because of invalid values so that tests like
>
>  if (x > 1)
>
> are not optimized away.

But the change causes the boolean type the Ada frontend uses
to be streamed out regularly, not merged with random boolean
types from either the middle-end or other language frontends.

Thus, if the frontend has generated if (x > 1) with x of type
boolean_type_node with precision 8 then after streaming the
stmt back in x will still have a boolean type of precision 8.
It just won't match up pointer-wise with what is boolean_type_node.

Richard.

>
> --
> Eric Botcazou
>

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Eric Botcazou]

> But the change causes the boolean type the Ada frontend uses
> to be streamed out regularly, not merged with random boolean
> types from either the middle-end or other language frontends.

But free_lang_data is called in non-LTO mode and boolean_type_node is changed 
behind the back of the compiler in this mode.  Note that this is not done for 
sizetype, which suffers from the same discrepancy, the breakage would be much 
more apparent otherwise.

-- 
Eric Botcazo0

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Richard Guenther]

On Mon, Oct 12, 2009 at 1:59 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> But the change causes the boolean type the Ada frontend uses
>> to be streamed out regularly, not merged with random boolean
>> types from either the middle-end or other language frontends.
>
> But free_lang_data is called in non-LTO mode and boolean_type_node is changed
> behind the back of the compiler in this mode.  Note that this is not done for
> sizetype, which suffers from the same discrepancy, the breakage would be much
> more apparent otherwise.

Even without LTO the frontend is supposed to be finished completely
if free-lang-data is called.  Where do we get back to the Ada FE?

And indeed, with Ada we'd need to fixup sizetype the same way
(though it'll get more interesting here because of
POINTER_PLUS_EXPR and verification and the awful choice
of sizetype for the offset).

Richard.

> --
> Eric Botcazo0
>

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Eric Botcazou]

> Even without LTO the frontend is supposed to be finished completely
> if free-lang-data is called.  Where do we get back to the Ada FE?

We don't, just do a 'grep boolean_type_node' and you'll see that optimizers 
will happily introduce it again.

> And indeed, with Ada we'd need to fixup sizetype the same way
> (though it'll get more interesting here because of
> POINTER_PLUS_EXPR and verification and the awful choice
> of sizetype for the offset).

Yes, LTO is totally broken for Ada at the moment and will require some serious 
work on the Ada side.  That's OK.  What is definitely not OK is to break Ada 
in non-LTO mode.

-- 
Eric Botcazou

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Richard Guenther]

On Tue, Oct 13, 2009 at 8:02 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Even without LTO the frontend is supposed to be finished completely
>> if free-lang-data is called.  Where do we get back to the Ada FE?
>
> We don't, just do a 'grep boolean_type_node' and you'll see that optimizers
> will happily introduce it again.

Sure, but that's a different boolean_type_node then.  And I don't
see a problem with that - do you?  Remember the original
boolean_type_nodes do not change.

>> And indeed, with Ada we'd need to fixup sizetype the same way
>> (though it'll get more interesting here because of
>> POINTER_PLUS_EXPR and verification and the awful choice
>> of sizetype for the offset).
>
> Yes, LTO is totally broken for Ada at the moment and will require some serious
> work on the Ada side.  That's OK.  What is definitely not OK is to break Ada
> in non-LTO mode.

I'm not convinced there is any breakage.  I guess you do not
have a testcase as otherwise you'd have shown it already, no?

Thanks,
Richard.

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Eric Botcazou]

> Sure, but that's a different boolean_type_node then.  And I don't
> see a problem with that - do you?

Yes, I do, I don't see what will prevent an optimizer from rewriting a boolean 
expression, say a condition, originally in 8-bit boolean, into an equivalent 
boolean expression in 1-bit boolean; this will change the semantics of the 
code, admittedly in rare corner cases but still.

> I'm not convinced there is any breakage.  I guess you do not
> have a testcase as otherwise you'd have shown it already, no?

Right, I haven't found any yet.  But I nevertheless think that the hack should 
be eliminated sooner than later.

-- 
Eric Botcazou

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Richard Guenther]

On Wed, Oct 14, 2009 at 1:53 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Sure, but that's a different boolean_type_node then.  And I don't
>> see a problem with that - do you?
>
> Yes, I do, I don't see what will prevent an optimizer from rewriting a boolean
> expression, say a condition, originally in 8-bit boolean, into an equivalent
> boolean expression in 1-bit boolean; this will change the semantics of the
> code, admittedly in rare corner cases but still.

Well, that would be a bug in the optimizer.  Optimizers cannot simply
change types of variables.  What optimizers do is if they build new
tcc_comparison expressions, simply use boolean_type_node as the
result type.  Which will be ok.  If that is assigned to an existing
formerly-boolean-type-node
typed variable the type verifier will complain unless the optimizer adds
a conversion.

>> I'm not convinced there is any breakage.  I guess you do not
>> have a testcase as otherwise you'd have shown it already, no?
>
> Right, I haven't found any yet.  But I nevertheless think that the hack should
> be eliminated sooner than later.

It's not a hack!  It's by design - and even correct!  Even the sizetype
case is correct - just the middle-end cannot deal at all with two
different sizetypes because of the awkward POINTER_PLUS_EXPR choice
(we simply could make it take either sizetype or ssizetype though).

Richard.

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Eric Botcazou]

> It's not a hack!  It's by design - and even correct!  Even the sizetype
> case is correct - just the middle-end cannot deal at all with two
> different sizetypes because of the awkward POINTER_PLUS_EXPR choice
> (we simply could make it take either sizetype or ssizetype though).

"Hack" is not my wording, it's originally yours, see the patch.  And I agree, 
there should be absolutely no reasons to change builtin types behind the back 
of the compiler in non-LTO mode.  Please conditionalize this on LTO mode.

-- 
Eric Botcazou

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Richard Guenther]

On Wed, Oct 14, 2009 at 10:56 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> It's not a hack!  It's by design - and even correct!  Even the sizetype
>> case is correct - just the middle-end cannot deal at all with two
>> different sizetypes because of the awkward POINTER_PLUS_EXPR choice
>> (we simply could make it take either sizetype or ssizetype though).
>
> "Hack" is not my wording, it's originally yours, see the patch.  And I agree,

Well, so I changed my mind.

> there should be absolutely no reasons to change builtin types behind the back
> of the compiler in non-LTO mode.  Please conditionalize this on LTO mode.

The point is this is not changing types behind the back of the compiler.  It is
switching the compiler from frontend to middle-end.  All the global_trees[] and
integer_types[] are sort-of frontend specific.  The middle-end shouldn't ever
add new uses of them, but instead use middle-end specific types.

Now, at the moment we enforce this only for those type nodes we know are
not compatible between languages - in which case we need to define
what is the middle-end notion of those types.

Richard.

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Eric Botcazou]

> Well, so I changed my mind.

OK, but you can understand that one can agree with the first Richard. ;-)

> The point is this is not changing types behind the back of the compiler. 
> It is switching the compiler from frontend to middle-end.  All the
> global_trees[] and integer_types[] are sort-of frontend specific.  The
> middle-end shouldn't ever add new uses of them, but instead use middle-end
> specific types.

That's a very nice plan, but not realistically doable for sizetype I'm afraid.

> Now, at the moment we enforce this only for those type nodes we know are
> not compatible between languages - in which case we need to define
> what is the middle-end notion of those types.

We just enforce it for boolean_type, we apparently never tried for sizetype so 
LTO doesn't for Ada.  I guess it's essentially for the benefit of Fortran?

I'm not sure that's the way to go; I think we should have a set of compatible 
types between LTO-ready languages, which comprises boolean_type and sizetype.
This would mean some serious work for Ada in particular, but that's OK, the 
signedness issue with sizetype is getting more and more annoying anyway.

-- 
Eric Botcazou

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Rafael Espindola]

> "Hack" is not my wording, it's originally yours, see the patch.  And I agree,
> there should be absolutely no reasons to change builtin types behind the back
> of the compiler in non-LTO mode.  Please conditionalize this on LTO mode.

Sorry for jumping in late.

We are not changing types behind the back. A FE is free to create
comparisons with a type of its choosing. So is the ME.

What is not very elegant right now is that both the FE and the ME use
the same variable to decide what type to use. I think that in a
"perfect" implementation a FE would have a variable like
java_boolean_type_node that is not visible outside of the FE.
Likewise, the type used by the ME when introducing a new comparison
would also be just an implementation detail of the ME.

Given that the "perfect" implementation would involve a lot of code
changes for a small gain, I think it is OK to just set
boolean_type_node once the FE is done.

> --
> Eric Botcazou
>


Cheers,
-- 
Rafael Ávila de Espíndola

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Eric Botcazou]

> We are not changing types behind the back. A FE is free to create
> comparisons with a type of its choosing. So is the ME.

What about the ME rewriting a comparison created by the FE into a new, 
logically equivalent comparison, changing the type in the process?

-- 
Eric Botcazou

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Richard Guenther]

On Thu, Oct 15, 2009 at 3:28 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> We are not changing types behind the back. A FE is free to create
>> comparisons with a type of its choosing. So is the ME.
>
> What about the ME rewriting a comparison created by the FE into a new,
> logically equivalent comparison, changing the type in the process?

That would be ok.  Expansion doesn't know the difference between
a comparison that results in a 8bit precision bool compared to one
that results in a 1bit precision bool.  I thought your point was that
Ada might create _variables_ of boolean_type_node that do _not_
have precision 1 and thus cannot be interpreted as variables
with precision 1.  And I say this does not happen anyway, and if
it does that would be a latent bug.

And yes, this was done for the benefit of fortran - C interoperability.
Fortran has all kinds of different precision boolean types.

Richard.

> --
> Eric Botcazou
>

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Eric Botcazou]

> And yes, this was done for the benefit of fortran - C interoperability.
> Fortran has all kinds of different precision boolean types.

I presume it's not possible/easy to set boolean_type_node to the C-compatible 
one from the start?

-- 
Eric Botcazou

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Richard Guenther]

On Thu, Oct 15, 2009 at 3:51 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> And yes, this was done for the benefit of fortran - C interoperability.
>> Fortran has all kinds of different precision boolean types.
>
> I presume it's not possible/easy to set boolean_type_node to the C-compatible
> one from the start?

You mean like Ada does?

;)

Richard.

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Eric Botcazou]

> You mean like Ada does?

Ada has only one boolean type so there isn't much leeway.  It will need to be 
changed but that's not straightforward.

But my question was serious: if Fortran has all kinds of different precision 
boolean types, why not decide that boolean_type_node is the Right One?  That 
would avoid frobbing it for other languages.

-- 
Eric Botcazou

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Rafael Espindola]

2009/10/15 Eric Botcazou <ebotcazou@adacore.com>:
>> We are not changing types behind the back. A FE is free to create
>> comparisons with a type of its choosing. So is the ME.
>
> What about the ME rewriting a comparison created by the FE into a new,
> logically equivalent comparison, changing the type in the process?

a bug in the ME? If it rewriting a comparison that produces a 128 bit
value,  the new comparison should also produce a 128 bit value.

> --
> Eric Botcazou
>

Cheers,
-- 
Rafael Ávila de Espíndola

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Diego Novillo]

On Wed, Oct 14, 2009 at 16:56, Eric Botcazou <ebotcazou@adacore.com> wrote:

> Please conditionalize this on LTO mode.

[ Rafael and Richard have already addressed the type issue, so I won't
duplicate the arguments here. ]

Notice that free_lang_data is only casually related to LTO.  The
purpose of f_l_d is to translate the front end types and symbols into
middle end counterparts.  LTO happens to rely on this step to avoid
having to understand front end specific information.

All LTO understands is gimple statements and gimple types.  As such,
making middle end type decisions conditional on "LTO mode" goes
against the intended design and means that you are papering over some
other problem.


Diego.

Re: [PATCH][LTO] Re-set boolean_type_node in free_lang_data

[Eric Botcazou]

> All LTO understands is gimple statements and gimple types.  As such,
> making middle end type decisions conditional on "LTO mode" goes
> against the intended design and means that you are papering over some
> other problem.

Yes, the problem is well known, it's the incompatibility of boolean_type and 
sizetype between languages.  The proper fix is to make them compatible from 
the start, but this requires some serious work, in particular for sizetype.
What is papering over something here is changing them in free_lang_data.

-- 
Eric Botcazou