Re: PR 6212

Re: PR 6212

[Richard Kenner]

Do you have the message where the nature of the miscompilation was
discussed?  Each time I look for it, I can't find it.

Thanks.

Re: PR 6212

[Mark Mitchell]

--On Sunday, May 05, 2002 02:48:21 PM -0400 Richard Kenner 
<kenner@vlsi1.ultra.nyu.edu> wrote:

> Do you have the message where the nature of the miscompilation was
> discussed?  Each time I look for it, I can't find it.

You should have asked for help earlier.

There was a link in the PR audit trail to this message:

  http://gcc.gnu.org/ml/gcc/2002-04/msg01220.html

which contains the small test case.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: PR 6212

[Neil Booth]

Richard Henderson wrote:-

> At minimum, PRs 2511, 3325, 3326, 3347, 5593.
> 
> At the heart of all, we mis-promote bit-fields.

And treat them as having the wrong range of values, since
so much of GCC assumes that is based upon the type, which
causes bogus or missed diagnostics, and, I'm sure, bad
optimizations or pessimizations.

Neil.

Re: PR 6212

[Richard Henderson]

On Wed, May 08, 2002 at 11:25:06PM -0400, Richard Kenner wrote:
> I saw the claim, but not the explanation of why.

At minimum, PRs 2511, 3325, 3326, 3347, 5593.

At the heart of all, we mis-promote bit-fields.


r~

Re: PR 6212

[Richard Kenner]

    Actually, if you'll read Neil's response elsewhere in this thread,
    you'd know that we *must* for bitfields.

I saw the claim, but not the explanation of why.

I really think this idea of making bogus types to reflect differences
between types and object is very bad.  We do this in GNAT and it's been the
source of a huge number of bugs and kludges.

Re: PR 6212

[Richard Henderson]

On Wed, May 08, 2002 at 07:00:22AM -0400, Richard Kenner wrote:
> But if you eliminate DECL_SIZE, how to you know the size of a bitfield?
> The issue is why make different types in just some cases, since we know we
> can't for bitfields.

Actually, if you'll read Neil's response elsewhere in this thread,
you'd know that we *must* for bitfields.

There is a patch pending for gcc 3.2.


r~

Re: PR 6212

[Robert Dewar]

> 
> The point is that a minimal constraint on many type systems is that two
> things with the same type support the same operations. 

Yes, exactly I agree with this entirely.

Re: PR 6212

[Mark Mitchell]

--On Wednesday, May 08, 2002 10:34:00 AM -0400 Robert Dewar 
<dewar@gnat.com> wrote:

>> The type of a pointer is -- in your average type system -- purely
>> dependent on the type pointed to by the pointer.  To say it points to
>> one kind of type, but another kind of object, is to have substantially
>> enrichened your type system in a way that is not clearly beneficial.
>
> I think you mean something like "representation of a pointer and allowable
> operations". Obviously the statement you gave is not true in any language
> with name equivalence of types. For example in Ada
>
>    type A is access Integer;
>    type B is access Integer;
>
> describe two quite different types (and indeed in Ada you cannot even
> assign one to the other, since these might be separate storage pools).
> Even if you force interconvertability
>
>    type A is access all Integer;
>    type B is access all Integer;
>
> they are still quite different types.

Right.  Ada gives you finer control over some of these situations; in
C/C++ "typedefs" do not create new types; they create new names for old
types.

The point is that a minimal constraint on many type systems is that two
things with the same type support the same operations.  (Note that this
doesn't talk about representation at all; if your language doesn't have
assignment-by-smushing-bits, representation may not matter.)  If you want
to draw finer bondaries than that, you can of course do that.  In C/C++,
representation matters greatly, so that becomes a finer distinction.  We
also don't treat "int" and "long" as the same type, even if they have the
exact same representation.  These GCC attributes essentially create new
types like "strangely-aligned, weirdly sized int" and that thing is only
a  little bit more like an int than a short is like an int.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: PR 6212

[Robert Dewar]

> The type of a pointer is -- in your average type system -- purely
> dependent on the type pointed to by the pointer.  To say it points to
> one kind of type, but another kind of object, is to have substantially
> enrichened your type system in a way that is not clearly beneficial.

I think you mean something like "representation of a pointer and allowable
operations". Obviously the statement you gave is not true in any language
with name equivalence of types. For example in Ada

   type A is access Integer;
   type B is access Integer;

describe two quite different types (and indeed in Ada you cannot even 
assign one to the other, since these might be separate storage pools).
Even if you force interconvertability

   type A is access all Integer;
   type B is access all Integer;

they are still quite different types.

Re: PR 6212

[Mark Mitchell]

--On Wednesday, May 08, 2002 07:00:22 AM -0400 Richard Kenner 
<kenner@vlsi1.ultra.nyu.edu> wrote:

>     > 3 bytes long and byte-aligned, I can make an object that's
> 4-byte-aligned     > (and hence 4 bytes long).  If I make a pointer to
> that object, that's a     > pointer to a 3-byte type, not a 4-byte type.
> I don't see how you can do
>
>     No, it's a pointer to a 4-byte type.
>
> I disagree.  It's a pointer to a 3-byte type, but a 4-byte *object*.

Well, you can call it what you like.

The type of a pointer is -- in your average type system -- purely
dependent on the type pointed to by the pointer.  To say it points to
one kind of type, but another kind of object, is to have substantially
enrichened your type system in a way that is not clearly beneficial.

This case isn't even the bad case; here you're pointing to a subtype,
and you just want to implicitly convert it to the supertype.  Fine;
I'm proposing making the type explicit, you want to forget it
immediately.

The real problem is when you have a byte-aligned int.  Now, a byte-aligned
int is a supertype of a word-aligned int; there are things you can do with
a word-aligned int you cannot do with a byte-aligned int.  In particular,
you can do an aligned load.

