[Bug c/66768] New: __seg_fs and __seg_gs: issue when adding address space support

[Bug c/66768] New: __seg_fs and __seg_gs: issue when adding address space support

[arigo at tunes dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66768

            Bug ID: 66768
           Summary: __seg_fs and __seg_gs: issue when adding address space
                    support
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: arigo at tunes dot org
  Target Milestone: ---

Created attachment 35913
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=35913&action=edit
Input incorrectly compiled with gcc-5.1.0-patched

I implemented support for %fs and %gs segment prefixes on the x86 and
x86-64 platforms, in what turns out to be a small patch.  A full description
and the motivation can be found in
https://gcc.gnu.org/ml/gcc/2015-07/msg00031.html .

This bug report is unusual in that it is not about a bug in the current version
of gcc, but in the gcc with a patch added
(https://gcc.gnu.org/ml/gcc/2015-07/txtJZYwz9PTTI.txt).  It might of course be
that my patch is incorrect, but the patch really only adds two extra address
spaces; given that it is straightforward, I'd guess there is a chance that the
same bug occurs on other platforms that already have multiple address spaces. 
(Either that, or some code somewhere is confused by the standard-sized,
alternate-address-spaced pointers.)

The bug is that with gcc 5.1.0 patched as above, compiling bug1.c on x86-64
with "-O1" produces https://gcc.gnu.org/ml/gcc/2015-07/msg00031/bug1.s.  In one
case, the __seg_gs address space is incorrectly dropped.  With "-O0" it works
fine.  If I try to dump all the rtl passes, I see that the __seg_gs address
space is already missing in the first rtl dump, so I suspect that some earlier
optimization does not respect the presence of address spaces.

(I tried to replace the "-O1" with the list of "-f" flags output by "-O1 -Q
--help=optimizers" with the goal of removing most of them until I can pinpoint
which optimization causes the problem, but with no luck: even replacing "-O1"
by all the [enabled] flags produces a different and correct result...)

[Bug c/66768] __seg_fs and __seg_gs: issue when adding address space support

[arigo at tunes dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66768

--- Comment #1 from Armin Rigo <arigo at tunes dot org> ---
Update: found out that the %gs prefix is correctly present when I compile with
"gcc -O1 -fno-ivopts bug1.c -S".  So ivopts might be the place to look.

[Bug c/66768] __seg_fs and __seg_gs: issue when adding address space support

[arigo at tunes dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66768

--- Comment #2 from Armin Rigo <arigo at tunes dot org> ---
Actually "gcc -O1 -fno-tree-loop-optimize bug1.c -S" also restores the %gs
prefix.  I suspect however that this flag implies "-fno-ivopts", or something. 
I found no other "-fno-xxx" that, when given alone, restores the %gs, no I
think that either ivopts or tree-loop-optimize is definitely the correct place
to look.

[Bug tree-optimization/66768] __seg_fs and __seg_gs: issue when adding address space support

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66768

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |amker.cheng at gmail dot com,
                   |                            |rguenth at gcc dot gnu.org
          Component|c                           |tree-optimization

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
I can very well imagine that IVOPTs fails to preserve address-spaces here. 
Eventually it is also RTL expansion that in the end makes the issue surface.

Not sure which existing targest with address-space support would share enough
properties with the proposed patch to expose the same issue.

[Bug tree-optimization/66768] __seg_fs and __seg_gs: issue when adding address space support

[amker at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66768

amker at gcc dot gnu.org changed:

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

--- Comment #4 from amker at gcc dot gnu.org ---
I shall have a look.

Thanks

[Bug tree-optimization/66768] __seg_fs and __seg_gs: issue when adding address space support

[amker at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66768

--- Comment #5 from amker at gcc dot gnu.org ---
I can reproduce the problem on avr using its namespace with GCC trunk.  The
example code is as below:

typedef __memx struct foo_s {
    int a[20];
} foo_t;


int sum1(const foo_t *p)
{
    int i, total=0;
    for (i=0; i<20; i++)
        total += p->a[i];
    return total;
}

int sum2(void)
{
    const foo_t *p = (const foo_t *)0x1234;
    int i, total=0;
    for (i=0; i<20; i++)
        total += p->a[i];
    return total;
}

The issue arises because of below facts.

1) To avoid undefined overflow behavior, IV candidates are all of unsigned
type.  As a result, rewrite_use_address can't tell IV pointing to memory
objects from IV that represents simply an offset.
2) To address 1), IVO records base_object information for every IV that
actually points to memory object.  And base_object is computed in alloc_iv with
a call to determine_base_object.
3) In this case, the address of memory object is of form like
"&MEM[(ptr_type)4660B].a[0]".
4) Now the address is firstly lowered into "4660B", and determine_base_object
can't identify any base_object from const_int, it just returns NULL.
5) With NULL base_object, rewrite_use_address_1 won't pass base_hint to
create_mem_ref.  As a result, memory reference is rewritten into form of
"MEM[base: 0B, index: ivtmp.20_8, offset: 0B]".
6) Though correct memory access can be generated from that reference,
address_space attribute is lost because of "ZERO base".

