[Bug middle-end/52436] New: BIT_FIELD_REF <MEM_REF <>> should be canonicalized for non-bitfield accesses

[Bug middle-end/52436] New: BIT_FIELD_REF <MEM_REF <>> should be canonicalized for non-bitfield accesses

[rguenth at gcc dot gnu.org]

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

             Bug #: 52436
           Summary: BIT_FIELD_REF <MEM_REF <>> should be canonicalized for
                    non-bitfield accesses
    Classification: Unclassified
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Keywords: missed-optimization
          Severity: enhancement
          Priority: P3
         Component: middle-end
        AssignedTo: rguenth@gcc.gnu.org
        ReportedBy: rguenth@gcc.gnu.org


typedef long long __m128i __attribute__ ((__vector_size__ (16),
                                          __may_alias__));
typedef struct
{
  __m128i b;
} s_1a;
typedef s_1a s_1m __attribute__((aligned(1)));
void
foo (s_1m *p)
{
  p->b[1] = 5;
}

Produces in .optimized

foo (struct s_1m * p)
{
<bb 2>:
  BIT_FIELD_REF <p_1(D)->b, 64, 64> = 5;
  return;

}

we should have canoncialized (aka fold_stmt'ed) the LHS to

  MEM <(__m128i *)p_1(D), 8> = 5;

[Bug middle-end/52436] BIT_FIELD_REF <MEM_REF <>> should be canonicalized for non-bitfield accesses

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2012-02-29
     Ever Confirmed|0                           |1

--- Comment #1 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-02-29 14:20:16 UTC ---
Mine.

[Bug middle-end/52436] BIT_FIELD_REF <MEM_REF <>> should be canonicalized for non-bitfield accesses

[glisse at gcc dot gnu.org]

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

--- Comment #2 from Marc Glisse <glisse at gcc dot gnu.org> 2013-04-01 15:14:59 UTC ---
Created attachment 29767
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=29767
patch

This patch may be a bit too strong. In particular, it breaks
gcc.dg/vect/nodump-forwprop-22.c (although we might consider that it is
forwprop that is too weak). Also, it doesn't put exactly __m128i* in the
MEM_REF but instead long long* (s_1m* would be easy as well, but I don't know
what property of __m128i makes it the right type so I can't teach that to the
compiler). The forwprop bit is for PR55266 and not strictly necessary here.

[Bug middle-end/52436] BIT_FIELD_REF <MEM_REF <>> should be canonicalized for non-bitfield accesses

[rguenth at gcc dot gnu.org]

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

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> 2013-04-02 08:27:51 UTC ---
The patch looks fine to me, though it might be a bit expensive.  I'd
re-organize it like

  if (TREE_INT_CST_LOW (arg1) % BITS_PER_UNIT == 0
      && TREE_INT_CST_LOW (arg2) % BITS_PER_UNIT == 0)
    {
      HOST_WIDE_INT coffset;
      tree base = get_addr_base_and_unit_offset (arg0, &coffset);
      if (base)
        return fold_build2 (MEM_REF, type,
                            build_fold_addr_expr (base),
                            build_int_cst (build_pointer_type
(TYPE_MAIN_VARIANT (type)), coffset + TREE_INT_CST_LOW (arg1) /
BITS_PER_UNIT));
    }

it avoids building a tree just to feed it into get_addr_base_and_unit_offset
It also avoids it if the access is not of byte-granularity which
get_addr_base_and_unit_offset does not even consider (it assumes it is fed
the argument of an ADDR_EXPR which obviously is byte-aligned).

The tree-flow-inline.h bits look ok.

I'm not sure what you need the forwprop / tree-ssa-propagate changes for.

[Bug middle-end/52436] BIT_FIELD_REF <MEM_REF <>> should be canonicalized for non-bitfield accesses

[glisse at gcc dot gnu.org]

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

--- Comment #4 from Marc Glisse <glisse at gcc dot gnu.org> 2013-04-02 14:08:57 UTC ---
(In reply to comment #3)
> I'd re-organize it like

Ok, I'll try something like that.

> it avoids building a tree just to feed it into get_addr_base_and_unit_offset

The idea was that there would be almost no cases where this was done and no
simplification occurred. If a transformation is indeed done, building one tree
is not that expensive, especially if it avoids code duplication.

> It also avoids it if the access is not of byte-granularity which
> get_addr_base_and_unit_offset does not even consider (it assumes it is fed
> the argument of an ADDR_EXPR which obviously is byte-aligned).
> 
> The tree-flow-inline.h bits look ok.

In get_addr_base_and_unit_offset_1 I already check if the offset is a multiple
of BITS_PER_UNIT, I should probably check that the size is a multiple as well,
no?

> I'm not sure what you need the forwprop / tree-ssa-propagate changes for.

I need something to call fold on a bit_field_ref of a mem_ref. For PR55266, the
vector lowering pass produces a mem_ref and a bit_field_ref in two distinct
gimple statements, and nothing tries to combine them.

[Bug middle-end/52436] BIT_FIELD_REF <MEM_REF <>> should be canonicalized for non-bitfield accesses

[rguenther at suse dot de]

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

--- Comment #5 from rguenther at suse dot de <rguenther at suse dot de> 2013-04-02 14:21:56 UTC ---
On Tue, 2 Apr 2013, glisse at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=52436
> 
> --- Comment #4 from Marc Glisse <glisse at gcc dot gnu.org> 2013-04-02 14:08:57 UTC ---
> (In reply to comment #3)
> > I'd re-organize it like
> 
> Ok, I'll try something like that.
> 
> > it avoids building a tree just to feed it into get_addr_base_and_unit_offset
> 
> The idea was that there would be almost no cases where this was done and no
> simplification occurred. If a transformation is indeed done, building one tree
> is not that expensive, especially if it avoids code duplication.

AFAIK it really doesn't.

> > It also avoids it if the access is not of byte-granularity which
> > get_addr_base_and_unit_offset does not even consider (it assumes it is fed
> > the argument of an ADDR_EXPR which obviously is byte-aligned).
> > 
> > The tree-flow-inline.h bits look ok.
> 
> In get_addr_base_and_unit_offset_1 I already check if the offset is a multiple
> of BITS_PER_UNIT, I should probably check that the size is a multiple as well,
> no?

No, get_addr_base_and_unit_offset_1 only is supposed to return the
addressable offset into an object - it doesn't care about access sizes.

> > I'm not sure what you need the forwprop / tree-ssa-propagate changes for.
> 
> I need something to call fold on a bit_field_ref of a mem_ref. For PR55266, the
> vector lowering pass produces a mem_ref and a bit_field_ref in two distinct
> gimple statements, and nothing tries to combine them.

But you can't really do what you do there.  You are possibly
transforming

  vecreg_2 = MEM[...];
....
  MEM[...] = xxxx; // clobber the memory location 
....
  scalreg_3 = BIT_FIELD_REF <vecreg_2, ...>;

into

  vecreg_2 = MEM[...]; // possibly unused
...
  MEM[...] = xxxx; // clobber the memory location
...
  scalreg_3 = MEM[...];

which is not correct, obviously.  "combining" with memory
accesses isn't trivial, certainly not a task I would consider
for forwprop.  I can't think of a suitable existing pass
that would combine this, but value-numbering should eventually
value-number both cases the same at least.

Richard.

[Bug middle-end/52436] BIT_FIELD_REF <MEM_REF <>> should be canonicalized for non-bitfield accesses

[glisse at gcc dot gnu.org]

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

--- Comment #6 from Marc Glisse <glisse at gcc dot gnu.org> 2013-04-02 14:36:59 UTC ---
(In reply to comment #5)
> No, get_addr_base_and_unit_offset_1 only is supposed to return the
> addressable offset into an object - it doesn't care about access sizes.

It is also allowed to say "bad idea, I will return NULL", but ok.

> But you can't really do what you do there.  You are possibly
> transforming
[...]

Good point, thanks.

> which is not correct, obviously.  "combining" with memory
> accesses isn't trivial, certainly not a task I would consider
> for forwprop.  I can't think of a suitable existing pass
> that would combine this, but value-numbering should eventually
> value-number both cases the same at least.

I guess that leaves the vector lowering pass as the easiest alternative for
PR55266: split all unsupported memory reads into supported reads and a
constructor (and similarly, split writes into bit_field_refs and writes) and
hope that nothing does the reverse transformation.

[Bug middle-end/52436] BIT_FIELD_REF <MEM_REF <>> should be canonicalized for non-bitfield accesses

[glisse at gcc dot gnu.org]

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

--- Comment #7 from Marc Glisse <glisse at gcc dot gnu.org> 2013-04-04 12:07:53 UTC ---
For the rest of the discussion, see the thread starting here:
http://gcc.gnu.org/ml/gcc-patches/2013-04/msg00169.html

In particular, the folding should be done in gimple only (most likely
maybe_fold_reference) and test !is_gimple_reg().