Re: [patch 0/2] PR49847: Add hook to place read-only lookup-tables in named address-space

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Aug 16, 2017 at 3:32 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
> On 28.07.2017 09:34, Richard Biener wrote:
>>
>> On Thu, Jul 27, 2017 at 3:32 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
>>>
>>> On 27.07.2017 14:34, Richard Biener wrote:
>>>>
>>>>
>>>> On Thu, Jul 27, 2017 at 2:29 PM, Georg-Johann Lay <avr@gjlay.de> wrote:
>>>>>
>>>>>
>>>>> For some targets, the best place to put read-only lookup tables as
>>>>> generated by -ftree-switch-conversion is not the generic address space
>>>>> but some target specific address space.
>>>>>
>>>>> This is the case for AVR, where for most devices .rodata must be
>>>>> located in RAM.
>>>>>
>>>>> Part #1 adds a new, optional target hook that queries the back-end
>>>>> about the desired address space.  There is currently only one user:
>>>>> tree-switch-conversion.c::build_one_array() which re-builds value_type
>>>>> and array_type if the address space returned by the backend is not
>>>>> the generic one.
>>>>>
>>>>> Part #2 is the AVR part that implements the new hook and adds some
>>>>> sugar around it.
>>>>
>>>>
>>>>
>>>> Given that switch-conversion just creates a constant initializer doesn't
>>>> AVR
>>>> benefit from handling those uniformly (at RTL expansion?).  Not sure but
>>>> I think it goes through the regular constant pool handling.
>>>>
>>>> Richard.
>>>
>>>
>>>
>>> avr doesn't use constant pools because they are not profitable for
>>> several reasons.
>>>
>>> Moreover, it's not sufficient to just patch the pool's section, you'd
>>> also have to patch *all* accesses to also use the exact same address
>>> space so that they use the correct instruction (LPM instead of LD).
>>
>>
>> Sure.  So then not handle it in constant pool handling but in expanding
>> the initializers of global vars.  I think the entry for this is
>> varasm.c:assemble_variable - that sets DECL_RTL for the variable.
>
>
> That cannot work, and here is why; just for completeness:
>
> cgraphunit.c::symbol_table::compile()
>
> runs
>
>   ...
>   expand_all_functions ();
>   output_variables (); // runs assemble_variable
>   ...
>
> So any patching of DECL_RTL will result in wrong code: The address
> space of the decl won't match the address space used by the code
> accessing decl.

Ok, so it's more tricky to hack it that way (you'd need to catch the
time the decl gets its DECL_RTL set, not sure where that happens
for globals).

That leaves doing a more broad transform.  One convenient place
to hook in would be the ipa_single_use pass which computes
the varpool_node.used_by_single_function flag.  All such variables
would be suitable for promotion (with some additional constraints
I suppose).  You'd add a transform phase to the pass that rewrites
the decls and performs GIMPLE IL adjustments (I think you need
to patch memory references to use the address-space).

Of course there would need to be a target hook determining
if/what section/address-space a variable should be promoted to
which somehow would allow switch-conversion to use that
as well.  Ho humm.

That said, do you think the switch-conversion created decl is
the only one that benefits from compiler-directed promotion
to different address-space?

Richard.

> Johann