On 6/27/19 9:00 AM, Jeff Law wrote:
> On 6/26/19 8:40 PM, Martin Sebor wrote:
>> On 6/26/19 4:31 PM, Jeff Law wrote:
>>> On 6/25/19 5:03 PM, Martin Sebor wrote:
>>>
>>>>
>>>> The caller ensures that handle_char_store is only called for stores
>>>> to arrays (MEM_REF) or single elements as wide as char.
>>> Where? I don't see it, even after fixing the formatting in
>>> strlen_check_and_optimize_stmt :-)
>>>
>>>> gimple *stmt = gsi_stmt (*gsi);
>>>>
>>>> if (is_gimple_call (stmt))
>>>
>>> I think we can agree that we don't have a call, so this is false.
>>>
>>>> else if (is_gimple_assign (stmt) && !gimple_clobber_p (stmt))
>>>> {
>>>> tree lhs = gimple_assign_lhs (stmt);
>>> This should be true IIUC, so we'll go into its THEN block.
>>>
>>>
>>>> if (TREE_CODE (lhs) == SSA_NAME && POINTER_TYPE_P (TREE_TYPE
>>>> (lhs)))
>>> Should be false.
>>>
>>>> else if (TREE_CODE (lhs) == SSA_NAME && INTEGRAL_TYPE_P
>>>> (TREE_TYPE (lhs)))
>>>
>>> Should also be false.
>>>
>>>> else if (TREE_CODE (lhs) != SSA_NAME && !TREE_SIDE_EFFECTS (lhs))
>>> Should be true since LHS is a MEM_REF.
>>>
>>>
>>>> {
>>>> tree type = TREE_TYPE (lhs);
>>>> if (TREE_CODE (type) == ARRAY_TYPE)
>>>> type = TREE_TYPE (type);
>>>> if (TREE_CODE (type) == INTEGER_TYPE
>>>> && TYPE_MODE (type) == TYPE_MODE (char_type_node)
>>>> && TYPE_PRECISION (type) == TYPE_PRECISION
>>>> (char_type_node))
>>>> {
>>>> if (! handle_char_store (gsi))
>>>> return false;
>>>> }
>>>> }
>>> If TREE_TYPE (type) is an ARRAY_TYPE, we strip the ARRAY_TYPE. We then
>>> verify that TYPE is a single character type. _But_ we stripped away the
>>> ARRAY_TYPE. So ISTM that we allow either an array of chars or a single
>>> char on the LHS.
>>>
>>> So how does this avoid multiple character stores?!? We could have had
>>> an ARRAY_REF on the LHS and we never check the number of elements in the
>>> array. There's no check on the RHS either. SO I don't see how we
>>> guarantee that we're dealing with a single character store.
>>>
>>> What am I missing here?
>>
>> Can you show me a test case where it breaks? If not, I don't know
>> what you want me to do. I'll just move on to something else.
> THe thing to do is research what gimple accepts and what other passes
> may do. Given this is a code generation bug, "just moving on" isn't
> really a good option unless we're going to rip out that little bit of code.
>
> As I was thinking about this last night, the pass we'd want to look at
> would be store merging. Let's do that on the call today.
Actually it was trivial to create with store merging.
char x[20];
foo()
{
x[0] = 0x41;
x[1] = 0x42;
}
MEM <unsigned short> [(char *)&x] = 16961;
So clearly we can get this in gimple. So we need a check of some kind,
either on the LHS or the RHS to ensure that we actually have a single
character store as opposed to something like the above.
jeff
