The gimple for test2_31 before RTL expansion contains:
# .MEMD.2034_2 = VDEF <.MEMD.2034_1(D)>
s1D.2035 = s1D.1255;
The RHS is a PARM_DECL. It doesn't have TREE_ADDRESSABLE and the LHS
has, which makes sense. But then the move is translated to memcpy but
the src memory location is still considered not addressable. Setting
TREE_ADDRESSABLE to RHS should fix this bug. I don't know which is the
best place to that though. At the point when it decides to use memcpy
to do the copying in emit_block_move_hints, we are dealing with
temporaries and we don't have the src TREE lying around. Is passing
the tree EXP to emit_block_move from store_expr a reasonable approach?
-Easwaran
On Fri, Jun 17, 2011 at 2:16 PM, Easwaran Raman <era...@google.com> wrote:
> This patch seems to break ia64 and some other targets. I have updated
> Bug 49429 with a test case triage. It looks like for some EXPR,
> may_be_aliased returns incorrect value and hence can_escape
> incorrectly concludes the variable can't escape resulting in removal
> of useful stores.
>
>
> (So can_escape() returns false. But later on, in the same BB, I see:
>
> In the following list of instructions,
>
> (insn 4 3 6 2 (set (mem/s/c:DI (reg/f:DI 341) [2 s1+0 S8 A64])
> (reg:DI 112 in0)) y.c:23 5 {movdi_internal}
> (expr_list:REG_DEAD (reg:DI 112 in0)
> (nil)))
>
> ...
>
> (insn 36 30 37 2 (set (reg:DI 120 out0)
> (reg/f:DI 357)) 5 {movdi_internal}
> (expr_list:REG_EQUAL (plus:DI (reg/f:DI 328 sfp)
> (const_int 62 [0x3e]))
> (nil)))
> (insn 37 36 38 2 (set (reg:DI 121 out1)
> (reg/f:DI 341)) 5 {movdi_internal}
> (expr_list:REG_DEAD (reg/f:DI 341)
> (expr_list:REG_EQUAL (plus:DI (reg/f:DI 328 sfp)
> (const_int 96 [0x60]))
> (nil))))
> (insn 38 37 39 2 (set (reg:DI 122 out2)
> (const_int 31 [0x1f])) 5 {movdi_internal}
> (nil))
> (call_insn 39 38 42 2 (parallel [
> (set (reg:DI 8 r8)
> (call (mem:DI (symbol_ref:DI ("memcpy") [flags 0x41]
> <function_decl 0x7ffff70d2e00 memcpy>) [0 memcpy S8 A64])
> (const_int 1 [0x1])))
> (clobber (reg:DI 320 b0))
> (clobber (scratch:DI))
> (clobber (scratch:DI))
> ]) 332 {call_value_gp}
> (expr_list:REG_DEAD (reg:DI 122 out2)
> (expr_list:REG_DEAD (reg:DI 121 out1)
> (expr_list:REG_DEAD (reg:DI 120 out0)
> (expr_list:REG_UNUSED (reg:DI 8 r8)
> (expr_list:REG_EH_REGION (const_int 0 [0])
> (nil))))))
> (expr_list:REG_DEP_TRUE (use (reg:DI 1 r1))
> (expr_list:REG_DEP_TRUE (use (reg:DI 122 out2))
> (expr_list:REG_DEP_TRUE (use (reg:DI 121 out1))
> (expr_list:REG_DEP_TRUE (use (reg:DI 120 out0))
> (nil))))))
>
>
> for the memory expression
>
> (set (mem/s/c:DI (reg/f:DI 341) [2 s1+0 S8 A64])
> (reg:DI 112 in0))
>
> may_be_aliased() returns false, but register 341 is passed as the
> second parameter to memcpy. I suspect this is a bug elsewhere which is
> exposed by this patch. If someone knows why this might be happening, I
> can tighten the can_escape() function appropriately.
>
> Thanks,
> Easwaran
>
>
>
> On Tue, Jun 14, 2011 at 9:34 AM, Jeff Law <l...@redhat.com> wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 05/10/11 13:18, Easwaran Raman wrote:
>>
>>>>> I am not sure I understand the problem here. If there is a wild read
>>>>> from asm, the instruction has the wild_read flag set. The if statement
>>>>> checks if that flag is set and if so it clears the bitmap - which was
>>>>> the original behavior. Originally, only if read_rec is non NULL you
>>>>> need to recompute the kill set. Now, even if read_rec is NULL,
>>>>> non_frame_wild_read could be set requiring the kill set to be
>>>>> modified, which is what this patch does. In fact, isn't what you have
>>>>> written above the equivalent to what is in the patch as '/* Leave this
>>>>> clause unchanged */' is the same as
>>>>>
>>>>> if (dump_file)
>>>>> fprintf (dump_file, "regular read\n");
>>>>> scan_reads_nospill (insn_info, v, NULL);
>>>>>
>>>>>
>>>>> -Easwaran
>>>>>
>>>
>>>> Ping. I have changed the test case to use int and added another test
>>>> case that shows DSE doesn't happen when the struct instance is
>>>> volatile (wild_read gets set in that case)
>>>
>>>
>>> What's the purpose behind using unit64_t in the testcase? Somehow I
>>> suspect using int64_t means the test is unlikely not going to work
>>> across targets with different word sizes.
>> Sorry for the exceedingly long wait. Things have been a bit crazy the
>> last several weeks.
>>
>> On a positive note, re-reading things now I think my objection/comment
>> was mis-guided.
>>
>> Patch approved, and again, sorry for the absurdly long period of
>> non-responsiveness.
>>
>> jeff
>> -----BEGIN PGP SIGNATURE-----
>> Version: GnuPG v1.4.11 (GNU/Linux)
>> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>>
>> iQEcBAEBAgAGBQJN942IAAoJEBRtltQi2kC7Fj4IAIUvXsEKHZEKHS2k/psJWyaM
>> Uo/vW3CLydRP0+Np/VVSwzHlmWfdWmOj1WPw1Svhvr4gP8BrZ13okVv5jbw1Hh3Y
>> R4mShXFK5eYzmGx5wL54hOze5zViN3gomNGbDAAhk6TCzNXmPyLT/6V1tLFTNhD5
>> 6zOiW8pXh9ik6qTTCKbG0EMuJXDnIbYrJs4d/gHFerUgmRPc8adKjF3PCngD3F4r
>> 40n9W/UxUejYUddavDW1fIdALWYc56F3glplFsII7SMnOmih8MTFYOvk6SZsLS5O
>> G2nzmnUuwt6tPWTyk9bpVKQi5dn8MmLkM13w22t36GKIg6OER2KfUdv44dgE7yw=
>> =o7AI
>> -----END PGP SIGNATURE-----
>>
>