So, if you say a pointer to a byte-aligned int is "a pointer to an int,
but a byte-aligned int object", you're in trouble, unless you've got a
pretty fancy type calculus up your sleeve.

Assigning that sucker to an ordinary pointer to int ought to be
invalid without a cast, just as assigning a "short *" to an "int *" is.

>     > Moreover, what do you do about bitfields?  Do we now have types with
>     > sizes that are not multiples of bytes?
>
>     Bitfields, at least in C, are not addressable, so this doesn't matter.
>     The only thing you can do with a bitfield lvalue is write to it, and
>     then you know just where you're writing.  Reading from it yields a
>     promotion to the appropriate integer type.
>
> But if you eliminate DECL_SIZE, how to you know the size of a bitfield?
> The issue is why make different types in just some cases, since we know we
> can't for bitfields.

Well, it's not that I care about whether the macro exists per se.  If
we need DECL_BITFIELD_SIZE, we need DECL_BITFIELD_SIZE.  That seems
perfectly sensible to me.

I've seen my share of C/C++ front ends, and ours is the only one with
DECL_SIZE.  They all have DECL_BITFIELD_SIZE.  The non-addressability
of bit-fields is what saves you there.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: PR 6212

[Richard Kenner]

    > 3 bytes long and byte-aligned, I can make an object that's 4-byte-aligned
    > (and hence 4 bytes long).  If I make a pointer to that object, that's a
    > pointer to a 3-byte type, not a 4-byte type.  I don't see how you can do

    No, it's a pointer to a 4-byte type.  

I disagree.  It's a pointer to a 3-byte type, but a 4-byte *object*.

    > Moreover, what do you do about bitfields?  Do we now have types with
    > sizes that are not multiples of bytes?

    Bitfields, at least in C, are not addressable, so this doesn't matter.
    The only thing you can do with a bitfield lvalue is write to it, and
    then you know just where you're writing.  Reading from it yields a
    promotion to the appropriate integer type.

But if you eliminate DECL_SIZE, how to you know the size of a bitfield?
The issue is why make different types in just some cases, since we know we
can't for bitfields.

Re: PR 6212

[Neil Booth]

Richard Kenner wrote:-

> Moreover, what do you do about bitfields?  Do we now have types with sizes that
> are not multiples of bytes?

The current implementation of bitfields is quite broken in GCC; they are
given a type and value range mostly of the declared type.  They should
have a new type whose precision is the bitfield's precision.  That they
don't is the cause of at least 5 open PR's in GNATS and many nasty bugs.

Neil.

Re: PR 6212

[Mark Mitchell]

>     Using types gives you a way to provide feedback about possibly invalid
>     conversions, like converting a pointer to a byte-aligned int to a
> pointer     to a word-aligned int.  The usual type theoretic rules would
> imply     you cannot go that way, but you can go the other way.  This is
> the     whole point of a type system.
>
> I don't follow.  Objects still have types, but those types are more
> generic than the object.  In other words, if I have a record type that's

That's the point; saying that the type is more generic than the object
is what's weird here.

> 3 bytes long and byte-aligned, I can make an object that's 4-byte-aligned
> (and hence 4 bytes long).  If I make a pointer to that object, that's a
> pointer to a 3-byte type, not a 4-byte type.  I don't see how you can do

No, it's a pointer to a 4-byte type.  You may be able to implicitly
convert it to a pointer to a 3-byte type, if you like, but those two
types are different.  (The basic idea behind a type is that it tells
you what operations you can do; you can't do "copy 4 bytes" form a
3-byte object.)  The 4-byte type is a subtype of the 3-byte type;
you can do (strictly) more things with it; you can do all the 3-byte
types and then some.

> Moreover, what do you do about bitfields?  Do we now have types with
> sizes that are not multiples of bytes?

Bitfields, at least in C, are not addressable, so this doesn't matter.
The only thing you can do with a bitfield lvalue is write to it, and
then you know just where you're writing.  Reading from it yields a
promotion to the appropriate integer type.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: PR 6212

[Richard Kenner]

    This is a language question as much as an implementation question.  If
    these things have different types, then you have to decide what all the
    conversion rules are.  On the other hand, saying that two things that
    have the same type have different sizes is just plain wrong.

I must say that when we did the initial Ada implementation, we took that
approach: I wanted to be sure that the size (and alignment) of each object
was the same as its size.

That needed a tremendous amount of code and it has been the most tricky part
of the entire interface between the Ada front end and GCC.  One of my
next projects was to eliminate that entire mess and take advantage of the
separate fields in a decl.

    DECL_SIZE/DECL_ALIGN are just plain wrong, and should be eliminated.

Well, it's also DECL_SIZE_UNIT and DECL_MODE.

    Using types gives you a way to provide feedback about possibly invalid
    conversions, like converting a pointer to a byte-aligned int to a pointer
    to a word-aligned int.  The usual type theoretic rules would imply
    you cannot go that way, but you can go the other way.  This is the
    whole point of a type system.

I don't follow.  Objects still have types, but those types are more generic
than the object.  In other words, if I have a record type that's 3 bytes long
and byte-aligned, I can make an object that's 4-byte-aligned (and hence 4
bytes long).  If I make a pointer to that object, that's a pointer to a 3-byte
type, not a 4-byte type.  I don't see how you can do things consistently
if you don't take that approach.

Moreover, what do you do about bitfields?  Do we now have types with sizes that
are not multiples of bytes?

Re: PR 6212

[Mark Mitchell]

--On Tuesday, May 07, 2002 06:37:54 PM -0400 Richard Kenner 
<kenner@vlsi1.ultra.nyu.edu> wrote:

