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. > + /* 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? Thanks, Richard
