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. */
> +
> +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.
> +}
> +
> /* 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;".
>
> 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.
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[] =