>     > Say you have an object whose type is 1-byte integer
>     > with a user-specifed alignment of 32 bits.  What do you do?
>
> 	new = build_type_copy (char_type_node);
> 	TYPE_ALIGN (new) = 32;
>
> I don't believe we support such a thing.
>
>     Why are you creating a record type?  Surely that's the pain
>     you're saying you'll have converting to and from the new type.
>
> Why create *any* extra type?  Even if the above can be made to work, why
> have the conversions?

This is a language question as much as an implementation question.  If
these things have different types, then you have to decide what all the
conversion rules are.  On the other hand, saying that two things that
have the same type have different sizes is just plain wrong.

Therefore, I agree with Richard Henderson.

Types are where this information should go.

DECL_SIZE/DECL_ALIGN are just plain wrong, and should be eliminated.

Using types gives you a way to provide feedback about possibly invalid
conversions, like converting a pointer to a byte-aligned int to a pointer
to a word-aligned int.  The usual type theoretic rules would imply
you cannot go that way, but you can go the other way.  This is the
whole point of a type system.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: PR 6212

[Richard Kenner]

    > Say you have an object whose type is 1-byte integer
    > with a user-specifed alignment of 32 bits.  What do you do?

	new = build_type_copy (char_type_node);
	TYPE_ALIGN (new) = 32;

I don't believe we support such a thing.

    Why are you creating a record type?  Surely that's the pain
    you're saying you'll have converting to and from the new type.

Why create *any* extra type?  Even if the above can be made to work, why
have the conversions?

And what about if the type is a record type and the object is that record type
with a different alignment?   You can't simply build a copy of that type
since DECL_CONTEXT will be wrong on the (now shared) fields.

Re: PR 6212

[Richard Henderson]

On Tue, May 07, 2002 at 06:20:41PM -0400, Richard Kenner wrote:
> Say you have an object whose type is 1-byte integer
> with a user-specifed alignment of 32 bits.  What do you do?

	new = build_type_copy (char_type_node);
	TYPE_ALIGN (new) = 32;

> If you say that DECL_ALIGN must equal TYPE_ALIGN, you end up constructing
> a record type containing the character as a field and have to deal with
> converting to and from it.

Why are you creating a record type?  Surely that's the pain
you're saying you'll have converting to and from the new type.


r~

Re: PR 6212

[Richard Kenner]

    > I'd like to move more toward a situation where we are allowed
    > to have these different from that of the type even more than we do now.

    Why?  I can think of no valid use for this.  It's just confusing.

Otherwise you make bogus types that end up having to be converted from 
and to and it's a pain.

Suppose you have an object with an alignment stricter than its type.
What do you do?  Say you have an object whose type is 1-byte integer
with a user-specifed alignment of 32 bits.  What do you do?  If you
say that DECL_ALIGN must equal TYPE_ALIGN, you end up constructing a
record type containing the character as a field and have to deal with
converting to and from it.

It's far simpler just to say that DECL_TYPE is char_type_node,
DECL_ALIGN and DECL_SIZE are 32, and DECL_SIZE_UNIT is 4.

Re: PR 6212

[Richard Henderson]

On Tue, May 07, 2002 at 05:59:34PM -0400, Richard Kenner wrote:
> If so, why do we have DECL_SIZE, DECL_MODE, etc.

A historic mistake.

> I'd like to move more toward a situation where we are allowed
> to have these different from that of the type even more than we do now.

Why?  I can think of no valid use for this.  It's just confusing.


r~

Re: PR 6212

[Richard Henderson]

On Tue, May 07, 2002 at 05:55:54PM -0400, Richard Kenner wrote:
> Packed fields aren't necessarily even on a byte boundary.

That doesn't negate my argument that putting this information
on the decl is wrong.

And they are on byte boundaries in C, which is where a pointer
to unaligned something would be useful on occasion.


r~

Re: PR 6212

[Richard Kenner]

    Grr.  I think this is a serious mistake.  The type should
    hold all of this information so that it would be possible
    in principal to have a pointer to a packed int.

Packed fields aren't necessarily even on a byte boundary.

    It also means my alignment patch yesterday is probably
    wrong for packed fields at the moment.

I think you could construct such a case, yes.  But it's also unlikely
because you are only specifying the alignment *of the offset*.

Re: PR 6212

[Richard Kenner]

    Grr.  I think this is a serious mistake.  The type should
    hold all of this information so that it would be possible
    in principal to have a pointer to a packed int.

Also, in general, I don't think that we should make a separate type for
a decl that has different layout.  If so, why do we have DECL_SIZE,
DECL_MODE, etc.   I'd like to move more toward a situation where we are allowed
to have these different from that of the type even more than we do now.

Re: PR 6212

[Richard Henderson]

On Tue, May 07, 2002 at 05:40:40PM -0400, Richard Kenner wrote:
> No, you are thinking of DECL_PACKED and DECL_ALIGN.  TYPE_PACKED applies
> to a record type and says the record type is packed.  The types of packed
> and unpacked fields are the same and should be.

Grr.  I think this is a serious mistake.  The type should
hold all of this information so that it would be possible
in principal to have a pointer to a packed int.

It also means my alignment patch yesterday is probably
wrong for packed fields at the moment.


r~

Re: PR 6212

[Richard Kenner]

    Err, no, that is not true.  A packed field has a type with
    TYPE_PACKED set and TYPE_ALIGN set to the minimum.

No, you are thinking of DECL_PACKED and DECL_ALIGN.  TYPE_PACKED applies
to a record type and says the record type is packed.  The types of packed
and unpacked fields are the same and should be.

Re: PR 6212

[Richard Henderson]

On Tue, May 07, 2002 at 04:40:34PM -0400, Richard Kenner wrote:
> but what's *is* true is that the alignment of the *type* of X[J} may
> be smaller than that of the *type* of Y.  Think of a bitfield or a
> packed field.

Err, no, that is not true.  A packed field has a type with
TYPE_PACKED set and TYPE_ALIGN set to the minimum.

