fwprop and zero_extract

[Amir Gonnen-2]

(gcc-4.4.1, proprietary backend)

Hi,
It looks like fwprop does not propagate addresses that appear in
zero_extract on the set dest.
Here is an example:

(insn 8 5 9 2 c:\mingw\home\amirg\test\test.c:157 (set (reg/f:HI 46)
        (const:HI (plus:HI (symbol_ref:HI ("r") [flags 0x40] <var_decl
0x2ee40b0 r>)
                (const_int 68 [0x44])))) 18 {*movhi} (nil))

(insn 9 8 0 2 c:\mingw\home\amirg\test\test.c:157 (set
(zero_extract:SI (mem/s/j:QI (plus:HI (reg/f:HI 46)
                    (const_int 2 [0x2])) [0 S1 A8])
            (const_int 1 [0x1])
            (const_int 1 [0x1]))
        (const_int 0 [0x0])) 29 {*movb_lh} (nil))

The problem seems to be in df_scan.c: df_uses_record doesn't handle
the case of (set (zero_extract(mem(...) ...) ...).
So I tried the following patch in df_uses_record under case SET: ...
case ZERO_EXTRACT: ...

--- df-scan.c
+++ df-scan.c
@@ -3152,11 +3152,24 @@
   {
     width = INTVAL (XEXP (dst, 1));
     offset = INTVAL (XEXP (dst, 2));
-    mode = GET_MODE (dst);
+    mode = GET_MODE (dst);
+ /* AG start */
+ /* Handle the case of zero_extract(mem(...)) in the set dest */
+ if (GET_CODE (XEXP (dst,0)) == MEM)
+ {
+ df_uses_record (DF_REF_EXTRACT, collection_rec, &XEXP (XEXP (dst,0), 0),
+  DF_REF_REG_MEM_STORE, bb, insn_info,
+  DF_REF_ZERO_EXTRACT,
+  width, offset, mode);
+ }
+ else
+ {
+ /* AG end */
      df_uses_record (DF_REF_EXTRACT, collection_rec, &XEXP (dst, 0),
  DF_REF_REG_USE, bb, insn_info,
  DF_REF_READ_WRITE | DF_REF_ZERO_EXTRACT,
  width, offset, mode);
+ } /* AG added closing brackets */
   }
  else
   {

and now fwprop propagates properly:

(insn 9 5 0 2 c:\mingw\home\amirg\test\test.c:157 (set
(zero_extract:SI (mem/s/j:QI (const:HI (plus:HI (symbol_ref:HI ("r")
[flags 0x40] <var_decl 0x2ee40b0 r>)
                        (const_int 70 [0x46]))) [0 S1 A8])
            (const_int 1 [0x1])
            (const_int 1 [0x1]))
        (const_int 0 [0x0])) 29 {*movb_lh} (nil))

I'll appreciate any comments,
Thanks,

Amir

[Paolo Bonzini-6]

On 11/04/2009 01:38 PM, Amir Gonnen wrote:
> The problem seems to be in df_scan.c: df_uses_record doesn't handle
> the case of (set (zero_extract(mem(...) ...) ...).
> So I tried the following patch in df_uses_record under case SET: ...
> case ZERO_EXTRACT: ...

The patch makes sense, can you please check what happens for a
ZERO_EXTRACT with non-constant values in the 2nd and 3rd field?

To submit it, you will need to provide a ChangeLog entry, remove the AG
markers and follow the GNU coding standards with respect to indentation.

Thanks in advance for this contribution!

Paolo

[Kenneth Zadeck-2]

Amir,

I tested and committed a slightly different and cleaned up version of
this patch with a proper changelog.  It would be good if you reported
back the results of request by Paolo in case that shows any problems.

We unfortunately cannot directly accept patches from people that do
not have a gcc/gnu copyright assignment.    We would strongly recommend
that you go through the trouble to get this.   It makes it much easier
to interact with the rest of the gcc community.  You can contact David
Edelsohn <edelsohn@...> for details on how to do this.

Thanks for the bug report and patch.   We appreciate the help.

Iant confirmed today via irc that the rtl that was generated in this private
port was legitimate.   I tested my patch on x86-64 to make sure that I
did not break anything.  

My patch was committed as revision 153924.

Kenny

> gcc-4.4.1, proprietary backend)
>
> Hi,
> It looks like fwprop does not propagate addresses that appear in
> zero_extract on the set dest.
> Here is an example:
>
> (insn 8 5 9 2 c:\mingw\home\amirg\test\test.c:157 (set (reg/f:HI 46)
>         (const:HI (plus:HI (symbol_ref:HI ("r") [flags 0x40] <var_decl
> 0x2ee40b0 r>)
>                 (const_int 68 [0x44])))) 18 {*movhi} (nil))
>
> (insn 9 8 0 2 c:\mingw\home\amirg\test\test.c:157 (set
> (zero_extract:SI (mem/s/j:QI (plus:HI (reg/f:HI 46)
>                     (const_int 2 [0x2])) [0 S1 A8])
>             (const_int 1 [0x1])
>             (const_int 1 [0x1]))
>         (const_int 0 [0x0])) 29 {*movb_lh} (nil))
>
> The problem seems to be in df_scan.c: df_uses_record doesn't handle
> the case of (set (zero_extract(mem(...) ...) ...).
> So I tried the following patch in df_uses_record under case SET: ...
> case ZERO_EXTRACT: ...
>
> --- df-scan.c
> +++ df-scan.c
> @@ -3152,11 +3152,24 @@
>    {
>      width = INTVAL (XEXP (dst, 1));
>      offset = INTVAL (XEXP (dst, 2));
> -    mode = GET_MODE (dst);
> +    mode = GET_MODE (dst);
> + /* AG start */
> + /* Handle the case of zero_extract(mem(...)) in the set dest */
> + if (GET_CODE (XEXP (dst,0)) == MEM)
> + {
> + df_uses_record (DF_REF_EXTRACT, collection_rec, &XEXP (XEXP (dst,0), 0),
> +  DF_REF_REG_MEM_STORE, bb, insn_info,
> +  DF_REF_ZERO_EXTRACT,
> +  width, offset, mode);
> + }
> + else
> + {
> + /* AG end */
>       df_uses_record (DF_REF_EXTRACT, collection_rec, &XEXP (dst, 0),
>   DF_REF_REG_USE, bb, insn_info,
>   DF_REF_READ_WRITE | DF_REF_ZERO_EXTRACT,
>   width, offset, mode);
> + } /* AG added closing brackets */
>    }
>   else
>    {
>
> and now fwprop propagates properly:
>
> (insn 9 5 0 2 c:\mingw\home\amirg\test\test.c:157 (set
> (zero_extract:SI (mem/s/j:QI (const:HI (plus:HI (symbol_ref:HI ("r")
> [flags 0x40] <var_decl 0x2ee40b0 r>)
>                         (const_int 70 [0x46]))) [0 S1 A8])
>             (const_int 1 [0x1])
>             (const_int 1 [0x1]))
>         (const_int 0 [0x0])) 29 {*movb_lh} (nil))
>
> I'll appreciate any comments,
> Thanks,
>
> Amir
>

===File ~/gcc/patches/dataflow/scanfix2.diff================
Index: gcc/df-scan.c
===================================================================
--- gcc/df-scan.c       (revision 153920)
+++ gcc/df-scan.c       (working copy)
@@ -3248,10 +3248,23 @@ df_uses_record (enum df_ref_class cl, st
                    width = INTVAL (XEXP (dst, 1));
                    offset = INTVAL (XEXP (dst, 2));
                    mode = GET_MODE (dst);
-                   df_uses_record (DF_REF_EXTRACT, collection_rec, &XEXP (dst, 0),
-                               DF_REF_REG_USE, bb, insn_info,
-                               DF_REF_READ_WRITE | DF_REF_ZERO_EXTRACT,
-                               width, offset, mode);
+                   if (GET_CODE (XEXP (dst,0)) == MEM)
+                     {
+                       /* Handle the case of zero_extract(mem(...)) in the set dest.
+                          This special case is allowed only if the mem is a single byte and
+                          is useful to set a bitfield in memory.  */
+                       df_uses_record (DF_REF_EXTRACT, collection_rec, &XEXP (XEXP (dst,0), 0),
+                                       DF_REF_REG_MEM_STORE, bb, insn_info,
+                                       DF_REF_ZERO_EXTRACT,
+                                       width, offset, mode);
+                     }
+                   else
+                     {
+                       df_uses_record (DF_REF_EXTRACT, collection_rec, &XEXP (dst, 0),
+                                       DF_REF_REG_USE, bb, insn_info,
+                                       DF_REF_READ_WRITE | DF_REF_ZERO_EXTRACT,
+                                       width, offset, mode);
+                     }
                  }
                else
                  {
============================================================

[Amir Gonnen-2]

Thanks Kenny.
My target does not support zero_extract with non-constant values in
the 2nd and 3rd field, so I can't really check this on my existing
code-base and regressions.
Anyway I suspect there is some problem with dse pass, after applying
that patch to df_scan.
It seems that zero_extract is not handled well and as a result dse
ignores the effect of stores with zero_extract on the lhs, and creates
wrong rtl.
Here is an example:

typedef struct _S
{
  short a: 14;
  short b: 1;
  short c: 1;
} S;

short g_short;

void f()
{
  short i=0;
  S *s = (S*) &i;
  s->a = 1;
  g_short = *(short*)s;
}

Here is dse pass dump:


;; Function f (f)

starting the processing of deferred insns
ending the processing of deferred insns
df_analyze called

**scanning insn=5
  mem: (plus:HI (reg/f:HI 35 fp)
    (const_int 4 [0x4]))

   after cselib_expand address: (plus:HI (reg/f:HI 35 fp)
    (const_int 4 [0x4]))

   after canon_rtx address: (plus:HI (reg/f:HI 35 fp)
    (const_int 4 [0x4]))
  gid=0 offset=4
 processing const base store gid=0[4..6)
mems_found = 1, cannot_delete = false

**scanning insn=6
mems_found = 0, cannot_delete = true

**scanning insn=7
  mem: (plus:HI (reg/f:HI 35 fp)
    (const_int 4 [0x4]))

   after cselib_expand address: (plus:HI (reg/f:HI 35 fp)
    (const_int 4 [0x4]))

   after canon_rtx address: (plus:HI (reg/f:HI 35 fp)
    (const_int 4 [0x4]))
  gid=0 offset=4
 processing const load gid=0[4..6)
trying to replace HImode load in insn 7 from HImode store in insn 5
deferring rescan insn with uid = 7.
deferring rescan insn with uid = 15.
 -- replaced the loaded MEM with (reg 43)
  mem: (symbol_ref:HI ("g_short") <var_decl 0x2ee40b0 g_short>)

   after cselib_expand address: (symbol_ref:HI ("g_short") <var_decl
0x2ee40b0 g_short>)

   after canon_rtx address: (symbol_ref:HI ("g_short") <var_decl
0x2ee40b0 g_short>)
  gid=1 offset=0
 processing const base store gid=1[0..2)
mems_found = 1, cannot_delete = false
Locally deleting insn 5
deferring deletion of insn with uid = 5.
group 0 is frame related group 0(0+2): n  p 4, 5
group 1(0+0): n  p
starting the processing of deferred insns
deleting insn with uid = 5.
rescanning insn with uid = 7.
deleting insn with uid = 7.
rescanning insn with uid = 15.
deleting insn with uid = 15.
ending the processing of deferred insns
df_analyze called
df_worklist_dataflow_doublequeue:n_basic_blocks 3 n_edges 2 count 3 (    1)


f

Dataflow summary:
def_info->table_size = 0, use_info->table_size = 8
;;  invalidated by call 32 [blink] 33 [cc] 37 [gccReturnValue]
;;  hardware regs used 34 [sp] 35 [fp] 36 [arg]
;;  regular block artificial uses 34 [sp] 35 [fp] 36 [arg]
;;  eh block artificial uses 34 [sp] 35 [fp] 36 [arg]
;;  entry block defs 34 [sp] 35 [fp] 36 [arg]
;;  exit block uses 34 [sp] 35 [fp]
;;  regs ever live
;;  ref usage r34={1d,2u} r35={1d,3u} r36={1d,1u} r43={1d,1u}
;;    total ref usage 11{4d,7u,0e} in 3{3 regular + 0 call} insns.

( )->[0]->( 2 )
;; bb 0 artificial_defs: { d0(34){ }d1(35){ }d2(36){ }}
;; bb 0 artificial_uses: { }
;; lr  in  
;; lr  use
;; lr  def 34 [sp] 35 [fp] 36 [arg]
;; live  in  
;; live  gen 34 [sp] 35 [fp] 36 [arg]
;; live  kill
;; lr  out 34 [sp] 35 [fp] 36 [arg]
;; live  out 34 [sp] 35 [fp] 36 [arg]

( 0 )->[2]->( 1 )
;; bb 2 artificial_defs: { }
;; bb 2 artificial_uses: { u0(34){ }u1(35){ }u2(36){ }}
;; lr  in   34 [sp] 35 [fp] 36 [arg]
;; lr  use 34 [sp] 35 [fp] 36 [arg]
;; lr  def 43
;; live  in   34 [sp] 35 [fp] 36 [arg]
;; live  gen
;; live  kill
;; lr  out 34 [sp] 35 [fp] 36 [arg]
;; live  out 34 [sp] 35 [fp] 36 [arg]

( 2 )->[1]->( )
;; bb 1 artificial_defs: { }
;; bb 1 artificial_uses: { u6(34){ }u7(35){ }}
;; lr  in   34 [sp] 35 [fp]
;; lr  use 34 [sp] 35 [fp]
;; lr  def
;; live  in   34 [sp] 35 [fp]
;; live  gen
;; live  kill
;; lr  out
;; live  out

Finding needed instructions:
  Adding insn 7 to worklist
  Adding insn 6 to worklist
Finished finding needed instructions:
processing block 2 lr out =  34 [sp] 35 [fp] 36 [arg]
  Adding insn 15 to worklist
df_worklist_dataflow_doublequeue:n_basic_blocks 3 n_edges 2 count 4 (  1.3)
doing global processing
df_worklist_dataflow_doublequeue:n_basic_blocks 3 n_edges 2 count 5 (  1.7)


*** Global dataflow info after analysis.

( )->[0]->( 2 )
  in:
  gen:
  kill: *MISSING*
  out:  1, 2

( 0 )->[2]->( 1 )
  in:   1, 2
  gen:
  kill:
  out:  1, 2

( 2 )->[1]->( )
  in:   1, 2
  gen:  1, 2
  kill: *MISSING*
  out:  *MISSING*

starting to process insn 7
  v:  1, 2
i = 0, index = 0
failing at i = 0
starting to process insn 6
  v:  1, 2
dse: local deletions = 1, global deletions = 0, spill deletions = 0
(note 3 0 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)

(note 2 3 15 2 NOTE_INSN_FUNCTION_BEG)

(insn 15 2 6 2 c:\mingw\home\amirg\test\test.c:12 (set (reg:HI 43)
        (const_int 0 [0x0])) -1 (nil))

(insn 6 15 7 2 c:\mingw\home\amirg\test\test.c:14 (set
(zero_extract:SI (mem/s/j:QI (plus:HI (reg/f:HI 35 fp)
                    (const_int 4 [0x4])) [0 S1 A8])
            (const_int 14 [0xe])
            (const_int 0 [0x0]))
        (const_int 1 [0x1])) 29 {*movb_lh} (nil))

(insn 7 6 0 2 c:\mingw\home\amirg\test\test.c:15 (set (mem/c/i:HI
(symbol_ref:HI ("g_short") <var_decl 0x2ee40b0 g_short>) [0 g_short+0
S2 A16])
        (reg:HI 43)) 18 {*movhi} (expr_list:REG_DEAD (reg:HI 43)
        (nil)))
starting the processing of deferred insns
ending the processing of deferred insns


Note that in insn 7, (mem (plus (reg fp) (const_int 4)) was replaced
by (reg 43), regardless of the fact that the same mem is updated (in
zero_extract) on insn 6.
I'm not sure how to actually fix the problem, but I was able to work
around it by treating zero_extract as wild store in record_store in
dse.c:

--- dse.c
+++ dse.c
@@ -1301,6 +1301,16 @@
     return 0;

   mem = SET_DEST (body);
+
+
+  /* bail out in case of ZERO_EXTRACT */
+  if (GET_CODE(mem) == ZERO_EXTRACT)
+  {
+ add_wild_read (bb_info);
+ insn_info->cannot_delete = true;
+ return 0;
+  }
+

   /* If this is not used, then this cannot be used to keep the insn
      from being deleted.  On the other hand, it does provide something


Then the same pass dump looks better:


;; Function f (f)

starting the processing of deferred insns
ending the processing of deferred insns
df_analyze called

**scanning insn=5
  mem: (plus:HI (reg/f:HI 35 fp)
    (const_int 4 [0x4]))

   after cselib_expand address: (plus:HI (reg/f:HI 35 fp)
    (const_int 4 [0x4]))

   after canon_rtx address: (plus:HI (reg/f:HI 35 fp)
    (const_int 4 [0x4]))
  gid=0 offset=4
 processing const base store gid=0[4..6)
mems_found = 1, cannot_delete = false

**scanning insn=6
mems_found = 0, cannot_delete = true

**scanning insn=7
  mem: (plus:HI (reg/f:HI 35 fp)
    (const_int 4 [0x4]))

   after cselib_expand address: (plus:HI (reg/f:HI 35 fp)
    (const_int 4 [0x4]))

   after canon_rtx address: (plus:HI (reg/f:HI 35 fp)
    (const_int 4 [0x4]))
  gid=0 offset=4
 processing const load gid=0[4..6)
  mem: (symbol_ref:HI ("g_short") <var_decl 0x2ee40b0 g_short>)

   after cselib_expand address: (symbol_ref:HI ("g_short") <var_decl
0x2ee40b0 g_short>)

   after canon_rtx address: (symbol_ref:HI ("g_short") <var_decl
0x2ee40b0 g_short>)
  gid=1 offset=0
 processing const base store gid=1[0..2)
mems_found = 1, cannot_delete = false
group 0 is frame related group 0(0+2): n  p 4, 5
group 1(0+0): n  p
starting the processing of deferred insns
ending the processing of deferred insns
df_analyze called
doing global processing
df_worklist_dataflow_doublequeue:n_basic_blocks 3 n_edges 2 count 5 (  1.7)


*** Global dataflow info after analysis.

( )->[0]->( 2 )
  in:
  gen:
  kill: *MISSING*
  out:  1, 2

( 0 )->[2]->( 1 )
  in:   1, 2
  gen:  1, 2
  kill: *MISSING*
  out:  1, 2

( 2 )->[1]->( )
  in:   1, 2
  gen:  1, 2
  kill: *MISSING*
  out:  *MISSING*

starting to process insn 7
  v:  1, 2
i = 0, index = 0
failing at i = 0
regular read
starting to process insn 6
  v:
wild read
starting to process insn 5
  v:
dse: local deletions = 0, global deletions = 0, spill deletions = 0
(note 3 0 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)

(note 2 3 5 2 NOTE_INSN_FUNCTION_BEG)

(insn 5 2 6 2 c:\mingw\home\amirg\test\test.c:12 (set (mem/c/i:HI
(plus:HI (reg/f:HI 35 fp)
                (const_int 4 [0x4])) [0 i+0 S2 A32])
        (const_int 0 [0x0])) 18 {*movhi} (nil))

(insn 6 5 7 2 c:\mingw\home\amirg\test\test.c:14 (set (zero_extract:SI
(mem/s/j:QI (plus:HI (reg/f:HI 35 fp)
                    (const_int 4 [0x4])) [0 S1 A8])
            (const_int 14 [0xe])
            (const_int 0 [0x0]))
        (const_int 1 [0x1])) 29 {*movb_lh} (nil))

(insn 7 6 0 2 c:\mingw\home\amirg\test\test.c:15 (set (mem/c/i:HI
(symbol_ref:HI ("g_short") <var_decl 0x2ee40b0 g_short>) [0 g_short+0
S2 A16])
        (mem/c/i:HI (plus:HI (reg/f:HI 35 fp)
                (const_int 4 [0x4])) [0 i+0 S2 A32])) 18 {*movhi} (nil))
starting the processing of deferred insns
ending the processing of deferred insns


Amir.


On Thu, Nov 5, 2009 at 1:34 AM, Kenneth Zadeck
<Kenneth.Zadeck@...> wrote:

> Amir,
>
> I tested and committed a slightly different and cleaned up version of
> this patch with a proper changelog.  It would be good if you reported
> back the results of request by Paolo in case that shows any problems.
>
> We unfortunately cannot directly accept patches from people that do
> not have a gcc/gnu copyright assignment.    We would strongly recommend
> that you go through the trouble to get this.   It makes it much easier
> to interact with the rest of the gcc community.  You can contact David
> Edelsohn <edelsohn@...> for details on how to do this.
>
> Thanks for the bug report and patch.   We appreciate the help.
>
> Iant confirmed today via irc that the rtl that was generated in this private
> port was legitimate.   I tested my patch on x86-64 to make sure that I
> did not break anything.
>
> My patch was committed as revision 153924.
>
> Kenny
>
>
>
>
>> gcc-4.4.1, proprietary backend)
>>
>> Hi,
>> It looks like fwprop does not propagate addresses that appear in
>> zero_extract on the set dest.
>> Here is an example:
>>
>> (insn 8 5 9 2 c:\mingw\home\amirg\test\test.c:157 (set (reg/f:HI 46)
>>         (const:HI (plus:HI (symbol_ref:HI ("r") [flags 0x40] <var_decl
>> 0x2ee40b0 r>)
>>                 (const_int 68 [0x44])))) 18 {*movhi} (nil))
>>
>> (insn 9 8 0 2 c:\mingw\home\amirg\test\test.c:157 (set
>> (zero_extract:SI (mem/s/j:QI (plus:HI (reg/f:HI 46)
>>                     (const_int 2 [0x2])) [0 S1 A8])
>>             (const_int 1 [0x1])
>>             (const_int 1 [0x1]))
>>         (const_int 0 [0x0])) 29 {*movb_lh} (nil))
>>
>> The problem seems to be in df_scan.c: df_uses_record doesn't handle
>> the case of (set (zero_extract(mem(...) ...) ...).
>> So I tried the following patch in df_uses_record under case SET: ...
>> case ZERO_EXTRACT: ...
>>
>> --- df-scan.c
>> +++ df-scan.c
>> @@ -3152,11 +3152,24 @@
>>                 {
>>                   width = INTVAL (XEXP (dst, 1));
>>                   offset = INTVAL (XEXP (dst, 2));
>> -                 mode = GET_MODE (dst);
>> +                 mode = GET_MODE (dst);
>> +                     /* AG start */
>> +                     /* Handle the case of zero_extract(mem(...)) in the set dest */
>> +                     if (GET_CODE (XEXP (dst,0)) == MEM)
>> +                     {
>> +                     df_uses_record (DF_REF_EXTRACT, collection_rec, &XEXP (XEXP (dst,0), 0),
>> +                               DF_REF_REG_MEM_STORE, bb, insn_info,
>> +                               DF_REF_ZERO_EXTRACT,
>> +                               width, offset, mode);
>> +                     }
>> +                     else
>> +                     {
>> +                     /* AG end */
>>                       df_uses_record (DF_REF_EXTRACT, collection_rec, &XEXP (dst, 0),
>>                               DF_REF_REG_USE, bb, insn_info,
>>                               DF_REF_READ_WRITE | DF_REF_ZERO_EXTRACT,
>>                               width, offset, mode);
>> +                     } /* AG added closing brackets */
>>                 }
>>               else
>>                 {
>>
>> and now fwprop propagates properly:
>>
>> (insn 9 5 0 2 c:\mingw\home\amirg\test\test.c:157 (set
>> (zero_extract:SI (mem/s/j:QI (const:HI (plus:HI (symbol_ref:HI ("r")
>> [flags 0x40] <var_decl 0x2ee40b0 r>)
>>                         (const_int 70 [0x46]))) [0 S1 A8])
>>             (const_int 1 [0x1])
>>             (const_int 1 [0x1]))
>>         (const_int 0 [0x0])) 29 {*movb_lh} (nil))
>>
>> I'll appreciate any comments,
>> Thanks,
>>
>> Amir
>>
>
> ===File ~/gcc/patches/dataflow/scanfix2.diff================
> Index: gcc/df-scan.c
> ===================================================================
> --- gcc/df-scan.c       (revision 153920)
> +++ gcc/df-scan.c       (working copy)
> @@ -3248,10 +3248,23 @@ df_uses_record (enum df_ref_class cl, st
>                    width = INTVAL (XEXP (dst, 1));
>                    offset = INTVAL (XEXP (dst, 2));
>                    mode = GET_MODE (dst);
> -                   df_uses_record (DF_REF_EXTRACT, collection_rec, &XEXP (dst, 0),
> -                               DF_REF_REG_USE, bb, insn_info,
> -                               DF_REF_READ_WRITE | DF_REF_ZERO_EXTRACT,
> -                               width, offset, mode);
> +                   if (GET_CODE (XEXP (dst,0)) == MEM)
> +                     {
> +                       /* Handle the case of zero_extract(mem(...)) in the set dest.
> +                          This special case is allowed only if the mem is a single byte and
> +                          is useful to set a bitfield in memory.  */
> +                       df_uses_record (DF_REF_EXTRACT, collection_rec, &XEXP (XEXP (dst,0), 0),
> +                                       DF_REF_REG_MEM_STORE, bb, insn_info,
> +                                       DF_REF_ZERO_EXTRACT,
> +                                       width, offset, mode);
> +                     }
> +                   else
> +                     {
> +                       df_uses_record (DF_REF_EXTRACT, collection_rec, &XEXP (dst, 0),
> +                                       DF_REF_REG_USE, bb, insn_info,
> +                                       DF_REF_READ_WRITE | DF_REF_ZERO_EXTRACT,
> +                                       width, offset, mode);
> +                     }
>                  }
>                else
>                  {
> ============================================================
>

[Paolo Bonzini-6]

On Thu, Nov 5, 2009 at 16:28, Amir Gonnen <amirgonnen@...> wrote:
> Thanks Kenny.
> My target does not support zero_extract with non-constant values in
> the 2nd and 3rd field, so I can't really check this on my existing
> code-base and regressions.
> Anyway I suspect there is some problem with dse pass, after applying
> that patch to df_scan.
> It seems that zero_extract is not handled well and as a result dse
> ignores the effect of stores with zero_extract on the lhs, and creates
> wrong rtl.

So if I understand correctly the patch changes a missed optimization
bug to wrong code?

Paolo

[Amir Gonnen-2]

At first I though the df_scan patch unveiled this bug, but now I see
this bug happens regardless.
Even without the patch, once my code generates zero_extract on the set
destination, dse creates wrong code.

Amir

> So if I understand correctly the patch changes a missed optimization
> bug to wrong code?
>
> Paolo
>

[Kenneth Zadeck]

Amir Gonnen wrote:

> At first I though the df_scan patch unveiled this bug, but now I see
> this bug happens regardless.
> Even without the patch, once my code generates zero_extract on the set
> destination, dse creates wrong code.
>
> Amir
>
>  
>> So if I understand correctly the patch changes a missed optimization
>> bug to wrong code?
>>
>> Paolo
>>
>>    

The main reason that this was missed was that there are no public,
active ports that actually do this.  I accepted the patch on Ian
Taylor's (iant) advice because he said that it was legal as long as the
mem was a single byte.   Iant claimed that the last machine that could
do this kind of thing was the 68k but in his comments he did not
actually say if the gcc port actually did it.   Thus, you are really on
your own as to finding out how much else is broken to support this feature.

When we test new passes, and I admit to being the person who wrote much
of dse, we test them on the active ports and we fix bugs against those
ports.   But a project like gcc is really a use it or loose it kind of
system.   The features that are exercised on the public active ports
work, and the features that are not exercised tend to rot, generally
until someone adds a new port that utilizes the feature.

I think that the policy is that we accept patches from private ports as
long as they are fix problems with things that are supposed to work.  
Certainly no one has pushed back on me for accepting your first patch.  

Kenny

[Amir Gonnen-2]

Ok, I accept the fact I'm on my own here :) at least regarding
zero_extract on set destination.
Let me try and revive this feature so that if any port in the future
use it, it would still work.

So dse does not handle lhs zero_extract, but it's still possible to
work around the problem by treating such zero_extract as wild read in
record_store in
dse.c:

--- dse.c
+++ dse.c
@@ -1301,6 +1301,16 @@
     return 0;

   mem = SET_DEST (body);
+
+
+  /* bail out in case of ZERO_EXTRACT */
+  if (GET_CODE(mem) == ZERO_EXTRACT)
+  {
+ add_wild_read (bb_info);
+ insn_info->cannot_delete = true;
+ return 0;
+  }
+

   /* If this is not used, then this cannot be used to keep the insn
      from being deleted.  On the other hand, it does provide something

What do you think?


On Thu, Nov 5, 2009 at 6:50 PM, Kenneth Zadeck <zadeck@...> wrote:

> The main reason that this was missed was that there are no public,
> active ports that actually do this.  I accepted the patch on Ian
> Taylor's (iant) advice because he said that it was legal as long as the
> mem was a single byte.   Iant claimed that the last machine that could
> do this kind of thing was the 68k but in his comments he did not
> actually say if the gcc port actually did it.   Thus, you are really on
> your own as to finding out how much else is broken to support this feature.
>
> When we test new passes, and I admit to being the person who wrote much
> of dse, we test them on the active ports and we fix bugs against those
> ports.   But a project like gcc is really a use it or loose it kind of
> system.   The features that are exercised on the public active ports
> work, and the features that are not exercised tend to rot, generally
> until someone adds a new port that utilizes the feature.
>
> I think that the policy is that we accept patches from private ports as
> long as they are fix problems with things that are supposed to work.
> Certainly no one has pushed back on me for accepting your first patch.
>
> Kenny
>

[Kenneth Zadeck]

Amir Gonnen wrote:

> Ok, I accept the fact I'm on my own here :) at least regarding
> zero_extract on set destination.
> Let me try and revive this feature so that if any port in the future
> use it, it would still work.
>
> So dse does not handle lhs zero_extract, but it's still possible to
> work around the problem by treating such zero_extract as wild read in
> record_store in
> dse.c:
>
> --- dse.c
> +++ dse.c
> @@ -1301,6 +1301,16 @@
>      return 0;
>
>    mem = SET_DEST (body);
> +
> +
> +  /* bail out in case of ZERO_EXTRACT */
> +  if (GET_CODE(mem) == ZERO_EXTRACT)
> +  {
> + add_wild_read (bb_info);
> + insn_info->cannot_delete = true;
> + return 0;
> +  }
> +
>
>    /* If this is not used, then this cannot be used to keep the insn
>       from being deleted.  On the other hand, it does provide something
>
> What do you think?
>  

i think that something like that should work.

kenny

>
> On Thu, Nov 5, 2009 at 6:50 PM, Kenneth Zadeck <zadeck@...> wrote:
>  
>> The main reason that this was missed was that there are no public,
>> active ports that actually do this.  I accepted the patch on Ian
>> Taylor's (iant) advice because he said that it was legal as long as the
>> mem was a single byte.   Iant claimed that the last machine that could
>> do this kind of thing was the 68k but in his comments he did not
>> actually say if the gcc port actually did it.   Thus, you are really on
>> your own as to finding out how much else is broken to support this feature.
>>
>> When we test new passes, and I admit to being the person who wrote much
>> of dse, we test them on the active ports and we fix bugs against those
>> ports.   But a project like gcc is really a use it or loose it kind of
>> system.   The features that are exercised on the public active ports
>> work, and the features that are not exercised tend to rot, generally
>> until someone adds a new port that utilizes the feature.
>>
>> I think that the policy is that we accept patches from private ports as
>> long as they are fix problems with things that are supposed to work.
>> Certainly no one has pushed back on me for accepting your first patch.
>>
>> Kenny
>>
>>    

[Amir Gonnen-2]

It works for me.
What is the process of submitting that patch?

The benefits would be:
1. A future port will be able to use zero_extract on set dest.
2. I will not need to re-apply these patches every time I update my
port to a newer gcc version.

Thanks,

Amir

>
> i think that something like that should work.
>
> kenny

[Kenneth Zadeck]

Amir Gonnen wrote:
> It works for me.
> What is the process of submitting that patch?
>
>  
you do much the same way as you did the first time.     It will help if
you get the assignment because, the free software foundation has to own
all of the code in the compiler.   The assignment transfers ownership of
the patch to the fsf.    Otherwise someone has to "rewrite the patch"
unless it is under some specific size.   We generally do not like to do
this very often, so there will be increasing pressure to get the
assignment assuming you plan to be an active contributor.

But there are generally 5 things required for a patch:
1) The patch.  "diff -up" is generally what people use.
2) A justification.
3) A ChangeLog entry.   See gcc/ChangeLog for examples.   There are very
terse.
4) A test case.   This is problematic in your case, since your test
cases will not trigger problems in any of the public ports.
5) Some indication that the patch has been tested.   At the least we
would like to see that the patch causes no damage to at least one of the
public ports.   This should be done by building a compiler both with and
without the patch and checking that the test results do not change (or
justifying the change).  Note that I did this for my version of your
patch before checking it in.

Assuming you get the assignment, you will be allowed to check in the
patch yourself after it has been approved by the proper maintainer.  
There are guidelines and coding standards that must be followed.  
people are very picky about this.

Kenny
> The benefits would be:
> 1. A future port will be able to use zero_extract on set dest.
> 2. I will not need to re-apply these patches every time I update my
> port to a newer gcc version.
>  
also we will not have to rewrite the patch if we like it.
> Thanks,
>
> Amir
>
>  
>> i think that something like that should work.
>>
>> kenny
>>    

[Paolo Bonzini-6]

> It will help if
> you get the assignment because, the free software foundation has to own
> all of the code in the compiler.   The assignment transfers ownership of
> the patch to the fsf.    Otherwise someone has to "rewrite the patch"
> unless it is under some specific size.

Rewriting the patch almost never happens, the patch is usually left to
its fate until the assignment is completed.  We generally accept
without assignment up to 15 lines, cumulative across all patches
submitted by the same person, with some leeway for trivial changes
such as reindentation (but not much more than that)

> 4) A test case.   This is problematic in your case, since your test
> cases will not trigger problems in any of the public ports.

I guess we can live without a testcase.

Paolo

[Kenneth Zadeck]

Paolo Bonzini wrote:

>> It will help if
>> you get the assignment because, the free software foundation has to own
>> all of the code in the compiler.   The assignment transfers ownership of
>> the patch to the fsf.    Otherwise someone has to "rewrite the patch"
>> unless it is under some specific size.
>>    
>
> Rewriting the patch almost never happens, the patch is usually left to
> its fate until the assignment is completed.  We generally accept
> without assignment up to 15 lines, cumulative across all patches
> submitted by the same person, with some leeway for trivial changes
> such as reindentation (but not much more than that)
>
>  

you are correct,   i am in a mode where if i do not get things off my
desk quickly they just rot forever.  
>> 4) A test case.   This is problematic in your case, since your test
>> cases will not trigger problems in any of the public ports.
>>    
>
> I guess we can live without a testcase.
>
> Paolo
>  

[Jeff Law]

On 11/05/09 09:50, Kenneth Zadeck wrote:
>
> The main reason that this was missed was that there are no public,
> active ports that actually do this.  I accepted the patch on Ian
> Taylor's (iant) advice because he said that it was legal as long as the
> mem was a single byte.   Iant claimed that the last machine that could
> do this kind of thing was the 68k but in his comments he did not
> actually say if the gcc port actually did it.   Thus, you are really on
> your own as to finding out how much else is broken to support this feature.
>    
I believe the H8 port generates

(set (zero_extract (mem)))

in certain circumstances.    Unfortunately, limitations of GCC's
insv/extv support make it much more difficult than it should be to
generate these kinds of insn on the H8 series processors.

> I think that the policy is that we accept patches from private ports as
> long as they are fix problems with things that are supposed to work.
> Certainly no one has pushed back on me for accepting your first patch.
Right.  Obviously with a private port it usually takes a more thorough
description of the problem as the review isn't able to fire up a
debugger if they needed to understand how a particular hunk code was
behaving.

jeff