I suspect this issue exists all the time.  Even we pass "&MEM[(ptr)4660B].a[0]"
directly to determine_base_object, it won't consider *((ptr_type)const_int) as
the memory object and just return NULL.

I can see one possible fix to it.
1) call determine_base_object before lowering the address expression in
alloc_iv.
2) change determine_base_object to take memory objects at constant int address
into consideration.
3) for subsequent alloc_iv calls in add_candidate_1, we need to add new
parameter base_object and pass the original iv->base_object to it.  Otherwise,
it's impossible to determine base_object because "base" has already been
lowered...

So what's your opinion?

Thanks.

[Bug tree-optimization/66768] address space gets lost on literal pointer

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66768

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Target|                            |avr
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2015-07-07
            Version|unknown                     |6.0
            Summary|__seg_fs and __seg_gs:      |address space gets lost on
                   |issue when adding address   |literal pointer
                   |space support               |
     Ever confirmed|0                           |1

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
IVOPTs seems to carry address-space info on 'type' here (in fact if I amend
tree dumping with address-space dumping on memory references I fail to get
proper address-space info on the first function _before_ IVOPTs...).

Address-space info is on the base object type, not on the type of the
memory reference (for MEM_REF and TARGET_MEM_REF, that is).

For the IVOPTs issue a way to fix the particular testcase is

Index: gcc/tree-ssa-address.c
===================================================================
--- gcc/tree-ssa-address.c      (revision 225504)
+++ gcc/tree-ssa-address.c      (working copy)
@@ -395,7 +395,7 @@ create_mem_ref_raw (tree type, tree alia
     }
   else
     {
-      base = build_int_cst (ptr_type_node, 0);
+      base = build_int_cst (build_pointer_type (type), 0);
       index2 = addr->base;
     }

but then we ICE in convert_memory_address_addr_space_1

#1  0x000000000093323b in convert_memory_address_addr_space_1 (
    to_mode=PSImode, x=0x7ffff69f5c60, as=7 '\a', in_const=false)
    at /space/rguenther/tramp3d/trunk/gcc/explow.c:282
282       gcc_assert (GET_MODE (x) == to_mode || GET_MODE (x) == VOIDmode);
(gdb) p debug_rtx (x)
(subreg:HI (reg:PSI 42 [ ivtmp.21 ]) 0)

at least the address-space is now on the MEM_REF:

  <bb 3>:
  # total_11 = PHI <total_5(3), 0(2)>
  # ivtmp.21_2 = PHI <ivtmp.21_7(3), 4660(2)>
  _12 = (sizetype) ivtmp.21_2;
  _4 = MEM[base: 0B, index: _12, offset: 0B]<address-space-7> ;

