[Bug middle-end/49937] New: [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C

[Bug middle-end/49937] New: [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49937

           Summary: [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: middle-end
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: hp@gcc.gnu.org
        Depends on: 49545
              Host: x64_86-unknown-linux-gnu
            Target: cris-axis-elf


+++ This bug was initially created as a clone of Bug #49545 +++

Still seeing
FAIL: g++.dg/tree-ssa/fwprop-align.C scan-tree-dump-times forwprop2 "& 1" 0
at r177051.

See PR49545 for the rest.

[Bug middle-end/49937] [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49937

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.7.0

[Bug middle-end/49937] [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49937

Hans-Peter Nilsson <hp at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #1 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2011-08-09 00:05:00 UTC ---
I'm going out on a limb and CC the author of the most suspect patch, r176508,
in the svn range, r176507:176524, where g++.dg/tree-ssa/fwprop-align.C was
"reintroduced" as a regression for cris-elf.  Will confirm with triaging.

[Bug middle-end/49937] [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49937

Hans-Peter Nilsson <hp at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011.08.09 11:38:39
     Ever Confirmed|0                           |1

--- Comment #2 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2011-08-09 11:38:39 UTC ---
(In reply to comment #1)
> Will confirm with triaging.
Done.

[Bug tree-optimization/49937] [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49937

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
          Component|middle-end                  |tree-optimization
         AssignedTo|unassigned at gcc dot       |rguenth at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #3 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-08-09 12:19:20 UTC ---
The folding happens (and is supposed to happen) in CCP for x86_64 (that was
the idea when removing the forwprop duplicate, which has correctness issues,
instead of "fixing" it).

CCP uses get_value_from_alignment, the difference between x86_64 and the
cross is:

Visiting statement:
D.2090_3 = (long int) foo;
which is likely CONSTANT
Lattice value changed to CONSTANT Lattice value changed to CONSTANT
0x00000000000000000 (0xfffffffffffffffffffffffffffffffe).  Adding SSA edges to
worklist.

Visiting statement:
D.2091_4 = D.2090_3 & 1;
which is likely CONSTANT
Lattice value changed to CONSTANT 0.  Adding SSA edges to worklist.

vs. cris:

Visiting statement:
D.1722_3 = (long int) foo;
which is likely CONSTANT
Lattice value changed to VARYING.  Adding SSA edges to worklist.

Visiting statement:
D.1723_4 = D.1722_3 & 1;
which is likely CONSTANT
Lattice value changed to CONSTANT Lattice value changed to CONSTANT
0x00000000000000000 (0x00000000000000001).  Adding SSA edges to worklist.

which is because

  else if (base
           && ((align = get_object_alignment (base, BIGGEST_ALIGNMENT))
                > BITS_PER_UNIT))

even though the FUNCTION_DECL has align 16, BIGGEST_ALIGNMENT is 8 on cris
(which means we can't rely on any DECL_ALIGN or other stuff
get_object_alignment
uses if it is bigger than that value?).

Thus, the get_object_alignment_1 hunk

  if (DECL_P (exp)
      && TREE_CODE (exp) != LABEL_DECL)
    {
      if (TREE_CODE (exp) == FUNCTION_DECL)
        {
          /* Function addresses can encode extra information besides their
             alignment.  However, if TARGET_PTRMEMFUNC_VBIT_LOCATION
             allows the low bit to be used as a virtual bit, we know
             that the address itself must be 2-byte aligned.  */
          if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn)
            align = 2 * BITS_PER_UNIT;
          else
            align = BITS_PER_UNIT;
        }

is non-functional for cris.

>From looking at the docs for BIGGEST_ALIGNMENT I wonder why we use that
for all callers of get_object_alignment and not, for example,
MAX_OFILE_ALIGNMENT?

CCP certainly doesn't care - as long as it can trust the value returned.
CCP wants to use get_object_alignment_1 anyway.

hp, can you maybe answer the question about correctness for aligns
bigger than BIGGEST_ALIGNMENT?  And eventually audit other callers?

[Bug tree-optimization/49937] [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49937

--- Comment #4 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-08-09 13:22:23 UTC ---
Created attachment 24959
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24959
patch

Patch I'm testing.

[Bug tree-optimization/49937] [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49937

--- Comment #5 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2011-08-09 15:38:32 UTC ---
(In reply to comment #3)
Thanks for looking.

> hp, can you maybe answer the question about correctness for aligns
> bigger than BIGGEST_ALIGNMENT?

I'm not completely sure what you're asking, but I think I can answer the
question, when asked. :)

Having BIGGEST_ALIGNMENT 8 for cris-* seems valid, as BIGGEST_ALIGNMENT is the
"biggest alignment that any data type can require on this machine, in bits",
but when calling a function we're not accessing data, right?

Anyway, "this is not the biggest alignment that is supported, just the biggest
alignment that, when violated, may cause a fault".  So, it's definitely valid
to mention or explicitly demanding higher alignment (like in
attribute-aligned).  I guess we're violating an alignment when calling a
misaligned function, but letting BIGGEST_ALIGNMENT control that, doesn't seem
right.  All the macros defaulting to BIGGEST_ALIGNMENT and many (all?) other
uses are about data accesses and aligning objects being created for valid
alignment for any data.  Raising it just to match FUNCTION_BOUNDARY would be
just wrong, I can't see any caller to intend to include function alignments and
it'd pessimize the results for them.  I see frv and lm32 also have a higher
FUNCTION_BOUNDARY than BIGGEST_ALIGNMENT but luckily larger than 8 bits...

>  And eventually audit other callers?

I had a look and can definitely have a more thorough review when we're done
interpreting what the alignment macros should control. ;)  I'm not sure all
callers accessing data are using it correctly; some of those using it for
pointers (rather than objects) may assume a too big alignment.

[Bug tree-optimization/49937] [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49937

--- Comment #6 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2011-08-09 20:25:14 UTC ---
(In reply to comment #5)
> BIGGEST_ALIGNMENT is the
> "biggest alignment that any data type can require on this machine, in bits"
> "this is not the biggest alignment that is supported, just the biggest
> alignment that, when violated, may cause a fault".

Hence its use is suboptimal except as a fallback or when the *preferred*
alignment isn't stated, like in "LOCAL_ALIGNMENT (blob, BIGGEST_ALIGNMENT)".

Maybe it should be renamed to something matching its use, like
MAXIMUM_OF_MININUM_ALIGNMENTS_FOR_CPU_DATA_ACCESS...

[Bug tree-optimization/49937] [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C

[hp at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49937

--- Comment #7 from Hans-Peter Nilsson <hp at gcc dot gnu.org> 2011-08-10 00:00:28 UTC ---
(In reply to comment #4)
> Created attachment 24959 [details]
> patch
> 
> Patch I'm testing.

Me too: fixes the bug with no regressions for cris-elf at r177605.  Thanks.
What do you think if anything, should be done for BIGGEST_ALIGNMENT usage?
Ok, let's take it to the list.

[Bug tree-optimization/49937] [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49937

--- Comment #8 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-08-10 08:50:43 UTC ---
Author: rguenth
Date: Wed Aug 10 08:50:39 2011
New Revision: 177615

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=177615
Log:
2011-08-10  Richard Guenther  <rguenther@suse.de>

    PR tree-optimization/49937
    * tree-ssa-ccp.c (get_value_from_alignment): Re-implement
    using get_object_alignment_1.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-ssa-ccp.c

[Bug tree-optimization/49937] [4.7 Regression] g++.dg/tree-ssa/fwprop-align.C

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49937

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|                            |FIXED

--- Comment #9 from Richard Guenther <rguenth at gcc dot gnu.org> 2011-08-10 08:53:09 UTC ---
(In reply to comment #7)
> (In reply to comment #4)
> > Created attachment 24959 [details]
> > patch
> > 
> > Patch I'm testing.
> 
> Me too: fixes the bug with no regressions for cris-elf at r177605.  Thanks.
> What do you think if anything, should be done for BIGGEST_ALIGNMENT usage?
> Ok, let's take it to the list.

I _think_ now that get_{pointer,object}_alignment is returning a conservative
result (thus, 8 for any pointer that hasn't alignment meta information
attached) limiting the result is pointless - certainly limiting it to
BIGGEST_ALIGNMENT.

Thus I'd propose to get rid of that argument to those functions.  On that
course see if any caller cannot deal with an alignment bigger than
BIGGEST_ALIGNMENT (which would be odd).

Well, this one is fixed at least.