Bug 18631 - avx512: wrong memory size for vcvtt?ps2u?qq instructions with data broadcast
Summary: avx512: wrong memory size for vcvtt?ps2u?qq instructions with data broadcast
Status: RESOLVED FIXED
Alias: None
Product: binutils
Classification: Unclassified
Component: binutils (show other bugs)
Version: 2.26
: P2 normal
Target Milestone: 2.26
Assignee: Alexander Fomin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-07 07:53 UTC by Ramiro Polla
Modified: 2015-07-22 20:30 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:


Attachments
Simple patch that should fix the issue (without testcase updates) (329 bytes, patch)
2015-07-07 07:54 UTC, Ramiro Polla
Details | Diff
A patch that should fix the issue (+ testcases update) (4.35 KB, patch)
2015-07-14 16:07 UTC, Alexander Fomin
Details | Diff
A patch that should fix the issue + testcases update/extension (14.62 KB, patch)
2015-07-16 14:29 UTC, Alexander Fomin
Details | Diff
A patch that should fix the issue + minimal testcases update/extension (7.11 KB, patch)
2015-07-17 11:56 UTC, Alexander Fomin
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ramiro Polla 2015-07-07 07:53:19 UTC
The vcvtps2qq, vcvtps2uqq, vcvttps2qq, and vcvttps2uqq instructions are incorrectly defined regarding the memory operand size with data broadcast for avx512. Simple example (with git master):

==========
$ cat vcvtps2qq.s
.text
vcvtps2qq (%rax){1to8}, %zmm0
$ as-new -o vcvtps2qq.o vcvtps2qq.s
$ objdump -Mintel -d vcvtps2qq.o 

vcvtps2qq.o:     file format elf64-x86-64


Disassembly of section .text:

0000000000000000 <.text>:
   0:	62 f1 7d 58 7b 00    	vcvtps2qq zmm0,YMMWORD PTR [rax]{1to8}
==========

It should be:

vcvtps2qq zmm0,DWORD PTR [rax]{1to8}
Comment 1 Ramiro Polla 2015-07-07 07:54:05 UTC
Created attachment 8418 [details]
Simple patch that should fix the issue (without testcase updates)
Comment 2 H.J. Lu 2015-07-07 12:14:10 UTC
We need to fix both assembler and disassembler.
Comment 3 Alexander Fomin 2015-07-14 16:07:45 UTC
Created attachment 8436 [details]
A patch that should fix the issue (+ testcases update)

This patch is based on the changes proposed by Ramiro Polla, but extends them with gas testsuite update.
Comment 4 H.J. Lu 2015-07-14 16:28:36 UTC
(In reply to Alexander Fomin from comment #3)
> Created attachment 8436 [details]
> A patch that should fix the issue (+ testcases update)
> 
> This patch is based on the changes proposed by Ramiro Polla, but extends
> them with gas testsuite update.

Please add assembly testcases for Intel syntax with memory size, like

vcvtps2xxx zmm0,DWORD PTR [rax]{1to8}

to

avx512dq.s
avx512dq_vl.s
avx512f.s
avx512f_vl.s
x86-64-avx512dq.s
x86-64-avx512dq_vl.s
x86-64-avx512f.s
x86-64-avx512f_vl.s
Comment 5 Alexander Fomin 2015-07-16 14:29:39 UTC
Created attachment 8442 [details]
A patch that should fix the issue + testcases update/extension

I've extended the testcases for AVX512DQ and AVX512DQ/VL both with masked and non-masked versions of vcvtt?ps2u?qq instructions.
As all these instructions require AVX512DQ or AVX512DQ/VL CPUID, I think we should not add them to (x86-64-)?avx512f(_vl)?.s files.
Comment 6 H.J. Lu 2015-07-16 14:49:59 UTC
(In reply to Alexander Fomin from comment #5)
> Created attachment 8442 [details]
> A patch that should fix the issue + testcases update/extension
> 
> I've extended the testcases for AVX512DQ and AVX512DQ/VL both with masked
> and non-masked versions of vcvtt?ps2u?qq instructions.
> As all these instructions require AVX512DQ or AVX512DQ/VL CPUID, I think we
> should not add them to (x86-64-)?avx512f(_vl)?.s files.

1. No need to add AT&T syntax tests.
2. For each instruction, only add one Intel syntax test for broadcast with
DWORD.
Comment 7 Alexander Fomin 2015-07-17 11:56:28 UTC
Created attachment 8447 [details]
A patch that should fix the issue + minimal testcases update/extension

The patch has been refined.
Only one Intel syntax test with broadcasting has been added for each vcvtt?ps2u?qq instruction.
Comment 8 H.J. Lu 2015-07-17 12:04:19 UTC
(In reply to Alexander Fomin from comment #7)
> Created attachment 8447 [details]
> A patch that should fix the issue + minimal testcases update/extension
> 
> The patch has been refined.
> Only one Intel syntax test with broadcasting has been added for each
> vcvtt?ps2u?qq instruction.

Looks good to me.  Please submit to binutils mailing list with
proper ChangeLog entries.  Thanks.
Comment 9 Sourceware Commits 2015-07-22 20:28:31 UTC
The master branch has been updated by H.J. Lu <hjl@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=20c2a615876bdf0d5dc8e2db86eea1312d9dc9b8

commit 20c2a615876bdf0d5dc8e2db86eea1312d9dc9b8
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Wed Jul 22 13:24:20 2015 -0700

    Fix memory operand size for vcvtt?ps2u?qq instructions
    
    When disassembling AVX512 vcvtt?ps2u?q instructions with data
    broadcasting enabled, memory operand size should be DWORD.
    
    gas/testsuite/
    
    	PR binutils/18631
    	* gas/i386avx512dq-intel.d: Replace "QWORD" with "DWORD" in
    	vcvtt?ps2u?qq instructions disassembly regexes.
    	Add disassembly regex for new test.
    	* gas/i386/avx512dq.d: Likewise.
    	* gas/i386/avx512dq_vl-intel.d: Likewise.
    	* gas/i386/avx512dq_vl.d: Likewise.
    	* gas/i386/x86-64-avx512dq-intel.d: Likewise.
    	* gas/i386/x86-64-avx512dq.d: Likewise.
    	* gas/i386/x86-64-avx512dq_vl-intel.d: Likewise.
    	* gas/i386/x86-64-avx512dq_vl.d: Likewise.
    	* gas/i386/avx512dq.s: Add new test for Intel syntax with memory
    	operand and broadcasting enabled.
    	* gas/i386/avx512dq_vl.s: Likewise.
    	* gas/i386/x86-64-avx512dq.s: Likewise.
    	* gas/i386/x86-64-avx512dq_vl.s: Likewise.
    
    opcodes/
    
    	PR binutils/18631
    	* i386-dis-evex.h (EVEX_W_0F78_P_2): Replace "EXxmmq" with
    	"EXEvexHalfBcstXmmq" for the second operand.
    	(EVEX_W_0F79_P_2): Likewise.
    	(EVEX_W_0F7A_P_2): Likewise.
    	(EVEX_W_0F7B_P_2): Likewise.
Comment 10 H.J. Lu 2015-07-22 20:30:06 UTC
Fixed.