[Patch] Fix ix86_expand_sse_comi_round (PR Target/89750, PR Target/86444)

[Jeff Law]

On 5/6/19 11:38 PM, Hongtao Liu wrote:
> Hi Uros and GCC:
>   This patch is to fix ix86_expand_sse_comi_round whose implementation
> was not correct.
>   New implentation aligns with _mm_cmp_round_s[sd]_mask.
> 
> Bootstrap and regression tests for x86 is fine.
> Ok for trunk?
> 
> 
> ChangeLog:
> gcc/
>    * config/i386/i386-expand.c (ix86_expand_sse_comi_round):
>    Modified, original implementation isn't correct.
> 
> gcc/testsuite
>    * gcc.target/i386/avx512f-vcomisd-2.c: New runtime tests.
>    * gcc.target/i386/avx512f-vcomisd-2.c: Likewise.
So you'll have to bear with me, I'm not really familiar with this code,
but in the absence of a maintainer I'll try to work through it.


> 
> -- BR, Hongtao
> 
> 
> 0001-Fix-ix86_expand_sse_comi_round.patch
> 
> Index: gcc/ChangeLog
> ===================================================================
> --- gcc/ChangeLog	(revision 270933)
> +++ gcc/ChangeLog	(working copy)
> @@ -1,3 +1,11 @@
> +2019-05-06  H.J. Lu  <hongjiu.lu@intel.com>
> +	    Hongtao Liu  <hongtao.liu@intel.com>
> +
> +	PR Target/89750
> +	PR Target/86444
> +	* config/i386/i386-expand.c (ix86_expand_sse_comi_round):
> +	Modified, original implementation isn't correct.
> +
>  2019-05-06  Segher Boessenkool  <segher@kernel.crashing.org>
>  
>  	* config/rs6000/rs6000.md (FIRST_ALTIVEC_REGNO, LAST_ALTIVEC_REGNO)
> Index: gcc/config/i386/i386-expand.c
> ===================================================================
> --- gcc/config/i386/i386-expand.c	(revision 270933)
> +++ gcc/config/i386/i386-expand.c	(working copy)
> @@ -9853,18 +9853,24 @@
>    const struct insn_data_d *insn_p = &insn_data[icode];
>    machine_mode mode0 = insn_p->operand[0].mode;
>    machine_mode mode1 = insn_p->operand[1].mode;
> -  enum rtx_code comparison = UNEQ;
> -  bool need_ucomi = false;
>  
>    /* See avxintrin.h for values.  */
> -  enum rtx_code comi_comparisons[32] =
> +  static const enum rtx_code comparisons[32] =
So I assume the comment refers to the _CMP_* #defines in avxintrin.h?


>      {
> -      UNEQ, GT, GE, UNORDERED, LTGT, UNLE, UNLT, ORDERED, UNEQ, UNLT,
> -      UNLE, LT, LTGT, GE, GT, LT, UNEQ, GT, GE, UNORDERED, LTGT, UNLE,
> -      UNLT, ORDERED, UNEQ, UNLT, UNLE, LT, LTGT, GE, GT, LT
> +      EQ, LT, LE, UNORDERED, NE, UNGE, UNGT, ORDERED,
> +      EQ, UNLT, UNLE, UNORDERED, LTGT, GE, GT, ORDERED,
> +      EQ, LT, LE, UNORDERED, NE, UNGE, UNGT, ORDERED,
> +      EQ, UNLT, UNLE, UNORDERED, LTGT, GE, GT, ORDERED
>      };

For CMP_EQ_UQ aren't we looking for an unordered comparison, so UNEQ
seems right, but you're using EQ.  Can you double-check this?  If it's
wrong, then please make sure we cover this case with a test.




> @@ -9932,11 +10021,37 @@
>      }
>  
>    emit_insn (pat);
> +
> +  /* XXX: Set CCFPmode and check a different CCmode.  Does it work
> +     correctly?  */
> +  if (GET_MODE (set_dst) != mode)
> +    set_dst = gen_rtx_REG (mode, REGNO (set_dst));
This looks worrisome, even without the cryptic comment.  I don't think
you can just blindly change the mode like that.  Unless you happen to
know that the only things you test in the new mode were set in precisely
the same way as the old mode.

Jeff