the ICE is probably a AVR bug (it's inside POITNERS_EXTEND_UNSIGNED).
Possibly the subreg case can be applied for POINTERS_EXTEND_UNSIGNED as well.

[Bug tree-optimization/66768] address space gets lost on literal pointer

[amker at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66768

--- Comment #7 from amker at gcc dot gnu.org ---
(In reply to Richard Biener from comment #6)
> IVOPTs seems to carry address-space info on 'type' here (in fact if I amend
> tree dumping with address-space dumping on memory references I fail to get
> proper address-space info on the first function _before_ IVOPTs...).
> 
> Address-space info is on the base object type, not on the type of the
> memory reference (for MEM_REF and TARGET_MEM_REF, that is).
> 
> For the IVOPTs issue a way to fix the particular testcase is
> 
> Index: gcc/tree-ssa-address.c
> ===================================================================
> --- gcc/tree-ssa-address.c      (revision 225504)
> +++ gcc/tree-ssa-address.c      (working copy)
> @@ -395,7 +395,7 @@ create_mem_ref_raw (tree type, tree alia
>      }
>    else
>      {
> -      base = build_int_cst (ptr_type_node, 0);
> +      base = build_int_cst (build_pointer_type (type), 0);
>        index2 = addr->base;
>      }
>  
> but then we ICE in convert_memory_address_addr_space_1
> 
> #1  0x000000000093323b in convert_memory_address_addr_space_1 (
>     to_mode=PSImode, x=0x7ffff69f5c60, as=7 '\a', in_const=false)
>     at /space/rguenther/tramp3d/trunk/gcc/explow.c:282
> 282       gcc_assert (GET_MODE (x) == to_mode || GET_MODE (x) == VOIDmode);
> (gdb) p debug_rtx (x)
> (subreg:HI (reg:PSI 42 [ ivtmp.21 ]) 0)
> 
> at least the address-space is now on the MEM_REF:
> 
>   <bb 3>:
>   # total_11 = PHI <total_5(3), 0(2)>
>   # ivtmp.21_2 = PHI <ivtmp.21_7(3), 4660(2)>
>   _12 = (sizetype) ivtmp.21_2;
>   _4 = MEM[base: 0B, index: _12, offset: 0B]<address-space-7> ;
> 
> the ICE is probably a AVR bug (it's inside POITNERS_EXTEND_UNSIGNED).
> Possibly the subreg case can be applied for POINTERS_EXTEND_UNSIGNED as well.

Yes, I had tried this method and run into exactly the same ICE.  I don't know
if it's AVR's bug so went into IVOPT for a fix.  According to IVO comments,
rewrite_use_address&create_mem_ref should try their best to distribute
candidate var into base part pf MEM_REF/TARGET_MEM_REF if the cand actually
stands for address of memory reference.  While it failed in this case.

[Bug tree-optimization/66768] address space gets lost on literal pointer

[amker at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66768

--- Comment #8 from amker at gcc dot gnu.org ---
So address space info is kept and checked in base object's type of MEM_REF.  As
in function expand_expr_real_1:

    case TARGET_MEM_REF:
      {
        addr_space_t as
          = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 0))));
        enum insn_code icode;
        unsigned int align;

The AVR ICE happens when base of MEM_REF[base:0, index:(sizetype)ivtmp] has
__memx address space attribute.  Since pointer to memory object in __memx
address space (PSImode) has 24 bits type length, while the type of index is
sizetype(Pmode == HImode in this case), which has 16 bits type length.  The
expression "(sizetype)ivtmp" is expanded into (subreg:HI (reg:PSI 40) 0).

So I still think IVO should distribute ivtmp as base part of MEM_REF since it
stands for a memory object.  Otherwise, we have below IVOed code:

  <bb 3>:
  # total_10 = PHI <total_5(4), 0(2)>
  # ivtmp.7_8 = PHI <ivtmp.7_7(4), 4660(2)>
  _12 = (sizetype) ivtmp.7_8;
  _4 = MEM[base: 0B, index: _12, offset: 0B];
  total_5 = _4 + total_10;
  ivtmp.7_7 = ivtmp.7_8 + 2;
  if (ivtmp.7_7 != 4700)
    goto <bb 4>;
  else
    goto <bb 5>;

  <bb 4>:
  goto <bb 3>;


This is wrong since truncation of ivtmp.7_8 to sizetype could result in wrong
address.

Thanks.

[Bug tree-optimization/66768] address space gets lost on literal pointer

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66768

--- Comment #9 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 7 Jul 2015, amker at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66768
> 
> --- Comment #8 from amker at gcc dot gnu.org ---
> So address space info is kept and checked in base object's type of MEM_REF.  As
> in function expand_expr_real_1:
> 
>     case TARGET_MEM_REF:
>       {
>         addr_space_t as
>           = TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (TREE_OPERAND (exp, 0))));
>         enum insn_code icode;
>         unsigned int align;
> 
> The AVR ICE happens when base of MEM_REF[base:0, index:(sizetype)ivtmp] has
> __memx address space attribute.  Since pointer to memory object in __memx
> address space (PSImode) has 24 bits type length, while the type of index is
> sizetype(Pmode == HImode in this case), which has 16 bits type length.  The
> expression "(sizetype)ivtmp" is expanded into (subreg:HI (reg:PSI 40) 0).
> 
> So I still think IVO should distribute ivtmp as base part of MEM_REF since it
> stands for a memory object.  Otherwise, we have below IVOed code:
> 
>   <bb 3>:
>   # total_10 = PHI <total_5(4), 0(2)>
>   # ivtmp.7_8 = PHI <ivtmp.7_7(4), 4660(2)>
>   _12 = (sizetype) ivtmp.7_8;
>   _4 = MEM[base: 0B, index: _12, offset: 0B];
>   total_5 = _4 + total_10;
>   ivtmp.7_7 = ivtmp.7_8 + 2;
>   if (ivtmp.7_7 != 4700)
>     goto <bb 4>;
>   else
>     goto <bb 5>;
> 
>   <bb 4>:
>   goto <bb 3>;
> 
> 
> This is wrong since truncation of ivtmp.7_8 to sizetype could result in wrong
> address.

Indeed truncating it to sizetype just to make it fit into INDEX
looks wrong (extension is ok, truncation is not).

[Bug tree-optimization/66768] address space gets lost on literal pointer

[gjl at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66768

Georg-Johann Lay <gjl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |addr-space
                 CC|                            |gjl at gcc dot gnu.org

--- Comment #10 from Georg-Johann Lay <gjl at gcc dot gnu.org> ---
Guess the target should be x86?