On Sat, Jun 18, 2011 at 9:03 PM, Easwaran Raman <era...@google.com> wrote:
> 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?
Well, this is the kind of issues you can run into when gimple gets lowered
to RTL (I can imagine structs passed by value on a callee-copy arch
will fail similarly - they'd get passed by reference and you wouldn't see
the callee-copy).
Until now we didn't try to disambiguate anything against calls, so the
issue pops up only now. But yes, eventually calling mark_addressable
on the trees that get their address exposed to function calls is a good
way to avoid this. Longer term we want to expose such details by
lowering them early during gimple.
Richard.
> -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-----
>>>
>>
>
