Re: [PATCH 1/3] targhooks: Extend legitimate_address_p with code_helper [PR110248]
On Mon, Aug 7, 2023 at 12:15 PM Kewen.Lin wrote:
>
> Hi Richi,
>
> on 2023/6/30 17:13, Kewen.Lin via Gcc-patches wrote:
> > Hi Richi,
> >
> > Thanks for your review!
> >
> > on 2023/6/30 16:56, Richard Biener wrote:
> >> On Fri, Jun 30, 2023 at 7:38 AM Kewen.Lin wrote:
> >>>
> >>> Hi,
> >>>
> >>> As PR110248 shows, some middle-end passes like IVOPTs can
> >>> query the target hook legitimate_address_p with some
> >>> artificially constructed rtx to determine whether some
> >>> addressing modes are supported by target for some gimple
> >>> statement. But for now the existing legitimate_address_p
> >>> only checks the given mode, it's unable to distinguish
> >>> some special cases unfortunately, for example, for LEN_LOAD
> >>> ifn on Power port, we would expand it with lxvl hardware
> >>> insn, which only supports one register to hold the address
> >>> (the other register is holding the length), that is we
> >>> don't support base (reg) + index (reg) addressing mode for
> >>> sure. But hook legitimate_address_p only considers the
> >>> given mode which would be some vector mode for LEN_LOAD
> >>> ifn, and we do support base + index addressing mode for
> >>> normal vector load and store insns, so the hook will return
> >>> true for the query unexpectedly.
> >>>
> >>> This patch is to introduce one extra argument of type
> >>> code_helper for hook legitimate_address_p, it makes targets
> >>> able to handle some special case like what's described
> >>> above. The subsequent patches will show how to leverage
> >>> the new code_helper argument.
> >>>
> >>> I didn't separate those target specific adjustments to
> >>> their own patches, since those changes are no function
> >>> changes. One typical change is to add one unnamed argument
> >>> with default ERROR_MARK, some ports need to include tree.h
> >>> in their {port}-protos.h since the hook is used in some
> >>> machine description files. I've cross-built a corresponding
> >>> cc1 successfully for at least one triple of each affected
> >>> target so I believe they are safe. But feel free to correct
> >>> me if separating is needed for the review of this patch.
> >>>
> >>> Besides, it's bootstrapped and regtested on
> >>> x86_64-redhat-linux and powerpc64{,le}-linux-gnu.
> >>>
> >>> Is it ok for trunk?
> >>
> >> Is defaulting the arguments in the targets necessary for
> >> the middle-end or only for direct uses in the targets?
> >
> > It's only for the direct uses in target codes, the call
> > sites in generic code of these hooks would use the given
> > code_helper type variable or an explicit ERROR_MARK, they
> > don't require target codes to set that.
> >
> >>
> >> It looks OK in general but please give others some time to
> >> comment.
>
> Some weeks passed and no further comments are received, I guess
> this still looks good to you?
Yes.
> BR,
> Kewen
>
> >
> > OK, thanks again!
> >
> > BR,
> > Kewen
> >
> >>
> >> Thanks,
> >> Richard.
> >>
> >>> BR,
> >>> Kewen
> >>> --
> >>> PR tree-optimization/110248
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> * coretypes.h (class code_helper): Add forward declaration.
> >>> * doc/tm.texi: Regenerate.
> >>> * lra-constraints.cc (valid_address_p): Call target hook
> >>> targetm.addr_space.legitimate_address_p with an extra parameter
> >>> ERROR_MARK as its prototype changes.
> >>> * recog.cc (memory_address_addr_space_p): Likewise.
> >>> * reload.cc (strict_memory_address_addr_space_p): Likewise.
> >>> * target.def (legitimate_address_p,
> >>> addr_space.legitimate_address_p):
> >>> Extend with one more argument of type code_helper, update the
> >>> documentation accordingly.
> >>> * targhooks.cc (default_legitimate_address_p): Adjust for the
> >>> new code_helper argument.
> >>> (default_addr_space_legitimate_address_p): Likewise.
> >>> * targhooks.h (default_legitimate_address_p): Likewise.
> >>> (default_addr_space_legitimate_address_p): Likewise.
> >>> * config/aarch64/aarch64.cc (aarch64_legitimate_address_hook_p):
> >>> Adjust
> >>> with extra unnamed code_helper argument with default ERROR_MARK.
> >>> * config/alpha/alpha.cc (alpha_legitimate_address_p): Likewise.
> >>> * config/arc/arc.cc (arc_legitimate_address_p): Likewise.
> >>> * config/arm/arm-protos.h (arm_legitimate_address_p): Likewise.
> >>> (tree.h): New include for tree_code ERROR_MARK.
> >>> * config/arm/arm.cc (arm_legitimate_address_p): Adjust with extra
> >>> unnamed code_helper argument with default ERROR_MARK.
> >>> * config/avr/avr.cc (avr_addr_space_legitimate_address_p):
> >>> Likewise.
> >>> * config/bfin/bfin.cc (bfin_legitimate_address_p): Likewise.
> >>> * config/bpf/bpf.cc (bpf_legitimate_address_p): Likewise.
> >>> * config/c6x/c6x.cc (c6x_legitimate_address_p): Likewise.
> >>> *
Re: [PATCH 1/3] targhooks: Extend legitimate_address_p with code_helper [PR110248]
Hi Richi,
on 2023/6/30 17:13, Kewen.Lin via Gcc-patches wrote:
> Hi Richi,
>
> Thanks for your review!
>
> on 2023/6/30 16:56, Richard Biener wrote:
>> On Fri, Jun 30, 2023 at 7:38 AM Kewen.Lin wrote:
>>>
>>> Hi,
>>>
>>> As PR110248 shows, some middle-end passes like IVOPTs can
>>> query the target hook legitimate_address_p with some
>>> artificially constructed rtx to determine whether some
>>> addressing modes are supported by target for some gimple
>>> statement. But for now the existing legitimate_address_p
>>> only checks the given mode, it's unable to distinguish
>>> some special cases unfortunately, for example, for LEN_LOAD
>>> ifn on Power port, we would expand it with lxvl hardware
>>> insn, which only supports one register to hold the address
>>> (the other register is holding the length), that is we
>>> don't support base (reg) + index (reg) addressing mode for
>>> sure. But hook legitimate_address_p only considers the
>>> given mode which would be some vector mode for LEN_LOAD
>>> ifn, and we do support base + index addressing mode for
>>> normal vector load and store insns, so the hook will return
>>> true for the query unexpectedly.
>>>
>>> This patch is to introduce one extra argument of type
>>> code_helper for hook legitimate_address_p, it makes targets
>>> able to handle some special case like what's described
>>> above. The subsequent patches will show how to leverage
>>> the new code_helper argument.
>>>
>>> I didn't separate those target specific adjustments to
>>> their own patches, since those changes are no function
>>> changes. One typical change is to add one unnamed argument
>>> with default ERROR_MARK, some ports need to include tree.h
>>> in their {port}-protos.h since the hook is used in some
>>> machine description files. I've cross-built a corresponding
>>> cc1 successfully for at least one triple of each affected
>>> target so I believe they are safe. But feel free to correct
>>> me if separating is needed for the review of this patch.
>>>
>>> Besides, it's bootstrapped and regtested on
>>> x86_64-redhat-linux and powerpc64{,le}-linux-gnu.
>>>
>>> Is it ok for trunk?
>>
>> Is defaulting the arguments in the targets necessary for
>> the middle-end or only for direct uses in the targets?
>
> It's only for the direct uses in target codes, the call
> sites in generic code of these hooks would use the given
> code_helper type variable or an explicit ERROR_MARK, they
> don't require target codes to set that.
>
>>
>> It looks OK in general but please give others some time to
>> comment.
Some weeks passed and no further comments are received, I guess
this still looks good to you?
BR,
Kewen
>
> OK, thanks again!
>
> BR,
> Kewen
>
>>
>> Thanks,
>> Richard.
>>
>>> BR,
>>> Kewen
>>> --
>>> PR tree-optimization/110248
>>>
>>> gcc/ChangeLog:
>>>
>>> * coretypes.h (class code_helper): Add forward declaration.
>>> * doc/tm.texi: Regenerate.
>>> * lra-constraints.cc (valid_address_p): Call target hook
>>> targetm.addr_space.legitimate_address_p with an extra parameter
>>> ERROR_MARK as its prototype changes.
>>> * recog.cc (memory_address_addr_space_p): Likewise.
>>> * reload.cc (strict_memory_address_addr_space_p): Likewise.
>>> * target.def (legitimate_address_p,
>>> addr_space.legitimate_address_p):
>>> Extend with one more argument of type code_helper, update the
>>> documentation accordingly.
>>> * targhooks.cc (default_legitimate_address_p): Adjust for the
>>> new code_helper argument.
>>> (default_addr_space_legitimate_address_p): Likewise.
>>> * targhooks.h (default_legitimate_address_p): Likewise.
>>> (default_addr_space_legitimate_address_p): Likewise.
>>> * config/aarch64/aarch64.cc (aarch64_legitimate_address_hook_p):
>>> Adjust
>>> with extra unnamed code_helper argument with default ERROR_MARK.
>>> * config/alpha/alpha.cc (alpha_legitimate_address_p): Likewise.
>>> * config/arc/arc.cc (arc_legitimate_address_p): Likewise.
>>> * config/arm/arm-protos.h (arm_legitimate_address_p): Likewise.
>>> (tree.h): New include for tree_code ERROR_MARK.
>>> * config/arm/arm.cc (arm_legitimate_address_p): Adjust with extra
>>> unnamed code_helper argument with default ERROR_MARK.
>>> * config/avr/avr.cc (avr_addr_space_legitimate_address_p): Likewise.
>>> * config/bfin/bfin.cc (bfin_legitimate_address_p): Likewise.
>>> * config/bpf/bpf.cc (bpf_legitimate_address_p): Likewise.
>>> * config/c6x/c6x.cc (c6x_legitimate_address_p): Likewise.
>>> * config/cris/cris-protos.h (cris_legitimate_address_p): Likewise.
>>> (tree.h): New include for tree_code ERROR_MARK.
>>> * config/cris/cris.cc (cris_legitimate_address_p): Adjust with extra
>>> unnamed code_helper argument with default ERROR_MARK.
>>> *
Re: [PATCH 1/3] targhooks: Extend legitimate_address_p with code_helper [PR110248]
Hi Richi,
Thanks for your review!
on 2023/6/30 16:56, Richard Biener wrote:
> On Fri, Jun 30, 2023 at 7:38 AM Kewen.Lin wrote:
>>
>> Hi,
>>
>> As PR110248 shows, some middle-end passes like IVOPTs can
>> query the target hook legitimate_address_p with some
>> artificially constructed rtx to determine whether some
>> addressing modes are supported by target for some gimple
>> statement. But for now the existing legitimate_address_p
>> only checks the given mode, it's unable to distinguish
>> some special cases unfortunately, for example, for LEN_LOAD
>> ifn on Power port, we would expand it with lxvl hardware
>> insn, which only supports one register to hold the address
>> (the other register is holding the length), that is we
>> don't support base (reg) + index (reg) addressing mode for
>> sure. But hook legitimate_address_p only considers the
>> given mode which would be some vector mode for LEN_LOAD
>> ifn, and we do support base + index addressing mode for
>> normal vector load and store insns, so the hook will return
>> true for the query unexpectedly.
>>
>> This patch is to introduce one extra argument of type
>> code_helper for hook legitimate_address_p, it makes targets
>> able to handle some special case like what's described
>> above. The subsequent patches will show how to leverage
>> the new code_helper argument.
>>
>> I didn't separate those target specific adjustments to
>> their own patches, since those changes are no function
>> changes. One typical change is to add one unnamed argument
>> with default ERROR_MARK, some ports need to include tree.h
>> in their {port}-protos.h since the hook is used in some
>> machine description files. I've cross-built a corresponding
>> cc1 successfully for at least one triple of each affected
>> target so I believe they are safe. But feel free to correct
>> me if separating is needed for the review of this patch.
>>
>> Besides, it's bootstrapped and regtested on
>> x86_64-redhat-linux and powerpc64{,le}-linux-gnu.
>>
>> Is it ok for trunk?
>
> Is defaulting the arguments in the targets necessary for
> the middle-end or only for direct uses in the targets?
It's only for the direct uses in target codes, the call
sites in generic code of these hooks would use the given
code_helper type variable or an explicit ERROR_MARK, they
don't require target codes to set that.
>
> It looks OK in general but please give others some time to
> comment.
OK, thanks again!
BR,
Kewen
>
> Thanks,
> Richard.
>
>> BR,
>> Kewen
>> --
>> PR tree-optimization/110248
>>
>> gcc/ChangeLog:
>>
>> * coretypes.h (class code_helper): Add forward declaration.
>> * doc/tm.texi: Regenerate.
>> * lra-constraints.cc (valid_address_p): Call target hook
>> targetm.addr_space.legitimate_address_p with an extra parameter
>> ERROR_MARK as its prototype changes.
>> * recog.cc (memory_address_addr_space_p): Likewise.
>> * reload.cc (strict_memory_address_addr_space_p): Likewise.
>> * target.def (legitimate_address_p, addr_space.legitimate_address_p):
>> Extend with one more argument of type code_helper, update the
>> documentation accordingly.
>> * targhooks.cc (default_legitimate_address_p): Adjust for the
>> new code_helper argument.
>> (default_addr_space_legitimate_address_p): Likewise.
>> * targhooks.h (default_legitimate_address_p): Likewise.
>> (default_addr_space_legitimate_address_p): Likewise.
>> * config/aarch64/aarch64.cc (aarch64_legitimate_address_hook_p):
>> Adjust
>> with extra unnamed code_helper argument with default ERROR_MARK.
>> * config/alpha/alpha.cc (alpha_legitimate_address_p): Likewise.
>> * config/arc/arc.cc (arc_legitimate_address_p): Likewise.
>> * config/arm/arm-protos.h (arm_legitimate_address_p): Likewise.
>> (tree.h): New include for tree_code ERROR_MARK.
>> * config/arm/arm.cc (arm_legitimate_address_p): Adjust with extra
>> unnamed code_helper argument with default ERROR_MARK.
>> * config/avr/avr.cc (avr_addr_space_legitimate_address_p): Likewise.
>> * config/bfin/bfin.cc (bfin_legitimate_address_p): Likewise.
>> * config/bpf/bpf.cc (bpf_legitimate_address_p): Likewise.
>> * config/c6x/c6x.cc (c6x_legitimate_address_p): Likewise.
>> * config/cris/cris-protos.h (cris_legitimate_address_p): Likewise.
>> (tree.h): New include for tree_code ERROR_MARK.
>> * config/cris/cris.cc (cris_legitimate_address_p): Adjust with extra
>> unnamed code_helper argument with default ERROR_MARK.
>> * config/csky/csky.cc (csky_legitimate_address_p): Likewise.
>> * config/epiphany/epiphany.cc (epiphany_legitimate_address_p):
>> Likewise.
>> * config/frv/frv.cc (frv_legitimate_address_p): Likewise.
>> * config/ft32/ft32.cc (ft32_addr_space_legitimate_address_p):
>> Likewise.
>>
Re: [PATCH 1/3] targhooks: Extend legitimate_address_p with code_helper [PR110248]
On Fri, Jun 30, 2023 at 7:38 AM Kewen.Lin wrote:
>
> Hi,
>
> As PR110248 shows, some middle-end passes like IVOPTs can
> query the target hook legitimate_address_p with some
> artificially constructed rtx to determine whether some
> addressing modes are supported by target for some gimple
> statement. But for now the existing legitimate_address_p
> only checks the given mode, it's unable to distinguish
> some special cases unfortunately, for example, for LEN_LOAD
> ifn on Power port, we would expand it with lxvl hardware
> insn, which only supports one register to hold the address
> (the other register is holding the length), that is we
> don't support base (reg) + index (reg) addressing mode for
> sure. But hook legitimate_address_p only considers the
> given mode which would be some vector mode for LEN_LOAD
> ifn, and we do support base + index addressing mode for
> normal vector load and store insns, so the hook will return
> true for the query unexpectedly.
>
> This patch is to introduce one extra argument of type
> code_helper for hook legitimate_address_p, it makes targets
> able to handle some special case like what's described
> above. The subsequent patches will show how to leverage
> the new code_helper argument.
>
> I didn't separate those target specific adjustments to
> their own patches, since those changes are no function
> changes. One typical change is to add one unnamed argument
> with default ERROR_MARK, some ports need to include tree.h
> in their {port}-protos.h since the hook is used in some
> machine description files. I've cross-built a corresponding
> cc1 successfully for at least one triple of each affected
> target so I believe they are safe. But feel free to correct
> me if separating is needed for the review of this patch.
>
> Besides, it's bootstrapped and regtested on
> x86_64-redhat-linux and powerpc64{,le}-linux-gnu.
>
> Is it ok for trunk?
Is defaulting the arguments in the targets necessary for
the middle-end or only for direct uses in the targets?
It looks OK in general but please give others some time to
comment.
Thanks,
Richard.
> BR,
> Kewen
> --
> PR tree-optimization/110248
>
> gcc/ChangeLog:
>
> * coretypes.h (class code_helper): Add forward declaration.
> * doc/tm.texi: Regenerate.
> * lra-constraints.cc (valid_address_p): Call target hook
> targetm.addr_space.legitimate_address_p with an extra parameter
> ERROR_MARK as its prototype changes.
> * recog.cc (memory_address_addr_space_p): Likewise.
> * reload.cc (strict_memory_address_addr_space_p): Likewise.
> * target.def (legitimate_address_p, addr_space.legitimate_address_p):
> Extend with one more argument of type code_helper, update the
> documentation accordingly.
> * targhooks.cc (default_legitimate_address_p): Adjust for the
> new code_helper argument.
> (default_addr_space_legitimate_address_p): Likewise.
> * targhooks.h (default_legitimate_address_p): Likewise.
> (default_addr_space_legitimate_address_p): Likewise.
> * config/aarch64/aarch64.cc (aarch64_legitimate_address_hook_p):
> Adjust
> with extra unnamed code_helper argument with default ERROR_MARK.
> * config/alpha/alpha.cc (alpha_legitimate_address_p): Likewise.
> * config/arc/arc.cc (arc_legitimate_address_p): Likewise.
> * config/arm/arm-protos.h (arm_legitimate_address_p): Likewise.
> (tree.h): New include for tree_code ERROR_MARK.
> * config/arm/arm.cc (arm_legitimate_address_p): Adjust with extra
> unnamed code_helper argument with default ERROR_MARK.
> * config/avr/avr.cc (avr_addr_space_legitimate_address_p): Likewise.
> * config/bfin/bfin.cc (bfin_legitimate_address_p): Likewise.
> * config/bpf/bpf.cc (bpf_legitimate_address_p): Likewise.
> * config/c6x/c6x.cc (c6x_legitimate_address_p): Likewise.
> * config/cris/cris-protos.h (cris_legitimate_address_p): Likewise.
> (tree.h): New include for tree_code ERROR_MARK.
> * config/cris/cris.cc (cris_legitimate_address_p): Adjust with extra
> unnamed code_helper argument with default ERROR_MARK.
> * config/csky/csky.cc (csky_legitimate_address_p): Likewise.
> * config/epiphany/epiphany.cc (epiphany_legitimate_address_p):
> Likewise.
> * config/frv/frv.cc (frv_legitimate_address_p): Likewise.
> * config/ft32/ft32.cc (ft32_addr_space_legitimate_address_p):
> Likewise.
> * config/gcn/gcn.cc (gcn_addr_space_legitimate_address_p): Likewise.
> * config/h8300/h8300.cc (h8300_legitimate_address_p): Likewise.
> * config/i386/i386.cc (ix86_legitimate_address_p): Likewise.
> * config/ia64/ia64.cc (ia64_legitimate_address_p): Likewise.
> * config/iq2000/iq2000.cc (iq2000_legitimate_address_p): Likewise.
> * config/lm32/lm32.cc
