Re: [PATCH] Add a TARGET_GEN_MEMSET_VALUE hook

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

On Fri, Aug 19, 2016 at 4:45 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Aug 19, 2016 at 2:21 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Thu, Aug 18, 2016 at 5:16 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Thu, Aug 18, 2016 at 1:18 AM, Richard Biener
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, Aug 17, 2016 at 10:11 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>>>>> builtin_memset_gen_str returns a register used for memset, which only
>>>>> supports integer registers.  But a target may use vector registers in
>>>>> memmset.  This patch adds a TARGET_GEN_MEMSET_VALUE hook to duplicate
>>>>> QImode value to mode derived from STORE_MAX_PIECES, which can be used
>>>>> with vector instructions.  The default hook is the same as the original
>>>>> builtin_memset_gen_str.  A target can override it to support vector
>>>>> instructions for STORE_MAX_PIECES.
>>>>>
>>>>> Tested on x86-64 and i686.  Any comments?
>>>>>
>>>>> H.J.
>>>>> ---
>>>>> gcc/
>>>>>
>>>>>         * builtins.c (builtin_memset_gen_str): Call targetm.gen_memset_value.
>>>>>         (default_gen_memset_value): New function.
>>>>>         * target.def (gen_memset_value): New hook.
>>>>>         * targhooks.c: Inclue "expmed.h" and "builtins.h".
>>>>>         (default_gen_memset_value): New function.
>>>>
>>>> I see default_gen_memset_value in builtins.c but it belongs here.
>>>>
>>>>>         * targhooks.h (default_gen_memset_value): New prototype.
>>>>>         * config/i386/i386.c (ix86_gen_memset_value): New function.
>>>>>         (TARGET_GEN_MEMSET_VALUE): New.
>>>>>         * config/i386/i386.h (STORE_MAX_PIECES): Likewise.
>>>>>         * doc/tm.texi.in: Add TARGET_GEN_MEMSET_VALUE hook.
>>>>>         * doc/tm.texi: Updated.
>>>>>
>>>
>>> Like this?
>>
>> Aww, ok - I see builtins.c is a better place - sorry for the extra work.
>>
>> Richard.
>
> Here is the original patch with the updated ChangeLog.  OK for trunk?

So now actually looking at what you are doing.  Why do we need a new
hook?  Is it that even if we make builtin_memset_gen_str handle vector-mode
MODE properly you don't get the desired optimized code?  Your patch doesn't
seem to change the mode to vector but only STORE_MAX_PIECES and
op_by_pieces_d::run will only consider integer modes.

So isn't this a general missed optimization that TImode 0x01010101...01 * x
is not optimized to a pxor or broadcast?

Thanks,
Richard.

>
> --
> H.J.