This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH] x86: Determine vector length from the last vector operand
On Tue, Jul 24, 2018 at 6:57 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Jul 24, 2018 at 6:50 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 24.07.18 at 15:43, <hjl.tools@gmail.com> wrote:
>>> On Tue, Jul 24, 2018 at 5:34 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 24.07.18 at 14:08, <hjl.tools@gmail.com> wrote:
>>>>> On Tue, Jul 24, 2018 at 4:55 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>> On 24.07.18 at 13:38, <hjl.tools@gmail.com> wrote:
>>>>>>> On Tue, Jul 24, 2018 at 12:41 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>>>> On 22.07.18 at 21:02, <hjl.tools@gmail.com> wrote:
>>>>>>>>> Determine VEX/EVEXE vector length from the last multi-length vector
>>>>>>>>> operand.
>>>>>>>>
>>>>>>>> I see you've committed this already: It would have been really
>>>>>>>> helpful to say _why_ you're doing the change in the commit message.
>>>>>>>> For posterity as well as my understanding - could you at least do so
>>>>>>>> here please? That's even more so that VEX and EVEX processing
>>>>>>>> differed in that regard before your change.
>>>>>>>
>>>>>>> The encoding can be determined by the last multi-length vector
>>>>>>> operand. There is no need to scan from the start.
>>>>>>
>>>>>> But why is scanning from the rear better? Immediates usually sit at the
>>>>>> end. In the EVEX case, considering the break-s you add, this may
>>>>>> indeed be more efficient, but the same then wouldn't hold for the VEX
>>>>>
>>>>> break can be used only when scanning from the last operand.
>>>>
>>>> Yes - that's why I understand (even without the patch description
>>>> saying so) why this way it's more efficient in the EVEX case. The
>>>> VEX case, otoh, is still unclear to me.
>>>>
>>>>>>>>> --- a/gas/config/tc-i386.c
>>>>>>>>> +++ b/gas/config/tc-i386.c
>>>>>>>>> @@ -3363,10 +3363,12 @@ build_vex_prefix (const insn_template *t)
>>>>>>>>> vector_length = 1;
>>>>>>>>> else
>>>>>>>>> {
>>>>>>>>> - unsigned int op;
>>>>>>>>> + int op;
>>>>>>>>
>>>>>>>> This is the sort of change I would have objected to, btw. Variables
>>>>>>>> used to index arrays should be unsigned whenever possible. And
>>>>>>>> doing so would have been easy enough here:
>>>>>>>
>>>>>>> I want
>>>>>>>
>>>>>>> if (op < 0)
>>>>>>> abort ();
>>>>>>>
>>>>>>> after the loop.
>>>>>>
>>>>>> if (op >= MAX_OPERANDS)
>>>>>> abort ()
>>>>>>
>>>>>> would be equivalent afaict.
>>>>>
>>>>> It won't work when scanning from the last operand.
>>>>
>>>> Why would it not? The value will wrap through zero to UINT_MAX.
>>>
>>> I don't see there are significant differences here.
>>> It is perfectly OK to use "int" as index.
>>
>> Generated code is worse on many architectures: Often sub-machine-
>> word size loads (and on x86-64 also other operations) are zero-
>> extending. A signed index hence needs sign-extending in a separate
>> step, rather than being able to directly use the result of the load (or
>> other operation).
>>
>
> Sure. Go ahead if you want to change it to unsigned.
>
This is what I checked in.
--
H.J.
From 56522fc5af76ec88f650d8d305c0aa3d669c2849 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Tue, 24 Jul 2018 09:47:47 -0700
Subject: [PATCH] x86: Use unsigned int to iterate through vector operands
Use unsigned int to iterate through multi-length vector operands to avoid
sign-extension.
* config/tc-i386.c (build_vex_prefix): Use unsigned int to
iterate through multi-length vector operands.
(build_evex_prefix): Likewise.
---
gas/ChangeLog | 6 ++++++
gas/config/tc-i386.c | 10 +++++-----
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/gas/ChangeLog b/gas/ChangeLog
index 7fb92597f0..c0b6801f89 100644
--- a/gas/ChangeLog
+++ b/gas/ChangeLog
@@ -1,3 +1,9 @@
+2018-07-24 H.J. Lu <hongjiu.lu@intel.com>
+
+ * config/tc-i386.c (build_vex_prefix): Use unsigned int to
+ iterate through multi-length vector operands.
+ (build_evex_prefix): Likewise.
+
2018-07-24 Jan Beulich <jbeulich@suse.com>
* config/tc-i386.c (check_VecOperands): Handle EVEXLIG when
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 81643a73ac..bcd2904044 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -3362,12 +3362,12 @@ build_vex_prefix (const insn_template *t)
vector_length = 1;
else
{
- int op;
+ unsigned int op;
/* Determine vector length from the last multi-length vector
operand. */
vector_length = 0;
- for (op = t->operands - 1; op >= 0; op--)
+ for (op = t->operands; op--;)
if (t->operand_types[op].bitfield.xmmword
&& t->operand_types[op].bitfield.ymmword
&& i.types[op].bitfield.ymmword)
@@ -3612,12 +3612,12 @@ build_evex_prefix (void)
if (!i.tm.opcode_modifier.evex
|| i.tm.opcode_modifier.evex == EVEXDYN)
{
- int op;
+ unsigned int op;
/* Determine vector length from the last multi-length vector
operand. */
vec_length = 0;
- for (op = i.operands - 1; op >= 0; op--)
+ for (op = i.operands; op--;)
if (i.tm.operand_types[op].bitfield.xmmword
+ i.tm.operand_types[op].bitfield.ymmword
+ i.tm.operand_types[op].bitfield.zmmword > 1)
@@ -3658,7 +3658,7 @@ build_evex_prefix (void)
}
}
- if (op < 0)
+ if (op >= MAX_OPERANDS)
abort ();
}
--
2.17.1