Bitfields may be confused in general, but for most targets, the
alignment of the bitfield type still affects the alignment of
the struct.  That is

	struct S {
	  unsigned long x : 1;
	};

	struct T {
	  unsigned int x : 1;
	};

the alignments of these two structs differ.  If in fact they
are confused, this should be fixed on mainline.


r~

Re: PR 6212

[Richard Kenner]

    > Consider x[j].y.  OFFSET is J*C and BITPOS is that for Y.  TYPE here
    > is that of field Y, but OFFSET's alignment corresponds to the type
    > of X[J].

    Are you suggesting that the outer object X[J] may have
    smaller alignment than the inner object Y?

I don't see how what I'm saying implies I'm suggesting that, but what's *is*
true is that the alignment of the *type* of X[J} may be smaller than that
of the *type* of Y.  Think of a bitfield or a packed field.

Re: PR 6212

[Richard Henderson]

On Tue, May 07, 2002 at 02:02:46PM -0400, Richard Kenner wrote:
> Consider x[j].y.  OFFSET is J*C and BITPOS is that for Y.  TYPE here is
> that of field Y, but OFFSET's alignment corresponds to the type of X[J].

Are you suggesting that the outer object X[J] may have
smaller alignment than the inner object Y?

I don't know of a type system that allows that.


r~

Re: PR 6212

[Richard Kenner]

    Huh?  The type of the accessed object is always visible.  It's
    TREE_TYPE (to).

Yes, but OFFSET is not to the accessed object: you have to add BITPOS
to get there.

Consider x[j].y.  OFFSET is J*C and BITPOS is that for Y.  TYPE here is
that of field Y, but OFFSET's alignment corresponds to the type of X[J].

Re: PR 6212

[Richard Henderson]

On Tue, May 07, 2002 at 01:24:28PM -0400, Richard Kenner wrote:
> No, not at all.  But consider that, in general, you have a nesting of
> field and array references.  The alignment of the offset is that of
> the type of the array element, but you can't see that type if there is
> a an outer field reference.

Huh?  The type of the accessed object is always visible.  It's
TREE_TYPE (to).


r~

Re: PR 6212

[Richard Kenner]

    Why?  We're being given information in the form of a type
    and you're going to ignore it?

No, not at all.  But consider that, in general, you have a nesting of
field and array references.  The alignment of the offset is that of
the type of the array element, but you can't see that type if there is
a an outer field reference.

The idea of NOP_WITH_ASSERT_EXPR is that get_inner_reference can use it
to encode what it knows about alignment other than from the form of the
expression (the biggest loss now is not being able to do anything with
DECL_OFFSET_ALIGN).

My point is just that information of this nature has to be encoded in the
value returned by get_inner_reference.

Re: PR 6212

[Mark Mitchell]

--On Tuesday, May 07, 2002 08:17:13 AM -0400 Richard Kenner 
<kenner@vlsi1.ultra.nyu.edu> wrote:

>     I think the following will do the job.  It adds the alignment
>     based on the type of the dereference, not based on the form of
>     the index.
>
> That's somewhat of a change in strategy from the way it's been done,
> but I agree it's best we can do now.  For 3.2, I'd like to do all the
> computations based on expressions, not types, but this seems like a
> good compromise for now.

Why?  The C standard makes certain guarantees about alignment based on
types; if you get a stronger constraint that way then you get from
expression analysis, you should definitely use the constraint from the
type.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: PR 6212

[Richard Henderson]

On Tue, May 07, 2002 at 08:17:13AM -0400, Richard Kenner wrote:
> For 3.2, I'd like to do all the computations based on expressions,
> not types...

Why?  We're being given information in the form of a type
and you're going to ignore it?


r~

Re: PR 6212

[Richard Henderson]

On Tue, May 07, 2002 at 08:06:07AM -0400, Richard Kenner wrote:
> I'm missing something: I don't see a variable offset here, so why is
> highest_pow2_factor called at all in this case?

It isn't.  The first test case I was just guessing.


r~

Re: PR 6212

[Mark Mitchell]

--On Tuesday, May 07, 2002 08:49:00 AM -0400 Robert Dewar <dewar@gnat.com> 
wrote:

> I am confused, why can't we just back out the patch that caused this
> problem in the first place if Richard cannot figure out how to fix it?

It was very large and went in months ago.  Problems were detected almost
immediately, but they weren't tracked down fully by any of the parties
involved.

Fortunately, Richard Henderson has found what seems a good fix.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: PR 6212

[Robert Dewar]

I am confused, why can't we just back out the patch that caused this
problem in the first place if Richard cannot figure out how to fix it?

Re: PR 6212

[Richard Kenner]

    I think the following will do the job.  It adds the alignment
    based on the type of the dereference, not based on the form of
    the index.

That's somewhat of a change in strategy from the way it's been done,
but I agree it's best we can do now.  For 3.2, I'd like to do all the
computations based on expressions, not types, but this seems like a
good compromise for now.

    Bar one, the other uses of highest_pow2_factor are simple field
    offsets, and so do not need adjustment.  

You have the issue of nestings of field and array references and the
tracking of the alignments through all that.  I can't prove it, but I
think what you are doing will catch most of the common case.

    but it's for sure better to guess low than high when talking about
    alignment.

Agreed.

Re: PR 6212

[Richard Kenner]

    Kenner, you're going to have to come up with a better patch.

    Yesterday would be a good time.

I don't know of any, short of the major surgery to add the
NOP_WITH_ASSERT_EXPR, which we clearly can't do for 3.1.

Right now, what we have is a peformance issue, with no broken code.
With this patch, we have obscure cases (which had to be generated
specifically to make it fail) where the alignment is set wrong, but
where it actually doesn't matter in that particular case.

For 3.1, I think we're going to be stuck with this choice.  My
recommendation is to go ahead with the patch since the cases where it
sets the wrong alignment are far less frequent than the cases when the
current code does so.

