Re: [PATCH] x86: fix AVX-512 16-bit addressing

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Wed, Nov 22, 2017 at 8:18 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 22.11.17 at 16:52, <hjl.tools@gmail.com> wrote:
>> On Wed, Nov 22, 2017 at 7:46 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 22.11.17 at 16:36, <hjl.tools@gmail.com> wrote:
>>>> On Wed, Nov 22, 2017 at 6:18 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> On 22.11.17 at 14:51, <hjl.tools@gmail.com> wrote:
>>>>>> On Wed, Nov 22, 2017 at 5:26 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>> On 21.11.17 at 20:06, <hjl.tools@gmail.com> wrote:
>>>>>>>> On Mon, Nov 20, 2017 at 7:15 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>> --- a/gas/config/tc-i386.c
>>>>>>>>> +++ b/gas/config/tc-i386.c
>>>>>>>>> @@ -4799,11 +4799,9 @@ check_VecOperands (const insn_template *
>>>>>>>>>                 else
>>>>>>>>>                   {
>>>>>>>>>                     /* Vector insn can only have Vec_Disp8/Disp32 in
>>>>>>>>> -                      32/64bit modes, and Vec_Disp8/Disp16 in 16bit
>>>>>>>>> -                      mode.  */
>>>>>>>>> +                      64bit mode, and Vec_Disp8/Disp16/Disp32 in 16/32bit
>>>>>>>>> +                      modes.  */
>>>>>>>>
>>>>>>>> Do we really support 32-bit displacement in 16-bit mode or with 0x67
>>>>>>>> address prefix?
>>>>>>>
>>>>>>> What a strange question: Of course we do, and why would we not?
>>>>>>> Using 32-bit addresses in 16-bit mode is quite useful, and using 16-bit
>>>>>>> addresses in 32-bit mode is at least not illegal. And quite obviously
>>>>>>> there should be no difference between EVEX encoded insns and any
>>>>>>> other ones.
>>>>>>
>>>>>> We need tests for 32-bit displacement with and without relocations for
>>>>>> .code16 as well as 0x67 prefix.
>>>>>
>>>>> You're kidding? The patch doesn't change the behavior of .code16
>>>>> at all! All it changes is the behavior for .code32 when using 16-bit
>>>>> addressing (assembler) and 16-bit code _without_ the 0x67 prefix
>>>>> (disassembler). If you had cared about testing what you say above,
>>>>> you should have long added a test case.
>>>>>
>>>>> Please can you stop inventing requirements for bug fixes to go in?
>>>>
>>>> If we don't know for sure that Disp32 works correctly for 16-bit code,
>>>> we shouldn't allow Disp32 for 16-bit instructions.
>>>
>>> That's a possibility (albeit a bad one, and I would veto a such a
>>> change if it affected Intel syntax mode as well), yet once again -
>>> it's completely orthogonal to what the patch does. The comment
>>> above that I'm changing simply is not true from an architecture
>>> perspective, no matter what the assembler actually means to
>>> support. But if you prefer I can leave the comment alone
>>> (ignoring the fact that it's then becoming not only wrong, but
>>> also stale) and remove only the two lines of code which cause
>>> the bug.
>>
>> I'd like to try your patch.  But your mailer mangled it.   It will
>> be easier for me if you create a git branch for your change.
>
> Here it is as an attachment (albeit I'm sending many patches to
> other lists, and nowadays they make through unmangled there).
> I'm not enough into git to start playing with branches.

          /* Vector insn can only have Vec_Disp8/Disp32 in
-            32/64bit modes, and Vec_Disp8/Disp16 in 16bit
-            mode.  */
+            64bit mode, and Vec_Disp8/Disp16/Disp32 in 16/32bit
+            modes.  */
          i.types[op].bitfield.disp8 = 0;
-         if (flag_code != CODE_16BIT)
-           i.types[op].bitfield.disp16 = 0;

Please change the comment to

/* Vector insn doesn't allow Disp8.   */

since Disp16/Disp32 are checked/handled later.

OK with this change.

Thanks.

-- 
H.J.