Re: [GCC][BUG][Aarch64][ARM] (PR93300) Fix ICE due to BFmode placement in GET_MODES_WIDER chain.

[Stam Markianos-Wright]

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.

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

Thanks,
Richard


This patch adds a new FLOAT_MODE_UNRANKED macro which uses the existing "order"
attribute of mode_data to place BFmode as:

HF -> SF - > DF -> TF -> BF -> VOID

This fixes the existing ICE seen by PR93300 (hence providing this with no
explicit test) and causes no further regressions.
Reg-tested on arm-none-eabi, aarch64-none-elf and bootstrapped on a Cortex-A15.

Ok for trunk?

Cheers,
Stam

gcc/ChangeLog:

2020-01-28  Stam Markianos-Wright  <stam.markianos-wri...@arm.com>

        * config/aarch64/aarch64-modes.def: Update BFmode to use 
FLOAT_MODE_UNRANKED.
        * config/arm/arm-modes.def: Update BFmode to use FLOAT_MODE_UNRANKED.
        * genmodes.c (FLOAT_MODE_UNRANKED): New macro.
           (make_float_mode): Add ORDER parameter.

The whole diff for reference:

diff --git a/gcc/config/aarch64/aarch64-modes.def
b/gcc/config/aarch64/aarch64-modes.def
index 1eeb8d88452..0b36da942b4 100644
--- a/gcc/config/aarch64/aarch64-modes.def
+++ b/gcc/config/aarch64/aarch64-modes.def
@@ -69,10 +69,10 @@ VECTOR_MODES (FLOAT, 16);     /*            V4SF V2DF.  */
   VECTOR_MODE (FLOAT, DF, 1);   /*                 V1DF.  */
   VECTOR_MODE (FLOAT, HF, 2);   /*                 V2HF.  */

-/* Bfloat16 modes.  */
-FLOAT_MODE (BF, 2, 0);
+/* Bfloat16 modes. Using 1 as the ORDER argument ensures that this is
+   placed after normal floating point modes in the GET_MODES_WIDER chain.  */
+FLOAT_MODE_UNRANKED (BF, 2, 0, 1);
   ADJUST_FLOAT_FORMAT (BF, &arm_bfloat_half_format);
-
   VECTOR_MODE (FLOAT, BF, 4);   /*              V4BF.  */
   VECTOR_MODE (FLOAT, BF, 8);   /*              V8BF.  */

diff --git a/gcc/config/arm/arm-modes.def b/gcc/config/arm/arm-modes.def
index ea92ef35723..86551be8e3b 100644
--- a/gcc/config/arm/arm-modes.def
+++ b/gcc/config/arm/arm-modes.def
@@ -78,7 +78,9 @@ VECTOR_MODES (FLOAT, 8);      /*            V4HF V2SF */
   VECTOR_MODES (FLOAT, 16);     /*       V8HF V4SF V2DF */
   VECTOR_MODE (FLOAT, HF, 2);   /*                 V2HF */

-FLOAT_MODE (BF, 2, 0);
+/* Bfloat16 modes. Using 1 as the ORDER argument ensures that this is
+   placed after normal floating point modes in the GET_MODES_WIDER chain.  */
+FLOAT_MODE_UNRANKED (BF, 2, 0, 1);
   ADJUST_FLOAT_FORMAT (BF, &arm_bfloat_half_format);
   VECTOR_MODE (FLOAT, BF, 4);   /*              V4BF.  */
   VECTOR_MODE (FLOAT, BF, 8);   /*              V8BF.  */
diff --git a/gcc/genmodes.c b/gcc/genmodes.c
index bd78310ea24..c4e3dd1150d 100644
--- a/gcc/genmodes.c
+++ b/gcc/genmodes.c
@@ -617,20 +617,23 @@ make_fixed_point_mode (enum mode_class cl,
     m->fbit = fbit;
   }

-#define FLOAT_MODE(N, Y, F)             FRACTIONAL_FLOAT_MODE (N, -1U, Y, F)
-#define FRACTIONAL_FLOAT_MODE(N, B, Y, F) \
-  make_float_mode (#N, B, Y, #F, __FILE__, __LINE__)
+#define FLOAT_MODE_UNRANKED(N, Y, F, ORDER)   \
+       FRACTIONAL_FLOAT_MODE (N, -1U, Y, F, ORDER)
+#define FLOAT_MODE(N, Y, F)             FRACTIONAL_FLOAT_MODE (N, -1U, Y, F, 0)
+#define FRACTIONAL_FLOAT_MODE(N, B, Y, F, ORDER) \
+  make_float_mode (#N, B, Y, #F, ORDER, __FILE__, __LINE__)

   static void
   make_float_mode (const char *name,
                 unsigned int precision, unsigned int bytesize,
-                const char *format,
+                const char *format, unsigned int order,
                 const char *file, unsigned int line)
   {
     struct mode_data *m = new_mode (MODE_FLOAT, name, file, line);
     m->bytesize = bytesize;
     m->precision = precision;
     m->format = format;
+  m->order = order;
   }

   #define DECIMAL_FLOAT_MODE(N, Y, F)  \