(The only other way is to disable fold's merging of MULT and DIV, but that
will cause far worse performance problems than this bug.)

Re: PR 6212

[Richard Kenner]

    Consider

	int foo(int *p)
	{
	  return *((char *)p);
	}

	char c[2] __attribute__((aligned));

	int bar()
	{
	  return foo((int *)&c[1]);
	}

    Since we recurse all the way down to the PARM_DECL, we decide that P
    is 4-byte aligned.  We already discussed cases with pointers having
    strange values (so long as they weren't dereferenced) and declared
    them acceptable in gcc.

I'm missing something: I don't see a variable offset here, so why is
highest_pow2_factor called at all in this case?

And you are also assuming tree inlining here, right?

Re: PR 6212

[Richard Henderson]

On Mon, May 06, 2002 at 05:13:39PM -0700, Mark Mitchell wrote:
> Your patch is certainly OK if it passes.

For the record, check passed on alphaev4, alphaev6, i686.
Bootstrap complete on mips ("make chekc", doh) and
sparc-solaris2.7 (check still in progress; sun should be ashamed).


r~

Re: PR 6212

[Mark Mitchell]

--On Monday, May 06, 2002 05:11:13 PM -0700 Richard Henderson 
<rth@redhat.com> wrote:

> On Mon, May 06, 2002 at 05:01:40PM -0700, Mark Mitchell wrote:
>> Kenner, you're going to have to come up with a better patch.
>>
>> Yesterday would be a good time.
>
> I think the following will do the job.  It adds the alignment
> based on the type of the dereference, not based on the form of
> the index.
>
> Bar one, the other uses of highest_pow2_factor are simple field
> offsets, and so do not need adjustment.  The one other remaining
> talks about the size of some BLKmode thingy.  Dunno what's going
> on there, but it's for sure better to guess low than high when
> talking about alignment.
>
> I'm in the process of testing both patches on Alpha and MIPS.

Your patch is certainly OK if it passes.

Thanks!

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: PR 6212

[Richard Henderson]

On Mon, May 06, 2002 at 05:01:40PM -0700, Mark Mitchell wrote:
> Kenner, you're going to have to come up with a better patch.
> 
> Yesterday would be a good time.

I think the following will do the job.  It adds the alignment
based on the type of the dereference, not based on the form of
the index.

Bar one, the other uses of highest_pow2_factor are simple field
offsets, and so do not need adjustment.  The one other remaining
talks about the size of some BLKmode thingy.  Dunno what's going
on there, but it's for sure better to guess low than high when
talking about alignment.

I'm in the process of testing both patches on Alpha and MIPS.


r~


Index: expr.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/expr.c,v
retrieving revision 1.423.2.18
diff -c -p -d -r1.423.2.18 expr.c
*** expr.c	16 Apr 2002 06:03:36 -0000	1.423.2.18
--- expr.c	6 May 2002 23:12:44 -0000
*************** static rtx store_field		PARAMS ((rtx, HO
*** 147,152 ****
--- 147,153 ----
  					 int));
  static rtx var_rtx		PARAMS ((tree));
  static HOST_WIDE_INT highest_pow2_factor PARAMS ((tree));
+ static HOST_WIDE_INT highest_pow2_factor_for_type PARAMS ((tree, tree));
  static int is_aligning_offset	PARAMS ((tree, tree));
  static rtx expand_increment	PARAMS ((tree, int, int));
  static void do_jump_by_parts_greater PARAMS ((tree, int, rtx, rtx));
*************** expand_assignment (to, from, want_value,
*** 3707,3713 ****
  	    }
  
  	  to_rtx = offset_address (to_rtx, offset_rtx,
! 				   highest_pow2_factor (offset));
  	}
  
        if (GET_CODE (to_rtx) == MEM)
--- 3708,3715 ----
  	    }
  
  	  to_rtx = offset_address (to_rtx, offset_rtx,
! 				   highest_pow2_factor_for_type (TREE_TYPE (to),
! 								 offset));
  	}
  
        if (GET_CODE (to_rtx) == MEM)
*************** highest_pow2_factor (exp)
*** 5908,5913 ****
--- 5910,5930 ----
      }
  
    return 1;
