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 :)
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[] =
