Stam Markianos-Wright <stam.markianos-wri...@arm.com> writes:
> On 2/4/20 12:02 PM, Richard Sandiford wrote:
>> Stam Markianos-Wright <stam.markianos-wri...@arm.com> writes:
>>> On 1/31/20 1:45 PM, Richard Sandiford wrote:
>>>> Stam Markianos-Wright <stam.markianos-wri...@arm.com> writes:
>>>>> On 1/30/20 10:01 AM, Richard Sandiford wrote:
>>>>>> Stam Markianos-Wright <stam.markianos-wri...@arm.com> writes:
>>>>>>> On 1/29/20 12:42 PM, Richard Sandiford wrote:
>>>>>>>> Stam Markianos-Wright <stam.markianos-wri...@arm.com> writes:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> This fixes:
>>>>>>>>>
>>>>>>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93300
>>>>>>>>>
>>>>>>>>> Genmodes.c was generating the "wider_mode" chain as follows:
>>>>>>>>>
>>>>>>>>> HF -> BF -> SF - > DF -> TF -> VOID
>>>>>>>>>
>>>>>>>>> This caused issues in some rare cases where conversion between modes
>>>>>>>>> was needed, such as the above PR93300 where BFmode was being picked up
>>>>>>>>> as a valid mode for:
>>>>>>>>>
>>>>>>>>> optabs.c:prepare_float_lib_cmp
>>>>>>>>>
>>>>>>>>> which then led to the ICE at expr.c:convert_mode_scalar.
>>>>>>>
>>>>>>> Hi Richard,
>>>>>>>
>>>>>>>>
>>>>>>>> Can you go into more details about why this chain was a problem?
>>>>>>>> Naively, it's the one I'd have expected: HF should certainly have
>>>>>>>> priority over BF,
>>>>>>>
>>>>>>> Is that because functionally it looks like genmodes puts things in
>>>>>>> reverse
>>>>>>> alphabetical order if all else is equal? (If I'm reading the comment
>>>>>>> about
>>>>>>> MODE_RANDOM, MODE_CC correctly)
>>>>>>>
>>>>>>>> but BF coming before SF doesn't seem unusual in itself.
>>>>>>>>
>>>>>>>> I'm not saying the patch is wrong. It just wasn't clear why it was
>>>>>>>> right either.
>>>>>>>>
>>>>>>> Yes, I see what you mean. I'll go through my thought process here:
>>>>>>>
>>>>>>> In investigating the ICE PR93300 I found that the diversion from
>>>>>>> pre-bf16
>>>>>>> behaviour was specifically at `optabs.c:prepare_float_lib_cmp`, where a
>>>>>>> `FOR_EACH_MODE_FROM (mode, orig_mode)` is used to then go off and
>>>>>>> generate
>>>>>>> library calls for conversions.
>>>>>>>
>>>>>>> This was then being caught further down by the gcc_assert at expr.c:325
>>>>>>> where
>>>>>>> GET_MODE_PRECISION (from_mode) was equal to GET_MODE_PRECISION
>>>>>>> (to_mode) because
>>>>>>> it was trying to emit a HF->BF conversion libcall as `bl __extendhfbf2`
>>>>>>> (which
>>>>>>> is what happened if i removed the gcc_assert at expr.c:325)
>>>>>>>
>>>>>>> With BFmode being a target-defined mode, I didn't want to add something
>>>>>>> like `if
>>>>>>> (mode != BFmode)` to specifically exclude BFmode from being selected
>>>>>>> for this.
>>>>>>> (and there's nothing different between HFmode and BFmode here to allow
>>>>>>> me to
>>>>>>> make this distinction?)
>>>>>>>
>>>>>>> Also I couldn't find anywhere where the target back-end is not
>>>>>>> consulted for a
>>>>>>> "is this supported: yes/no" between the `FOR_EACH_MODE_FROM` loop and
>>>>>>> the
>>>>>>> libcall being created later on as __extendhfbf2.
>>>>>>
>>>>>> Yeah, prepare_float_lib_cmp just checks for libfuncs rather than
>>>>>> calling target hooks directly. The libfuncs themselves are under
>>>>>> the control of the target though.
>>>>>>
>>>>>> By default we assume all float modes have associated libfuncs.
>>>>>> It's then up to the target to remove functions that don't exist
>>>>>> (or redirect them to other functions). So I think we need to remove
>>>>>> BFmode libfuncs in arm_init_libfuncs in the same way as we currently
>>>>>> do for HFmode.
>>>>>>
>>>>>> I guess we should also nullify the conversion libfuncs for BFmode,
>>>>>> not just the arithmetic and comparison ones.
>>>>>
>>>>> Ahhh now this works, thank you for the suggestion!
>>>>>
>>>>> I was aware of arm_init_libfuncs, but I had not realised that returning
>>>>> NULL
>>>>> would have the desired effect for us, in this case. So I have essentially
>>>>> rolled
>>>>> back the whole previous version of the patch and done this in the new
>>>>> diff.
>>>>> It seems to have fixed the ICE and I am currently in the process of
>>>>> regression
>>>>> testing!
>>>>
>>>> LGTM behaviourally, just a couple of requests about how it's written:
>>>>
>>>>>
>>>>> Thank you!
>>>>> Stam
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Richard
>>>>>>
>>>>>>> Finally, because we really don't want __bf16 to be in the same
>>>>>>> "conversion rank"
>>>>>>> as standard float modes for things like automatic promotion, this
>>>>>>> seemed like a
>>>>>>> reasonable solution to that problem :)
>>>>>>>
>>>>>>> Let me know of your thoughts!
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Stam
>>>>>
>>>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>>>> index c47fc232f39..18055d4a75e 100644
>>>>> --- a/gcc/config/arm/arm.c
>>>>> +++ b/gcc/config/arm/arm.c
>>>>> @@ -2643,6 +2643,30 @@ arm_init_libfuncs (void)
>>>>> default:
>>>>> break;
>>>>> }
>>>>> +
>>>>> + /* For all possible libcalls in BFmode, return NULL. */
>>>>> + /* Conversions. */
>>>>> + set_conv_libfunc (trunc_optab, BFmode, HFmode, (NULL));
>>>>> + set_conv_libfunc (sext_optab, HFmode, BFmode, (NULL));
>>>>> + set_conv_libfunc (trunc_optab, BFmode, SFmode, (NULL));
>>>>> + set_conv_libfunc (sext_optab, SFmode, BFmode, (NULL));
>>>>> + set_conv_libfunc (trunc_optab, BFmode, DFmode, (NULL));
>>>>> + set_conv_libfunc (sext_optab, DFmode, BFmode, (NULL));
>>>>
>>>> It might be slightly safer to do:
>>>>
>>>> FOR_EACH_MODE_IN_CLASS (mode_iter, MODE_FLOAT)
>>>>
>>>> to iterate over all float modes on the non-BF side.
>>>
>>> Done :)
>>>>
>>>>> + /* Arithmetic. */
>>>>> + set_optab_libfunc (add_optab, BFmode, NULL);
>>>>> + set_optab_libfunc (sdiv_optab, BFmode, NULL);
>>>>> + set_optab_libfunc (smul_optab, BFmode, NULL);
>>>>> + set_optab_libfunc (neg_optab, BFmode, NULL);
>>>>> + set_optab_libfunc (sub_optab, BFmode, NULL);
>>>>> +
>>>>> + /* Comparisons. */
>>>>> + set_optab_libfunc (eq_optab, BFmode, NULL);
>>>>> + set_optab_libfunc (ne_optab, BFmode, NULL);
>>>>> + set_optab_libfunc (lt_optab, BFmode, NULL);
>>>>> + set_optab_libfunc (le_optab, BFmode, NULL);
>>>>> + set_optab_libfunc (ge_optab, BFmode, NULL);
>>>>> + set_optab_libfunc (gt_optab, BFmode, NULL);
>>>>> + set_optab_libfunc (unord_optab, BFmode, NULL);
>>>>
>>>> Could you split this part out into a subroutine and reuse it for the
>>>> existing HFmode code too?
>>>
>>> Done
>>>
>>> Let me know if you spot any other issues or nits of course!
>>>
>>> Cheers,
>>> Stam
>>>
>>> New changelog:
>>>
>>> gcc/ChangeLog:
>>>
>>> 2020-02-04 Stam Markianos-Wright <stam.markianos-wri...@arm.com>
>>>
>>> * config/arm/arm.c (arm_block_arith_comp_libfuncs_for_mode): New.
>>> (arm_init_libfuncs): Add BFmode support to block spurious BF libfuncs.
>>> Use arm_block_arith_comp_libfuncs_for_mode for HFmode.
>>>
>>>
>>>>
>>>> Thanks,
>>>> Richard
>>>>
>>>
>>> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
>>> index c47fc232f39..80425f77f9d 100644
>>> --- a/gcc/config/arm/arm.c
>>> +++ b/gcc/config/arm/arm.c
>>> @@ -2491,10 +2491,35 @@ arm_set_fixed_conv_libfunc (convert_optab optable,
>>> machine_mode to,
>>>
>>> static GTY(()) rtx speculation_barrier_libfunc;
>>>
>>> +/* Return NULL to block arithmetic and comparison libfuncs in
>>> + a specific machine_mode. */
>>
>> "Return" to me sounds like this is talking about the return value
>> of the function. Maybe something like:
>>
>> /* Record that we have no arithmetic or comparison libfuncs for
>> machinde mode MODE. */
> Done
>>
>>> +
>>> +static void
>>> +arm_block_arith_comp_libfuncs_for_mode (machine_mode mode)
>>> +{
>>> + /* Arithmetic. */
>>> + set_optab_libfunc (add_optab, mode, NULL);
>>> + set_optab_libfunc (sdiv_optab, mode, NULL);
>>> + set_optab_libfunc (smul_optab, mode, NULL);
>>> + set_optab_libfunc (neg_optab, mode, NULL);
>>> + set_optab_libfunc (sub_optab, mode, NULL);
>>> +
>>> + /* Comparisons. */
>>> + set_optab_libfunc (eq_optab, mode, NULL);
>>> + set_optab_libfunc (ne_optab, mode, NULL);
>>> + set_optab_libfunc (lt_optab, mode, NULL);
>>> + set_optab_libfunc (le_optab, mode, NULL);
>>> + set_optab_libfunc (ge_optab, mode, NULL);
>>> + set_optab_libfunc (gt_optab, mode, NULL);
>>> + set_optab_libfunc (unord_optab, mode, NULL);
>>
>> Too much indentation, should just be two spaces.
> Done, sorry I didn't see that!
>>
>>> +}
>>> +
>>> /* Set up library functions unique to ARM. */
>>> static void
>>> arm_init_libfuncs (void)
>>> {
>>> + machine_mode mode_iter;
>>> +
>>> /* For Linux, we have access to kernel support for atomic operations.
>>> */
>>> if (arm_abi == ARM_ABI_AAPCS_LINUX)
>>> init_sync_libfuncs (MAX_SYNC_LIBFUNC_SIZE);
>>> @@ -2623,27 +2648,22 @@ arm_init_libfuncs (void)
>>> ? "__gnu_d2h_ieee"
>>> : "__gnu_d2h_alternative"));
>>>
>>> - /* Arithmetic. */
>>> - set_optab_libfunc (add_optab, HFmode, NULL);
>>> - set_optab_libfunc (sdiv_optab, HFmode, NULL);
>>> - set_optab_libfunc (smul_optab, HFmode, NULL);
>>> - set_optab_libfunc (neg_optab, HFmode, NULL);
>>> - set_optab_libfunc (sub_optab, HFmode, NULL);
>>> + arm_block_arith_comp_libfuncs_for_mode (HFmode);
>>>
>>> - /* Comparisons. */
>>> - set_optab_libfunc (eq_optab, HFmode, NULL);
>>> - set_optab_libfunc (ne_optab, HFmode, NULL);
>>> - set_optab_libfunc (lt_optab, HFmode, NULL);
>>> - set_optab_libfunc (le_optab, HFmode, NULL);
>>> - set_optab_libfunc (ge_optab, HFmode, NULL);
>>> - set_optab_libfunc (gt_optab, HFmode, NULL);
>>> - set_optab_libfunc (unord_optab, HFmode, NULL);
>>> break;
>>
>> Nit: no need for a blank line before "break;".
> Done
>>
>>>
>>> default:
>>> break;
>>> }
>>>
>>> + /* For all possible libcalls in BFmode, return NULL. */
>>> + FOR_EACH_MODE_IN_CLASS (mode_iter, MODE_FLOAT)
>>> + {
>>> + set_conv_libfunc (trunc_optab, BFmode, mode_iter, (NULL));
>>> + set_conv_libfunc (sext_optab, mode_iter, BFmode, (NULL));
>>
>> It would probably be safer to use both argument orders for each optab,
>> since BFmode isn't necessarily the "narrowest" mode.
>>
>> Nit: no need for brackets around NULL.
> Done
>
> Let me know if this is good to go now :)
Yeah, this is OK, thanks.
Richard
>
> Thank you so much for the help getting this finalised!
>
> Cheers,
> Stam
>>
>> Thanks,
>> Richard
>>
>>> + }
>>> + arm_block_arith_comp_libfuncs_for_mode (BFmode);
>>> +
>>> /* Use names prefixed with __gnu_ for fixed-point helper functions. */
>>> {
>>> const arm_fixed_mode_set fixed_arith_modes[] =
>
> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> index c47fc232f39..ad1c1fa8e3b 100644
> --- a/gcc/config/arm/arm.c
> +++ b/gcc/config/arm/arm.c
> @@ -2491,10 +2491,35 @@ arm_set_fixed_conv_libfunc (convert_optab optable,
> machine_mode to,
>
> static GTY(()) rtx speculation_barrier_libfunc;
>
> +/* Record that we have no arithmetic or comparison libfuncs for
> + machine mode MODE. */
> +
> +static void
> +arm_block_arith_comp_libfuncs_for_mode (machine_mode mode)
> +{
> + /* Arithmetic. */
> + set_optab_libfunc (add_optab, mode, NULL);
> + set_optab_libfunc (sdiv_optab, mode, NULL);
> + set_optab_libfunc (smul_optab, mode, NULL);
> + set_optab_libfunc (neg_optab, mode, NULL);
> + set_optab_libfunc (sub_optab, mode, NULL);
> +
> + /* Comparisons. */
> + set_optab_libfunc (eq_optab, mode, NULL);
> + set_optab_libfunc (ne_optab, mode, NULL);
> + set_optab_libfunc (lt_optab, mode, NULL);
> + set_optab_libfunc (le_optab, mode, NULL);
> + set_optab_libfunc (ge_optab, mode, NULL);
> + set_optab_libfunc (gt_optab, mode, NULL);
> + set_optab_libfunc (unord_optab, mode, NULL);
> +}
> +
> /* Set up library functions unique to ARM. */
> static void
> arm_init_libfuncs (void)
> {
> + machine_mode mode_iter;
> +
> /* For Linux, we have access to kernel support for atomic operations. */
> if (arm_abi == ARM_ABI_AAPCS_LINUX)
> init_sync_libfuncs (MAX_SYNC_LIBFUNC_SIZE);
> @@ -2623,27 +2648,23 @@ arm_init_libfuncs (void)
> ? "__gnu_d2h_ieee"
> : "__gnu_d2h_alternative"));
>
> - /* Arithmetic. */
> - set_optab_libfunc (add_optab, HFmode, NULL);
> - set_optab_libfunc (sdiv_optab, HFmode, NULL);
> - set_optab_libfunc (smul_optab, HFmode, NULL);
> - set_optab_libfunc (neg_optab, HFmode, NULL);
> - set_optab_libfunc (sub_optab, HFmode, NULL);
> -
> - /* Comparisons. */
> - set_optab_libfunc (eq_optab, HFmode, NULL);
> - set_optab_libfunc (ne_optab, HFmode, NULL);
> - set_optab_libfunc (lt_optab, HFmode, NULL);
> - set_optab_libfunc (le_optab, HFmode, NULL);
> - set_optab_libfunc (ge_optab, HFmode, NULL);
> - set_optab_libfunc (gt_optab, HFmode, NULL);
> - set_optab_libfunc (unord_optab, HFmode, NULL);
> + arm_block_arith_comp_libfuncs_for_mode (HFmode);
> break;
>
> default:
> break;
> }
>
> + /* For all possible libcalls in BFmode, record NULL. */
> + FOR_EACH_MODE_IN_CLASS (mode_iter, MODE_FLOAT)
> + {
> + set_conv_libfunc (trunc_optab, BFmode, mode_iter, NULL);
> + set_conv_libfunc (trunc_optab, mode_iter, BFmode, NULL);
> + set_conv_libfunc (sext_optab, mode_iter, BFmode, NULL);
> + set_conv_libfunc (sext_optab, BFmode, mode_iter, NULL);
> + }
> + arm_block_arith_comp_libfuncs_for_mode (BFmode);
> +
> /* Use names prefixed with __gnu_ for fixed-point helper functions. */
> {
> const arm_fixed_mode_set fixed_arith_modes[] =
