On 2/4/20 12:02 PM, Richard Sandiford wrote: > Stam Markianos-Wright writes: >> On 1/31/20 1:45 PM, Richard Sandiford wrote: >>> Stam Markianos-Wright writes: >>>> On 1/30/20 10:01 AM, Richard Sandiford wrote: >>>>> Stam Markianos-Wright writes: >>>>>> On 1/29/20 12:42 PM, Richard Sandiford wrote: >>>>>>> Stam Markianos-Wright 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 >> >> * 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 :) 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[] =