On Tue, Jan 9, 2018 at 3:39 PM, Richard Sandiford
<richard.sandif...@linaro.org> wrote:
> Richard Biener <richard.guent...@gmail.com> writes:
>> On Tue, Nov 7, 2017 at 7:04 PM, Richard Sandiford
>> <richard.sandif...@linaro.org> wrote:
>>> Richard Biener <richard.guent...@gmail.com> writes:
>>>> On Fri, Nov 3, 2017 at 5:32 PM, Richard Sandiford
>>>> <richard.sandif...@linaro.org> wrote:
>>>>> A general TARGET_MEM_REF is:
>>>>>
>>>>> BASE + STEP * INDEX + INDEX2 + OFFSET
>>>>>
>>>>> After classifying the address in this way, the code that builds
>>>>> TARGET_MEM_REFs tries to simplify the address until it's valid
>>>>> for the current target and for the mode of memory being addressed.
>>>>> It does this in a fixed order:
>>>>>
>>>>> (1) add SYMBOL to BASE
>>>>> (2) add INDEX * STEP to the base, if STEP != 1
>>>>> (3) add OFFSET to INDEX or BASE (reverted if unsuccessful)
>>>>> (4) add INDEX to BASE
>>>>> (5) add OFFSET to BASE
>>>>>
>>>>> So suppose we had an address:
>>>>>
>>>>> &symbol + offset + index * 8 (e.g. "a[i + 1]" for a global "a")
>>>>>
>>>>> on a target that only allows an index or an offset, not both. Following
>>>>> the steps above, we'd first create:
>>>>>
>>>>> tmp = symbol
>>>>> tmp2 = tmp + index * 8
>>>>>
>>>>> Then if the given offset value was valid for the mode being addressed,
>>>>> we'd create:
>>>>>
>>>>> MEM[base:tmp2, offset:offset]
>>>>>
>>>>> while if it was invalid we'd create:
>>>>>
>>>>> tmp3 = tmp2 + offset
>>>>> MEM[base:tmp3, offset:0]
>>>>>
>>>>> The problem is that this could happen if ivopts had decided to use
>>>>> a scaled index for an address that happens to have a constant base.
>>>>> The old procedure failed to give an indexed TARGET_MEM_REF in that case,
>>>>> and adding the offset last prevented later passes from being able to
>>>>> fold the index back in.
>>>>>
>>>>> The patch avoids this by skipping (2) if BASE + INDEX * STEP
>>>>> is a legitimate address and if OFFSET is stopping the address
>>>>> being valid.
>>>>>
>>>>> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu.
>>>>> OK to install?
>>>>>
>>>>> Richard
>>>>>
>>>>>
>>>>> 2017-10-31 Richard Sandiford <richard.sandif...@linaro.org>
>>>>> Alan Hayward <alan.hayw...@arm.com>
>>>>> David Sherwood <david.sherw...@arm.com>
>>>>>
>>>>> gcc/
>>>>> * tree-ssa-address.c (keep_index_p): New function.
>>>>> (create_mem_ref): Use it. Only split out the INDEX * STEP
>>>>> component if that is invalid even with the symbol and offset
>>>>> removed.
>>>>>
>>>>> Index: gcc/tree-ssa-address.c
>>>>> ===================================================================
>>>>> --- gcc/tree-ssa-address.c 2017-11-03 12:15:44.097060121 +0000
>>>>> +++ gcc/tree-ssa-address.c 2017-11-03 12:21:18.060359821 +0000
>>>>> @@ -746,6 +746,20 @@ gimplify_mem_ref_parts (gimple_stmt_iter
>>>>> true, GSI_SAME_STMT);
>>>>> }
>>>>>
>>>>> +/* Return true if the STEP in PARTS gives a valid BASE + INDEX * STEP
>>>>> + address for type TYPE and if the offset is making it appear invalid.
>>>>> */
>>>>> +
>>>>> +static bool
>>>>> +keep_index_p (tree type, mem_address parts)
>>>>
>>>> mem_ref_valid_without_offset_p (...)
>>>>
>>>> ?
>>>
>>> OK.
>>>
>>>>> +{
>>>>> + if (!parts.base)
>>>>> + return false;
>>>>> +
>>>>> + gcc_assert (!parts.symbol);
>>>>> + parts.offset = NULL_TREE;
>>>>> + return valid_mem_ref_p (TYPE_MODE (type), TYPE_ADDR_SPACE (type),
>>>>> &parts);
>>>>> +}
>>>>> +
>>>>> /* Creates and returns a TARGET_MEM_REF for address ADDR. If necessary
>>>>> computations are emitted in front of GSI. TYPE is the mode
>>>>> of created memory reference. IV_CAND is the selected iv candidate in
>>>>> ADDR,
>>>>> @@ -809,7 +823,8 @@ create_mem_ref (gimple_stmt_iterator *gs
>>>>
>>>> Which means all of the following would be more naturally written as
>>>>
>>>>> into:
>>>>> index' = index << step;
>>>>> [... + index' + ,,,]. */
>>>>> - if (parts.step && !integer_onep (parts.step))
>>>>> + bool scaled_p = (parts.step && !integer_onep (parts.step));
>>>>> + if (scaled_p && !keep_index_p (type, parts))
>>>>> {
>>>>
>>>> if (mem_ref_valid_without_offset_p (...))
>>>> {
>>>> ...
>>>> return create_mem_ref_raw (...);
>>>> }
>>>
>>> Is this inside the test for a scale:
>>>
>>> if (parts.step && !integer_onep (parts.step))
>>> {
>>> if (mem_ref_valid_without_offset_p (...))
>>> {
>>> tree tmp = parts.offset;
>>> if (parts.base)
>>> {
>>> tmp = fold_build_pointer_plus (parts.base, tmp);
>>> tmp = force_gimple_operand_gsi_1 (gsi, tmp,
>>> is_gimple_mem_ref_addr,
>>> NULL_TREE, true,
>>> GSI_SAME_STMT);
>>> }
>>> parts.base = tmp;
>>> parts.offset = NULL_TREE;
>>> mem_ref = create_mem_ref_raw (type, alias_ptr_type, &parts, true);
>>> gcc_assert (mem_ref);
>>> return mem_ref;
>>> }
>>> ...current code...
>>> }
>>>
>>> ? I can do that if you don't mind the cut-&-paste. Or I could
>>> split the code that adds the offset out into a subroutine.
>>
>> Sorry for the late response. Yes, sth like that.
>
> Sorry for the dropping the ball on this. Is the version below OK?
Ok.
Richard.
> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64-linux-gnu.
>
> Thanks,
> Richard
>
>
> 2018-01-09 Richard Sandiford <richard.sandif...@linaro.org>
> Alan Hayward <alan.hayw...@arm.com>
> David Sherwood <david.sherw...@arm.com>
>
> gcc/
> * tree-ssa-address.c (mem_ref_valid_without_offset_p): New function.
> (add_offset_to_base): New function, split out from...
> (create_mem_ref): ...here. When handling a scale other than 1,
> check first whether the address is valid without the offset.
> Add it into the base if so, leaving the index and scale as-is.
>
> Index: gcc/tree-ssa-address.c
> ===================================================================
> --- gcc/tree-ssa-address.c 2018-01-05 13:38:01.651810414 +0000
> +++ gcc/tree-ssa-address.c 2018-01-09 14:36:36.533460131 +0000
> @@ -746,6 +746,35 @@ gimplify_mem_ref_parts (gimple_stmt_iter
> true, GSI_SAME_STMT);
> }
>
> +/* Return true if the OFFSET in PARTS is the only thing that is making
> + it an invalid address for type TYPE. */
> +
> +static bool
> +mem_ref_valid_without_offset_p (tree type, mem_address parts)
> +{
> + if (!parts.base)
> + parts.base = parts.offset;
> + parts.offset = NULL_TREE;
> + return valid_mem_ref_p (TYPE_MODE (type), TYPE_ADDR_SPACE (type), &parts);
> +}
> +
> +/* Fold PARTS->offset into PARTS->base, so that there is no longer
> + a separate offset. Emit any new instructions before GSI. */
> +
> +static void
> +add_offset_to_base (gimple_stmt_iterator *gsi, mem_address *parts)
> +{
> + tree tmp = parts->offset;
> + if (parts->base)
> + {
> + tmp = fold_build_pointer_plus (parts->base, tmp);
> + tmp = force_gimple_operand_gsi_1 (gsi, tmp, is_gimple_mem_ref_addr,
> + NULL_TREE, true, GSI_SAME_STMT);
> + }
> + parts->base = tmp;
> + parts->offset = NULL_TREE;
> +}
> +
> /* Creates and returns a TARGET_MEM_REF for address ADDR. If necessary
> computations are emitted in front of GSI. TYPE is the mode
> of created memory reference. IV_CAND is the selected iv candidate in ADDR,
> @@ -812,6 +841,14 @@ create_mem_ref (gimple_stmt_iterator *gs
> if (parts.step && !integer_onep (parts.step))
> {
> gcc_assert (parts.index);
> + if (parts.offset && mem_ref_valid_without_offset_p (type, parts))
> + {
> + add_offset_to_base (gsi, &parts);
> + mem_ref = create_mem_ref_raw (type, alias_ptr_type, &parts, true);
> + gcc_assert (mem_ref);
> + return mem_ref;
> + }
> +
> parts.index = force_gimple_operand_gsi (gsi,
> fold_build2 (MULT_EXPR, sizetype,
> parts.index, parts.step),
> @@ -906,18 +943,7 @@ create_mem_ref (gimple_stmt_iterator *gs
> [base']. */
> if (parts.offset && !integer_zerop (parts.offset))
> {
> - tmp = parts.offset;
> - parts.offset = NULL_TREE;
> - /* Add offset to base. */
> - if (parts.base)
> - {
> - tmp = fold_build_pointer_plus (parts.base, tmp);
> - tmp = force_gimple_operand_gsi_1 (gsi, tmp,
> - is_gimple_mem_ref_addr,
> - NULL_TREE, true, GSI_SAME_STMT);
> - }
> - parts.base = tmp;
> -
> + add_offset_to_base (gsi, &parts);
> mem_ref = create_mem_ref_raw (type, alias_ptr_type, &parts, true);
> if (mem_ref)
> return mem_ref;