+ }
+ 
+ /* Similar, except that it is known that the expression must be a multiple
+    of the alignment of TYPE.  */
+ 
+ static HOST_WIDE_INT
+ highest_pow2_factor_for_type (type, exp)
+      tree type;
+      tree exp;
+ {
+   HOST_WIDE_INT type_align, factor;
+ 
+   factor = highest_pow2_factor (exp);
+   type_align = TYPE_ALIGN (type) / BITS_PER_UNIT;
+   return MAX (factor, type_align);
  }
  \f
  /* Return an object on the placeholder list that matches EXP, a

Re: PR 6212

[Mark Mitchell]

--On Monday, May 06, 2002 04:56:31 PM -0700 Richard Henderson 
<rth@redhat.com> wrote:

> On Mon, May 06, 2002 at 03:56:52PM -0700, Mark Mitchell wrote:
>> For now, highest_pow2_factor is only used as an argument to
>> offset_address, and in offset_address, there's already a "MIN"; the
>> offset address is never more aligned than the original address.
>>
>> Does that help?
>
> I don't think so.  The following will return 64-bit alignment for the
> memory.

OK.

Kenner, you're going to have to come up with a better patch.

Yesterday would be a good time.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: PR 6212

[Richard Henderson]

On Mon, May 06, 2002 at 03:56:52PM -0700, Mark Mitchell wrote:
> For now, highest_pow2_factor is only used as an argument to
> offset_address, and in offset_address, there's already a "MIN"; the
> offset address is never more aligned than the original address.
> 
> Does that help?

I don't think so.  The following will return 64-bit alignment for the memory.

This particular example doesn't fail on Alpha at the moment, since the Alpha
backend doesn't look at MEM_ALIGN for this case.  But there _is_ stuff that
uses MEM_ALIGN; it's just a matter of trying all the combinations to find
something that fails.


r~


struct S
{
  long x;
  short y[];
};

void foo(struct S *s, int *p, int *q)
{
  s->y[(char *)q - (char *)p] = 0;
}

void bar()
{
  struct S *s = alloca(100);
  char c[2];
  foo(&s, (int *)&c[0], (int *)&c[1]);
}

Re: PR 6212

[Mark Mitchell]

--On Monday, May 06, 2002 03:50:14 PM -0700 Richard Henderson 
<rth@redhat.com> wrote:

> On Mon, May 06, 2002 at 02:56:09PM -0700, Mark Mitchell wrote:
>> I can verify the patch on GNU/Linux, too.  Is there someone who would
>> care to try this on a MIPS or Alpha box to confirm that it works OK
>> there?  (Alpha was also affected by this problem.)
>
> I'll test Alpha, but I'm pretty sure this patch is incorrect.
>
> Consider
>
> 	int foo(int *p)
> 	{
> 	  return *((char *)p);
> 	}
>
> 	char c[2] __attribute__((aligned));
>
> 	int bar()
> 	{
> 	  return foo((int *)&c[1]);
> 	}
>
> Since we recurse all the way down to the PARM_DECL, we decide that P
> is 4-byte aligned.  We already discussed cases with pointers having
> strange values (so long as they weren't dereferenced) and declared
> them acceptable in gcc.

Yup, that's valid C.

For now, highest_pow2_factor is only used as an argument to
offset_address, and in offset_address, there's already a "MIN"; the
offset address is never more aligned than the original address.

Does that help?

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: PR 6212

[Richard Henderson]

On Mon, May 06, 2002 at 02:56:09PM -0700, Mark Mitchell wrote:
> I can verify the patch on GNU/Linux, too.  Is there someone who would
> care to try this on a MIPS or Alpha box to confirm that it works OK
> there?  (Alpha was also affected by this problem.)

I'll test Alpha, but I'm pretty sure this patch is incorrect.

Consider

	int foo(int *p)
	{
	  return *((char *)p);
	}

	char c[2] __attribute__((aligned));

	int bar()
	{
	  return foo((int *)&c[1]);
	}

Since we recurse all the way down to the PARM_DECL, we decide that P
is 4-byte aligned.  We already discussed cases with pointers having
strange values (so long as they weren't dereferenced) and declared
them acceptable in gcc.

No, I think a proper solution is to let highest_pow2_factor increase
the alignment, but not decrease it.  Assuming the alignment of the
type *that we dereference* would match the behaviour of previous
gcc versions.


r~

Re: PR 6212

[Mark Mitchell]

--On Monday, May 06, 2002 05:49:22 PM -0400 Daniel Jacobowitz 
<drow@mvista.com> wrote:

> On Mon, May 06, 2002 at 08:52:07AM -0700, Mark Mitchell wrote:
>>
>> > I get some strange error messages when I try to run the G++ testsuite,
>> > most likely because there's something I haven't fully recovered from
>> > backups yet.  But it bootstraps and the C test are fine.
>>
>> That's good news, but this close to the release, "probably" isn't
>> good enough.
>>
>> Please figure out what's wrong with the G++ tests.
>
> Whatever it is, it is most likely local to Richard's setup.  His patch
> has no effect on the C, C++, ObjC, Fortran, or Java testsuites on
> i686-pc-linux-gnu, at least.

Kenner sent me this patch as his complete fix for this problem, indicating
that he was not likely to get his machine set up so that it worked soon.

I can verify the patch on GNU/Linux, too.  Is there someone who would
care to try this on a MIPS or Alpha box to confirm that it works OK
there?  (Alpha was also affected by this problem.)

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

*** expr.c	16 Apr 2002 06:03:36 -0000	1.423.2.18
--- expr.c	6 May 2002 10:43:38 -0000
*************** static rtx store_field		PARAMS ((rtx, HO
*** 147,151 ****
  					 int));
  static rtx var_rtx		PARAMS ((tree));
! static HOST_WIDE_INT highest_pow2_factor PARAMS ((tree));
  static int is_aligning_offset	PARAMS ((tree, tree));
  static rtx expand_increment	PARAMS ((tree, int, int));
--- 147,151 ----
  					 int));
  static rtx var_rtx		PARAMS ((tree));
! static unsigned HOST_WIDE_INT highest_pow2_factor PARAMS ((tree));
  static int is_aligning_offset	PARAMS ((tree, tree));
  static rtx expand_increment	PARAMS ((tree, int, int));
*************** check_max_integer_computation_mode (exp)
*** 5844,5852 ****
     This is used in updating alignment of MEMs in array references.  */

! static HOST_WIDE_INT
  highest_pow2_factor (exp)
       tree exp;
  {
!   HOST_WIDE_INT c0, c1;

    switch (TREE_CODE (exp))
--- 5844,5852 ----
     This is used in updating alignment of MEMs in array references.  */

! static unsigned HOST_WIDE_INT
  highest_pow2_factor (exp)
       tree exp;
  {
!   unsigned HOST_WIDE_INT c0, c1;

    switch (TREE_CODE (exp))
*************** highest_pow2_factor (exp)
*** 5903,5906 ****
--- 5903,5912 ----
        c1 = highest_pow2_factor (TREE_OPERAND (exp, 2));
        return MIN (c0, c1);
+
+     case PARM_DECL:  case VAR_DECL:  case RESULT_DECL:
+       /* If this is an object that is a pointer type, assume it is
+ 	 properly aligned.  */
+       if (POINTER_TYPE_P (TREE_TYPE (exp)))
+ 	return TYPE_ALIGN (TREE_TYPE (TREE_TYPE (exp)));

      default:

Re: PR 6212

[David S. Miller]

   From: Daniel Jacobowitz <drow@mvista.com>
   Date: Mon, 6 May 2002 17:49:22 -0400
   
   Whatever it is, it is most likely local to Richard's setup.  His patch
   has no effect on the C, C++, ObjC, Fortran, or Java testsuites on
   i686-pc-linux-gnu, at least.

This isn't the platform we need to verify, which is mips.

Re: PR 6212

[Daniel Jacobowitz]

On Mon, May 06, 2002 at 08:52:07AM -0700, Mark Mitchell wrote:
> 
> >I get some strange error messages when I try to run the G++ testsuite,
> >most likely because there's something I haven't fully recovered from
> >backups yet.  But it bootstraps and the C test are fine.
> 
> That's good news, but this close to the release, "probably" isn't
> good enough.
> 
> Please figure out what's wrong with the G++ tests.

Whatever it is, it is most likely local to Richard's setup.  His patch
has no effect on the C, C++, ObjC, Fortran, or Java testsuites on
i686-pc-linux-gnu, at least.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer

Re: PR 6212

[Mark Mitchell]

> I get some strange error messages when I try to run the G++ testsuite,
> most likely because there's something I haven't fully recovered from
> backups yet.  But it bootstraps and the C test are fine.

That's good news, but this close to the release, "probably" isn't
good enough.

Please figure out what's wrong with the G++ tests.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com

Re: PR 6212

[Richard Kenner]

    Seems like it would be nice if "fold" would just give us back the original
    thing, there, but that's hardly a general solution.

It does and that's indeed the problem: if it didn't fold it, things would
work OK.

    Good, I like this patch.  Please run the complete set of tests and then
    check it in.

I get some strange error messages when I try to run the G++ testsuite,
most likely because there's something I haven't fully recovered from backups
yet.  But it bootstraps and the C test are fine.

Re: PR 6212

[Mark Mitchell]

> Well, it's not "merely being read", but not being dereferenced either.

Yes; I should have said "used" perhaps.

> The relevant part of the testcase is:
>
> void
> fde_split (fde **probe, struct fde_vector *linear, struct fde_vector
> *erratic) {
>   erratic->array[probe - linear->array] = 0;
> }
>
> To do this index computation, we do the subtraction, divide by the size
> of the object being pointed to (POINTER_SIZE), then multiply by the size
> of the element of ARRAY, also POINTER_SIZE.

Seems like it would be nice if "fold" would just give us back the original
thing, there, but that's hardly a general solution.

> Well, what I tested was the following, plus the change to make that
> function unsigned:

Good, I like this patch.  Please run the complete set of tests and then
check it in.

> *** expr.c	16 Apr 2002 06:03:36 -0000	1.423.2.18
> --- expr.c	6 May 2002 10:43:38 -0000
> *************** highest_pow2_factor (exp)
> *** 5903,5906 ****
> --- 5903,5912 ----
>         c1 = highest_pow2_factor (TREE_OPERAND (exp, 2));
>         return MIN (c0, c1);
> +
> +     case PARM_DECL:  case VAR_DECL:  case RESULT_DECL:
> +       /* If this is an object that is a pointer type, assume it is
> + 	 properly aligned.  */
> +       if (POINTER_TYPE_P (TREE_TYPE (exp)))
> + 	return TYPE_ALIGN (TREE_TYPE (TREE_TYPE (exp)));
>
>       default:
>
>     You should, in any case, fix this on the mainline quickly.  There, you
>     can of course use a more invasive technique if you like.
>
> Right, but the new tree node will be a lot of work to add, so I don't want
> to rush into it.  The above should work there too.

Sure.  I think you should definitely check the patch in on the mainline
if you've tested it.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: PR 6212

[Richard Kenner]

    How did it work before?  I know these things changed a lot, but still it
    would be useful to know how we got it right in the past.

We didn't try to track the alignment of objects, but relied just on the
types, so there's really no comparison.

    I think what you're saying is that we could assume that the value stored
    in a T*, considered as an integer, is divisible by the alignment of T.
    Is that right?

Yes.

    That is a safe assumption only when the pointer is being dereferenced.
    The C standard guarantees this property; you can't use a T* to access
    something that is not a T, and a T must, by definition, be aligned as
    required for a T.

    If the pointer value is merely being read, the assumption does not hold,

Well, it's not "merely being read", but not being dereferenced either.

The relevant part of the testcase is:

void
fde_split (fde **probe, struct fde_vector *linear, struct fde_vector *erratic)
{
  erratic->array[probe - linear->array] = 0;
}

To do this index computation, we do the subtraction, divide by the size 
of the object being pointed to (POINTER_SIZE), then multiply by the size
of the element of ARRAY, also POINTER_SIZE.  So we lose the fact that there
was the multiplication.  However, both things being subtracted are known to
be multiples of POINTER_SIZE / BITS_PER_UNIT if we look at the type.

    For example, we must not optimize:

      (int)(T*)x % alignof(T)

    to 0.

Since this is just in highest_pow2_factor, we wouldn't be.

    It depends on the patch.  I will have to make a judgement call.

    It would be helpful if you post and begin testing a patch.  Since it's
    easy, please do this soon.

Well, what I tested was the following, plus the change to make that
function unsigned:

*** expr.c	16 Apr 2002 06:03:36 -0000	1.423.2.18
--- expr.c	6 May 2002 10:43:38 -0000
*************** highest_pow2_factor (exp)
*** 5903,5906 ****
--- 5903,5912 ----
        c1 = highest_pow2_factor (TREE_OPERAND (exp, 2));
        return MIN (c0, c1);
+ 
+     case PARM_DECL:  case VAR_DECL:  case RESULT_DECL:
+       /* If this is an object that is a pointer type, assume it is
+ 	 properly aligned.  */
+       if (POINTER_TYPE_P (TREE_TYPE (exp)))
+ 	return TYPE_ALIGN (TREE_TYPE (TREE_TYPE (exp)));
  
      default:

    You should, in any case, fix this on the mainline quickly.  There, you
    can of course use a more invasive technique if you like.  

Right, but the new tree node will be a lot of work to add, so I don't want
to rush into it.  The above should work there too.

Re: PR 6212

[Richard Henderson]

On Sun, May 05, 2002 at 10:43:15PM -0700, Mark Mitchell wrote:
> If the pointer value is merely being read, the assumption does not hold,
> 
> For example, we must not optimize:
> 
>   (int)(T*)x % alignof(T)
> 
> to 0.

Correct, and there is in fact a test for this in the suite.

The point at which we need to be presuming alignment is the
point at which we actually create a MEM.


r~

Re: PR 6212

[Richard Henderson]

On Sun, May 05, 2002 at 11:03:05PM -0400, Richard Kenner wrote:
> But now I just realized that one way of doing this may be saying that
> a POINTER_TYPE variable has the alignment of the underlying type.
> That's an easy change, but one that has the potential of causing
> regressions.  What's the thought here?

I think that should be ok, since the underlying type of FOOP in

  typedef int foo __attribute__((packed));
  typedef foo *foop;

should have alignment 8 rather than, say, 32.  Also need to make
sure that 

  struct S {
    char x;
    int y __attribute__((packed));
    int z;
  } __attribute__((aligned(4)));

does the right thing wrt S.y and S.z;


r~

  

Re: PR 6212

[Mark Mitchell]

> Fixing it without major work is tricky.  The problem is that we are
> removing a divide/multiply combination, so we lose the info that
> there's an aligned value here.

How did it work before?  I know these things changed a lot, but still it
would be useful to know how we got it right in the past.

> But now I just realized that one way of doing this may be saying that
> a POINTER_TYPE variable has the alignment of the underlying type.

Let's be clear.

I think what you're saying is that we could assume that the value stored
in a T*, considered as an integer, is divisible by the alignment of T.
Is that right?

That is a safe assumption only when the pointer is being dereferenced.
The C standard guarantees this property; you can't use a T* to access
something that is not a T, and a T must, by definition, be aligned as
required for a T.

If the pointer value is merely being read, the assumption does not hold,

For example, we must not optimize:

  (int)(T*)x % alignof(T)

to 0.

> That's an easy change, but one that has the potential of causing
> regressions.  What's the thought here?

It depends on the patch.  I will have to make a judgement call.

It would be helpful if you post and begin testing a patch.  Since it's
easy, please do this soon.

You should, in any case, fix this on the mainline quickly.  There, you
can of course use a more invasive technique if you like.  Fixing it
there will give us a chance to test the change and, possibly, incorporate
it in GCC 3.1.1 if your easy patch doesn't work out.

-- 
Mark Mitchell                mark@codesourcery.com
CodeSourcery, LLC            http://www.codesourcery.com

Re: PR 6212

[Richard Kenner]

    The mips thing has been fixed.

Good.

    But the alignment issue is still a serious performance regression.
    That is why the PR is still open and high priority.

Well, not quite as high as something being broken ...

Fixing it without major work is tricky.  The problem is that we are
removing a divide/multiply combination, so we lose the info that
there's an aligned value here.

My idea for GCC 3.2 is to add a NOP_WITH_ASSERT tree node that can be used
to record the alignment information.

The first few times I looked at this, that's the only solution I saw.
But now I just realized that one way of doing this may be saying that
a POINTER_TYPE variable has the alignment of the underlying type.
That's an easy change, but one that has the potential of causing
regressions.  What's the thought here?

Re: PR 6212

[Richard Henderson]

On Sun, May 05, 2002 at 08:13:31PM -0400, Richard Kenner wrote:
>     So there is in fact a secondary bug in the mips backend.  Namely,
>     the movsi_usw and movdi_usd patterns are identical if the argument
>     is the constant zero.  Patch for that pending.
> 
> So I assumed it had been fixed and closed.

The mips thing has been fixed.

But the alignment issue is still a serious performance regression.
That is why the PR is still open and high priority.


r~

Re: PR 6212

[Richard Kenner]

    You should have asked for help earlier.

    There was a link in the PR audit trail to this message:

      http://gcc.gnu.org/ml/gcc/2002-04/msg01220.html

    which contains the small test case.

Now I remember.  I was going forward on messages the other day and hadn't
gotten to that one.

What I concluded from that is that we can't fix the alignment issue here
easily (it needs a mechanism that I already plan to do for 3.2), but the
bug can be fixed by fix mentioned here by RTH:

    So there is in fact a secondary bug in the mips backend.  Namely,
    the movsi_usw and movdi_usd patterns are identical if the argument
    is the constant zero.  Patch for that pending.

So I assumed it had been fixed and closed.

RTH?

PR 6212

[Mark Mitchell]

Kenner --

At this point, the *only* release-critical bug in the compiler proper
open in GNATS is PR 6212.

I have asked you several times to help with this bug.  Richard Henderson
was kind enough to extract a small test case for this bug, that can be
easily replicated with a cross compiler.  I reminded you of this fact
several days ago.

Please let us know ASAP what progress you have made towards fixing this
bug.  If, for some reason, you are not going to fix it, please let us
know that as well, so that I, or someone else, to fix it.

If, by chance, you have already fixed it and I have not noticed, please
close the PR.  It is your obligation to close PRs for bugs that you have
fixed.

I am not happy about this situation, for the following reasons:

1. The bug was reported months ago, and traced to a patch of yours.

2. Richard Henderson factored out a small test case some time ago, and
   the bug was still not fixed.

3. I notified you that this was a release-critical bug several days ago,
   and the bug is still not fixed.

--
Mark Mitchell                   mark@codesourcery.com
CodeSourcery, LLC               http://www.codesourcery.com