[1/9] x86: refine TPAUSE and UMWAIT

Message ID 3923caf7-f52b-a5d3-890f-4ffe312320e9@suse.com
State Superseded
Headers show
Series
  • x86: (mainly) misc IgnoreSize related adjustments
Related show

Commit Message

Jan Beulich March 4, 2020, 9:37 a.m.
Allowing 64-bit registers is misleading here: Elsewhere these get allowed
when there's no difference between either variant, because of 32-bit
destination registers having their upper halves zeroed in 64-bit mode.
Here, however, they're source registers, and hence specifying 64-bit
registers would lead to the ambiguity of whether the upper 32 bits
actually matter.

Additionally, for proper code generation in 16-bit mode, IgnoreSize is
needed on both.

And finally, just like for e.g. MONITOR/MWAIT, add variants with all
input registers explicitly specified.

gas/
2020-03-XX  Jan Beulich  <jbeulich@suse.com>

	* config/tc-i386.c (md_assemble): Also exclude tpause and umwait
	from having their operands swapped.
	* testsuite/gas/i386/waitpkg.s,
	testsuite/gas/i386/x86-64-waitpkg.s: Add tpause and umwait
	3-operand cases.
	* testsuite/gas/i386/waitpkg.d,
	testsuite/gas/i386/waitpkg-intel.d,
	testsuite/gas/i386/x86-64-waitpkg.d,
	testsuite/gas/i386/x86-64-waitpkg-intel.d: Adjust expectations.

opcodes/
2020-03-XX  Jan Beulich  <jbeulich@suse.com>

	* i386-opc.tbl (tpause, umwait): Add IgnoreSize. Add 3-operand
	template.
	* i386-tbl.h: Re-generate.

Comments

H.J. Lu March 4, 2020, 11:36 a.m. | #1
On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

> when there's no difference between either variant, because of 32-bit

> destination registers having their upper halves zeroed in 64-bit mode.

> Here, however, they're source registers, and hence specifying 64-bit

> registers would lead to the ambiguity of whether the upper 32 bits

> actually matter.

>

> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

> needed on both.


Are there testcases to show IgnoreSize is needed on them?

> And finally, just like for e.g. MONITOR/MWAIT, add variants with all

> input registers explicitly specified.

>

> gas/

> 2020-03-XX  Jan Beulich  <jbeulich@suse.com>

>

>         * config/tc-i386.c (md_assemble): Also exclude tpause and umwait

>         from having their operands swapped.

>         * testsuite/gas/i386/waitpkg.s,

>         testsuite/gas/i386/x86-64-waitpkg.s: Add tpause and umwait

>         3-operand cases.

>         * testsuite/gas/i386/waitpkg.d,

>         testsuite/gas/i386/waitpkg-intel.d,

>         testsuite/gas/i386/x86-64-waitpkg.d,

>         testsuite/gas/i386/x86-64-waitpkg-intel.d: Adjust expectations.

>

> opcodes/

> 2020-03-XX  Jan Beulich  <jbeulich@suse.com>

>

>         * i386-opc.tbl (tpause, umwait): Add IgnoreSize. Add 3-operand

>         template.

>         * i386-tbl.h: Re-generate.



--
H.J.
Jan Beulich March 4, 2020, 11:40 a.m. | #2
On 04.03.2020 12:36, H.J. Lu wrote:
> On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

>> when there's no difference between either variant, because of 32-bit

>> destination registers having their upper halves zeroed in 64-bit mode.

>> Here, however, they're source registers, and hence specifying 64-bit

>> registers would lead to the ambiguity of whether the upper 32 bits

>> actually matter.

>>

>> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

>> needed on both.

> 

> Are there testcases to show IgnoreSize is needed on them?


The situation with 16-bit test cases is rather poor anyway. I didn't
consider it reasonable to add such very special ones when far more
general ones don't exist. But if your question is to mean you demand
such to be added, then I'll (somewhat hesitantly) add/extend some.
Please clarify.

Jan
H.J. Lu March 4, 2020, 11:44 a.m. | #3
On Wed, Mar 4, 2020 at 3:40 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 04.03.2020 12:36, H.J. Lu wrote:

> > On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

> >> when there's no difference between either variant, because of 32-bit

> >> destination registers having their upper halves zeroed in 64-bit mode.

> >> Here, however, they're source registers, and hence specifying 64-bit

> >> registers would lead to the ambiguity of whether the upper 32 bits

> >> actually matter.

> >>

> >> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

> >> needed on both.

> >

> > Are there testcases to show IgnoreSize is needed on them?

>

> The situation with 16-bit test cases is rather poor anyway. I didn't

> consider it reasonable to add such very special ones when far more

> general ones don't exist. But if your question is to mean you demand


Let's start from somewhere.

> such to be added, then I'll (somewhat hesitantly) add/extend some.

> Please clarify.


Please add testcases.

Thanks.

-- 
H.J.
Jan Beulich March 5, 2020, 8:08 a.m. | #4
On 04.03.2020 12:44, H.J. Lu wrote:
> On Wed, Mar 4, 2020 at 3:40 AM Jan Beulich <jbeulich@suse.com> wrote:

>> On 04.03.2020 12:36, H.J. Lu wrote:

>>> On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

>>>> when there's no difference between either variant, because of 32-bit

>>>> destination registers having their upper halves zeroed in 64-bit mode.

>>>> Here, however, they're source registers, and hence specifying 64-bit

>>>> registers would lead to the ambiguity of whether the upper 32 bits

>>>> actually matter.

>>>>

>>>> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

>>>> needed on both.

>>>

>>> Are there testcases to show IgnoreSize is needed on them?

>>

>> The situation with 16-bit test cases is rather poor anyway. I didn't

>> consider it reasonable to add such very special ones when far more

>> general ones don't exist. But if your question is to mean you demand

> 

> Let's start from somewhere.

> 

>> such to be added, then I'll (somewhat hesitantly) add/extend some.

>> Please clarify.

> 

> Please add testcases.


Actually they were there, in patch 2. I've moved them to this patch
and have just sent v1.1 for just this one patch.

Jan
H.J. Lu March 5, 2020, 2:04 p.m. | #5
On Thu, Mar 5, 2020 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 04.03.2020 12:44, H.J. Lu wrote:

> > On Wed, Mar 4, 2020 at 3:40 AM Jan Beulich <jbeulich@suse.com> wrote:

> >> On 04.03.2020 12:36, H.J. Lu wrote:

> >>> On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

> >>>> when there's no difference between either variant, because of 32-bit

> >>>> destination registers having their upper halves zeroed in 64-bit mode.

> >>>> Here, however, they're source registers, and hence specifying 64-bit

> >>>> registers would lead to the ambiguity of whether the upper 32 bits

> >>>> actually matter.

> >>>>

> >>>> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

> >>>> needed on both.

> >>>

> >>> Are there testcases to show IgnoreSize is needed on them?

> >>

> >> The situation with 16-bit test cases is rather poor anyway. I didn't

> >> consider it reasonable to add such very special ones when far more

> >> general ones don't exist. But if your question is to mean you demand

> >

> > Let's start from somewhere.

> >

> >> such to be added, then I'll (somewhat hesitantly) add/extend some.

> >> Please clarify.

> >

> > Please add testcases.

>

> Actually they were there, in patch 2. I've moved them to this patch

> and have just sent v1.1 for just this one patch.


Do we need to adjust disassembler for 16-bit mode?

-- 
H.J.
Jan Beulich March 5, 2020, 2:08 p.m. | #6
On 05.03.2020 15:04, H.J. Lu wrote:
> On Thu, Mar 5, 2020 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> On 04.03.2020 12:44, H.J. Lu wrote:

>>> On Wed, Mar 4, 2020 at 3:40 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>> On 04.03.2020 12:36, H.J. Lu wrote:

>>>>> On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>>> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

>>>>>> when there's no difference between either variant, because of 32-bit

>>>>>> destination registers having their upper halves zeroed in 64-bit mode.

>>>>>> Here, however, they're source registers, and hence specifying 64-bit

>>>>>> registers would lead to the ambiguity of whether the upper 32 bits

>>>>>> actually matter.

>>>>>>

>>>>>> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

>>>>>> needed on both.

>>>>>

>>>>> Are there testcases to show IgnoreSize is needed on them?

>>>>

>>>> The situation with 16-bit test cases is rather poor anyway. I didn't

>>>> consider it reasonable to add such very special ones when far more

>>>> general ones don't exist. But if your question is to mean you demand

>>>

>>> Let's start from somewhere.

>>>

>>>> such to be added, then I'll (somewhat hesitantly) add/extend some.

>>>> Please clarify.

>>>

>>> Please add testcases.

>>

>> Actually they were there, in patch 2. I've moved them to this patch

>> and have just sent v1.1 for just this one patch.

> 

> Do we need to adjust disassembler for 16-bit mode?


I haven't checked, but (a) this would be an independent change
and (b) it would likely affect more than just the insns here
(see e.g. other parts of the series).

Jan
H.J. Lu March 5, 2020, 2:37 p.m. | #7
On Thu, Mar 5, 2020 at 6:08 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 05.03.2020 15:04, H.J. Lu wrote:

> > On Thu, Mar 5, 2020 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> On 04.03.2020 12:44, H.J. Lu wrote:

> >>> On Wed, Mar 4, 2020 at 3:40 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>> On 04.03.2020 12:36, H.J. Lu wrote:

> >>>>> On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>>> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

> >>>>>> when there's no difference between either variant, because of 32-bit

> >>>>>> destination registers having their upper halves zeroed in 64-bit mode.

> >>>>>> Here, however, they're source registers, and hence specifying 64-bit

> >>>>>> registers would lead to the ambiguity of whether the upper 32 bits

> >>>>>> actually matter.

> >>>>>>

> >>>>>> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

> >>>>>> needed on both.

> >>>>>

> >>>>> Are there testcases to show IgnoreSize is needed on them?

> >>>>

> >>>> The situation with 16-bit test cases is rather poor anyway. I didn't

> >>>> consider it reasonable to add such very special ones when far more

> >>>> general ones don't exist. But if your question is to mean you demand

> >>>

> >>> Let's start from somewhere.

> >>>

> >>>> such to be added, then I'll (somewhat hesitantly) add/extend some.

> >>>> Please clarify.

> >>>

> >>> Please add testcases.

> >>

> >> Actually they were there, in patch 2. I've moved them to this patch

> >> and have just sent v1.1 for just this one patch.

> >

> > Do we need to adjust disassembler for 16-bit mode?

>

> I haven't checked, but (a) this would be an independent change

> and (b) it would likely affect more than just the insns here

> (see e.g. other parts of the series).

>


Disassembler should be adjusted when IgnoreSize is added
to tpause and umwait.

-- 
H.J.
Jan Beulich March 5, 2020, 2:51 p.m. | #8
On 05.03.2020 15:37, H.J. Lu wrote:
> On Thu, Mar 5, 2020 at 6:08 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> On 05.03.2020 15:04, H.J. Lu wrote:

>>> On Thu, Mar 5, 2020 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>

>>>> On 04.03.2020 12:44, H.J. Lu wrote:

>>>>> On Wed, Mar 4, 2020 at 3:40 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>>> On 04.03.2020 12:36, H.J. Lu wrote:

>>>>>>> On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>>>>> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

>>>>>>>> when there's no difference between either variant, because of 32-bit

>>>>>>>> destination registers having their upper halves zeroed in 64-bit mode.

>>>>>>>> Here, however, they're source registers, and hence specifying 64-bit

>>>>>>>> registers would lead to the ambiguity of whether the upper 32 bits

>>>>>>>> actually matter.

>>>>>>>>

>>>>>>>> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

>>>>>>>> needed on both.

>>>>>>>

>>>>>>> Are there testcases to show IgnoreSize is needed on them?

>>>>>>

>>>>>> The situation with 16-bit test cases is rather poor anyway. I didn't

>>>>>> consider it reasonable to add such very special ones when far more

>>>>>> general ones don't exist. But if your question is to mean you demand

>>>>>

>>>>> Let's start from somewhere.

>>>>>

>>>>>> such to be added, then I'll (somewhat hesitantly) add/extend some.

>>>>>> Please clarify.

>>>>>

>>>>> Please add testcases.

>>>>

>>>> Actually they were there, in patch 2. I've moved them to this patch

>>>> and have just sent v1.1 for just this one patch.

>>>

>>> Do we need to adjust disassembler for 16-bit mode?

>>

>> I haven't checked, but (a) this would be an independent change

>> and (b) it would likely affect more than just the insns here

>> (see e.g. other parts of the series).

> 

> Disassembler should be adjusted when IgnoreSize is added

> to tpause and umwait.


But IgnoreSize has nothing to do with the disassembler. Right
now all I'm after is getting the assembler to produce correct
code. There are test cases to verify this is the case. Whether
the disassembler deals with any of this correctly (including
also the insns getting IgnoreSize added by "x86: add missing
IgnoreSize", which you've already approved) is an entirely
separate topic, which I may be willing to look into down the
road, but not now and here.

More generally, when anyone fixes one bug, why should they get
penalized to have their bug fix accepted only when they take
the - perhaps significant amount of - time to also fix another,
unrelated bug? In the case here, it is definitely not my fault
if 16-bit handling is in a bad state. And I can't afford fixing
all issues there are in one go.

Jan
H.J. Lu March 5, 2020, 2:53 p.m. | #9
On Thu, Mar 5, 2020 at 6:51 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 05.03.2020 15:37, H.J. Lu wrote:

> > On Thu, Mar 5, 2020 at 6:08 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> On 05.03.2020 15:04, H.J. Lu wrote:

> >>> On Thu, Mar 5, 2020 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>

> >>>> On 04.03.2020 12:44, H.J. Lu wrote:

> >>>>> On Wed, Mar 4, 2020 at 3:40 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>>> On 04.03.2020 12:36, H.J. Lu wrote:

> >>>>>>> On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>>>>> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

> >>>>>>>> when there's no difference between either variant, because of 32-bit

> >>>>>>>> destination registers having their upper halves zeroed in 64-bit mode.

> >>>>>>>> Here, however, they're source registers, and hence specifying 64-bit

> >>>>>>>> registers would lead to the ambiguity of whether the upper 32 bits

> >>>>>>>> actually matter.

> >>>>>>>>

> >>>>>>>> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

> >>>>>>>> needed on both.

> >>>>>>>

> >>>>>>> Are there testcases to show IgnoreSize is needed on them?

> >>>>>>

> >>>>>> The situation with 16-bit test cases is rather poor anyway. I didn't

> >>>>>> consider it reasonable to add such very special ones when far more

> >>>>>> general ones don't exist. But if your question is to mean you demand

> >>>>>

> >>>>> Let's start from somewhere.

> >>>>>

> >>>>>> such to be added, then I'll (somewhat hesitantly) add/extend some.

> >>>>>> Please clarify.

> >>>>>

> >>>>> Please add testcases.

> >>>>

> >>>> Actually they were there, in patch 2. I've moved them to this patch

> >>>> and have just sent v1.1 for just this one patch.

> >>>

> >>> Do we need to adjust disassembler for 16-bit mode?

> >>

> >> I haven't checked, but (a) this would be an independent change

> >> and (b) it would likely affect more than just the insns here

> >> (see e.g. other parts of the series).

> >

> > Disassembler should be adjusted when IgnoreSize is added

> > to tpause and umwait.

>

> But IgnoreSize has nothing to do with the disassembler. Right

> now all I'm after is getting the assembler to produce correct

> code. There are test cases to verify this is the case. Whether

> the disassembler deals with any of this correctly (including

> also the insns getting IgnoreSize added by "x86: add missing

> IgnoreSize", which you've already approved) is an entirely

> separate topic, which I may be willing to look into down the

> road, but not now and here.

>

> More generally, when anyone fixes one bug, why should they get

> penalized to have their bug fix accepted only when they take

> the - perhaps significant amount of - time to also fix another,

> unrelated bug? In the case here, it is definitely not my fault

> if 16-bit handling is in a bad state. And I can't afford fixing

> all issues there are in one go.

>


Then don't add IgnoreSize to tpause and umwait for now.

-- 
H.J.
Jan Beulich March 5, 2020, 3:16 p.m. | #10
On 05.03.2020 15:53, H.J. Lu wrote:
> On Thu, Mar 5, 2020 at 6:51 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> On 05.03.2020 15:37, H.J. Lu wrote:

>>> On Thu, Mar 5, 2020 at 6:08 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>

>>>> On 05.03.2020 15:04, H.J. Lu wrote:

>>>>> On Thu, Mar 5, 2020 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>>>

>>>>>> On 04.03.2020 12:44, H.J. Lu wrote:

>>>>>>> On Wed, Mar 4, 2020 at 3:40 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>>>>> On 04.03.2020 12:36, H.J. Lu wrote:

>>>>>>>>> On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>>>>>>> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

>>>>>>>>>> when there's no difference between either variant, because of 32-bit

>>>>>>>>>> destination registers having their upper halves zeroed in 64-bit mode.

>>>>>>>>>> Here, however, they're source registers, and hence specifying 64-bit

>>>>>>>>>> registers would lead to the ambiguity of whether the upper 32 bits

>>>>>>>>>> actually matter.

>>>>>>>>>>

>>>>>>>>>> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

>>>>>>>>>> needed on both.

>>>>>>>>>

>>>>>>>>> Are there testcases to show IgnoreSize is needed on them?

>>>>>>>>

>>>>>>>> The situation with 16-bit test cases is rather poor anyway. I didn't

>>>>>>>> consider it reasonable to add such very special ones when far more

>>>>>>>> general ones don't exist. But if your question is to mean you demand

>>>>>>>

>>>>>>> Let's start from somewhere.

>>>>>>>

>>>>>>>> such to be added, then I'll (somewhat hesitantly) add/extend some.

>>>>>>>> Please clarify.

>>>>>>>

>>>>>>> Please add testcases.

>>>>>>

>>>>>> Actually they were there, in patch 2. I've moved them to this patch

>>>>>> and have just sent v1.1 for just this one patch.

>>>>>

>>>>> Do we need to adjust disassembler for 16-bit mode?

>>>>

>>>> I haven't checked, but (a) this would be an independent change

>>>> and (b) it would likely affect more than just the insns here

>>>> (see e.g. other parts of the series).

>>>

>>> Disassembler should be adjusted when IgnoreSize is added

>>> to tpause and umwait.

>>

>> But IgnoreSize has nothing to do with the disassembler. Right

>> now all I'm after is getting the assembler to produce correct

>> code. There are test cases to verify this is the case. Whether

>> the disassembler deals with any of this correctly (including

>> also the insns getting IgnoreSize added by "x86: add missing

>> IgnoreSize", which you've already approved) is an entirely

>> separate topic, which I may be willing to look into down the

>> road, but not now and here.

>>

>> More generally, when anyone fixes one bug, why should they get

>> penalized to have their bug fix accepted only when they take

>> the - perhaps significant amount of - time to also fix another,

>> unrelated bug? In the case here, it is definitely not my fault

>> if 16-bit handling is in a bad state. And I can't afford fixing

>> all issues there are in one go.

> 

> Then don't add IgnoreSize to tpause and umwait for now.


You're kidding. Once again - where is the connection between
adding IgnoreSize and disassembler behavior? I want code to
be generated correctly, no more and no less.

Jan
Jan Beulich March 5, 2020, 3:22 p.m. | #11
On 05.03.2020 15:04, H.J. Lu wrote:
> On Thu, Mar 5, 2020 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> On 04.03.2020 12:44, H.J. Lu wrote:

>>> On Wed, Mar 4, 2020 at 3:40 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>> On 04.03.2020 12:36, H.J. Lu wrote:

>>>>> On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>>> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

>>>>>> when there's no difference between either variant, because of 32-bit

>>>>>> destination registers having their upper halves zeroed in 64-bit mode.

>>>>>> Here, however, they're source registers, and hence specifying 64-bit

>>>>>> registers would lead to the ambiguity of whether the upper 32 bits

>>>>>> actually matter.

>>>>>>

>>>>>> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

>>>>>> needed on both.

>>>>>

>>>>> Are there testcases to show IgnoreSize is needed on them?

>>>>

>>>> The situation with 16-bit test cases is rather poor anyway. I didn't

>>>> consider it reasonable to add such very special ones when far more

>>>> general ones don't exist. But if your question is to mean you demand

>>>

>>> Let's start from somewhere.

>>>

>>>> such to be added, then I'll (somewhat hesitantly) add/extend some.

>>>> Please clarify.

>>>

>>> Please add testcases.

>>

>> Actually they were there, in patch 2. I've moved them to this patch

>> and have just sent v1.1 for just this one patch.

> 

> Do we need to adjust disassembler for 16-bit mode?


I've checked now - no, we don't.

Jan
H.J. Lu March 5, 2020, 3:37 p.m. | #12
On Thu, Mar 5, 2020 at 7:22 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 05.03.2020 15:04, H.J. Lu wrote:

> > On Thu, Mar 5, 2020 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> On 04.03.2020 12:44, H.J. Lu wrote:

> >>> On Wed, Mar 4, 2020 at 3:40 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>> On 04.03.2020 12:36, H.J. Lu wrote:

> >>>>> On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>>> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

> >>>>>> when there's no difference between either variant, because of 32-bit

> >>>>>> destination registers having their upper halves zeroed in 64-bit mode.

> >>>>>> Here, however, they're source registers, and hence specifying 64-bit

> >>>>>> registers would lead to the ambiguity of whether the upper 32 bits

> >>>>>> actually matter.

> >>>>>>

> >>>>>> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

> >>>>>> needed on both.

> >>>>>

> >>>>> Are there testcases to show IgnoreSize is needed on them?

> >>>>

> >>>> The situation with 16-bit test cases is rather poor anyway. I didn't

> >>>> consider it reasonable to add such very special ones when far more

> >>>> general ones don't exist. But if your question is to mean you demand

> >>>

> >>> Let's start from somewhere.

> >>>

> >>>> such to be added, then I'll (somewhat hesitantly) add/extend some.

> >>>> Please clarify.

> >>>

> >>> Please add testcases.

> >>

> >> Actually they were there, in patch 2. I've moved them to this patch

> >> and have just sent v1.1 for just this one patch.

> >

> > Do we need to adjust disassembler for 16-bit mode?

>

> I've checked now - no, we don't.

>


So disassembler has been correct.  Is the bug only in assembler?
What is the difference in encoding before and after your change?

-- 
H.J.
Jan Beulich March 5, 2020, 3:42 p.m. | #13
On 05.03.2020 16:37, H.J. Lu wrote:
> On Thu, Mar 5, 2020 at 7:22 AM Jan Beulich <jbeulich@suse.com> wrote:

>>

>> On 05.03.2020 15:04, H.J. Lu wrote:

>>> On Thu, Mar 5, 2020 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>

>>>> On 04.03.2020 12:44, H.J. Lu wrote:

>>>>> On Wed, Mar 4, 2020 at 3:40 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>>> On 04.03.2020 12:36, H.J. Lu wrote:

>>>>>>> On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

>>>>>>>> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

>>>>>>>> when there's no difference between either variant, because of 32-bit

>>>>>>>> destination registers having their upper halves zeroed in 64-bit mode.

>>>>>>>> Here, however, they're source registers, and hence specifying 64-bit

>>>>>>>> registers would lead to the ambiguity of whether the upper 32 bits

>>>>>>>> actually matter.

>>>>>>>>

>>>>>>>> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

>>>>>>>> needed on both.

>>>>>>>

>>>>>>> Are there testcases to show IgnoreSize is needed on them?

>>>>>>

>>>>>> The situation with 16-bit test cases is rather poor anyway. I didn't

>>>>>> consider it reasonable to add such very special ones when far more

>>>>>> general ones don't exist. But if your question is to mean you demand

>>>>>

>>>>> Let's start from somewhere.

>>>>>

>>>>>> such to be added, then I'll (somewhat hesitantly) add/extend some.

>>>>>> Please clarify.

>>>>>

>>>>> Please add testcases.

>>>>

>>>> Actually they were there, in patch 2. I've moved them to this patch

>>>> and have just sent v1.1 for just this one patch.

>>>

>>> Do we need to adjust disassembler for 16-bit mode?

>>

>> I've checked now - no, we don't.

>>

> 

> So disassembler has been correct.  Is the bug only in assembler?

> What is the difference in encoding before and after your change?


UMWAIT gets a stray 66 prefix emitted, and TPAUSE fails to
assemble ("same type of prefix used twice").

Jan
H.J. Lu March 5, 2020, 4 p.m. | #14
On Thu, Mar 5, 2020 at 7:42 AM Jan Beulich <jbeulich@suse.com> wrote:
>

> On 05.03.2020 16:37, H.J. Lu wrote:

> > On Thu, Mar 5, 2020 at 7:22 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>

> >> On 05.03.2020 15:04, H.J. Lu wrote:

> >>> On Thu, Mar 5, 2020 at 12:08 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>

> >>>> On 04.03.2020 12:44, H.J. Lu wrote:

> >>>>> On Wed, Mar 4, 2020 at 3:40 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>>> On 04.03.2020 12:36, H.J. Lu wrote:

> >>>>>>> On Wed, Mar 4, 2020 at 1:37 AM Jan Beulich <jbeulich@suse.com> wrote:

> >>>>>>>> Allowing 64-bit registers is misleading here: Elsewhere these get allowed

> >>>>>>>> when there's no difference between either variant, because of 32-bit

> >>>>>>>> destination registers having their upper halves zeroed in 64-bit mode.

> >>>>>>>> Here, however, they're source registers, and hence specifying 64-bit

> >>>>>>>> registers would lead to the ambiguity of whether the upper 32 bits

> >>>>>>>> actually matter.

> >>>>>>>>

> >>>>>>>> Additionally, for proper code generation in 16-bit mode, IgnoreSize is

> >>>>>>>> needed on both.

> >>>>>>>

> >>>>>>> Are there testcases to show IgnoreSize is needed on them?

> >>>>>>

> >>>>>> The situation with 16-bit test cases is rather poor anyway. I didn't

> >>>>>> consider it reasonable to add such very special ones when far more

> >>>>>> general ones don't exist. But if your question is to mean you demand

> >>>>>

> >>>>> Let's start from somewhere.

> >>>>>

> >>>>>> such to be added, then I'll (somewhat hesitantly) add/extend some.

> >>>>>> Please clarify.

> >>>>>

> >>>>> Please add testcases.

> >>>>

> >>>> Actually they were there, in patch 2. I've moved them to this patch

> >>>> and have just sent v1.1 for just this one patch.

> >>>

> >>> Do we need to adjust disassembler for 16-bit mode?

> >>

> >> I've checked now - no, we don't.

> >>

> >

> > So disassembler has been correct.  Is the bug only in assembler?

> > What is the difference in encoding before and after your change?

>

> UMWAIT gets a stray 66 prefix emitted, and TPAUSE fails to

> assemble ("same type of prefix used twice").

>


Patch is OK.

Thanks.

-- 
H.J.

Patch

--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -4328,16 +4328,19 @@  md_assemble (char *line)
   /* Now we've parsed the mnemonic into a set of templates, and have the
      operands at hand.  */
 
-  /* All Intel opcodes have reversed operands except for "bound", "enter"
-     "monitor*", and "mwait*".  We also don't reverse intersegment "jmp"
-     and "call" instructions with 2 immediate operands so that the immediate
-     segment precedes the offset, as it does when in AT&T mode. */
+  /* All Intel opcodes have reversed operands except for "bound", "enter",
+     "monitor*", "mwait*", "tpause", and "umwait".  We also don't reverse
+     intersegment "jmp" and "call" instructions with 2 immediate operands so
+     that the immediate segment precedes the offset, as it does when in AT&T
+     mode.  */
   if (intel_syntax
       && i.operands > 1
       && (strcmp (mnemonic, "bound") != 0)
       && (strcmp (mnemonic, "invlpga") != 0)
       && (strncmp (mnemonic, "monitor", 7) != 0)
       && (strncmp (mnemonic, "mwait", 5) != 0)
+      && (strcmp (mnemonic, "tpause") != 0)
+      && (strcmp (mnemonic, "umwait") != 0)
       && !(operand_type_check (i.types[0], imm)
 	   && operand_type_check (i.types[1], imm)))
     swap_operands ();
--- a/gas/testsuite/gas/i386/waitpkg-intel.d
+++ b/gas/testsuite/gas/i386/waitpkg-intel.d
@@ -12,5 +12,9 @@  Disassembly of section \.text:
 [ 	]*[a-f0-9]+:[ 	]*f3 0f ae f0[ 	]*umonitor eax
 [ 	]*[a-f0-9]+:[ 	]*67 f3 0f ae f1[ 	]*umonitor cx
 [ 	]*[a-f0-9]+:[ 	]*f2 0f ae f1[ 	]*umwait ecx
+[ 	]*[a-f0-9]+:[ 	]*f2 0f ae f3[ 	]*umwait ebx
 [ 	]*[a-f0-9]+:[ 	]*66 0f ae f1[ 	]*tpause ecx
+[ 	]*[a-f0-9]+:[ 	]*66 0f ae f3[ 	]*tpause ebx
+[ 	]*[a-f0-9]+:[ 	]*f2 0f ae f7[ 	]*umwait edi
+[ 	]*[a-f0-9]+:[ 	]*66 0f ae f7[ 	]*tpause edi
 #pass
--- a/gas/testsuite/gas/i386/waitpkg.d
+++ b/gas/testsuite/gas/i386/waitpkg.d
@@ -12,5 +12,9 @@  Disassembly of section \.text:
 [ 	]*[a-f0-9]+:[ 	]*f3 0f ae f0[ 	]*umonitor %eax
 [ 	]*[a-f0-9]+:[ 	]*67 f3 0f ae f1[ 	]*umonitor %cx
 [ 	]*[a-f0-9]+:[ 	]*f2 0f ae f1[ 	]*umwait %ecx
+[ 	]*[a-f0-9]+:[ 	]*f2 0f ae f3[ 	]*umwait %ebx
 [ 	]*[a-f0-9]+:[ 	]*66 0f ae f1[ 	]*tpause %ecx
+[ 	]*[a-f0-9]+:[ 	]*66 0f ae f3[ 	]*tpause %ebx
+[ 	]*[a-f0-9]+:[ 	]*f2 0f ae f7[ 	]*umwait %edi
+[ 	]*[a-f0-9]+:[ 	]*66 0f ae f7[ 	]*tpause %edi
 #pass
--- a/gas/testsuite/gas/i386/waitpkg.s
+++ b/gas/testsuite/gas/i386/waitpkg.s
@@ -5,4 +5,11 @@  _start:
 	umonitor %eax
 	umonitor %cx
 	umwait %ecx
+	umwait %ebx, %edx, %eax
 	tpause %ecx
+	tpause %ebx, %edx, %eax
+
+	.intel_syntax noprefix
+
+	umwait edi, edx, eax
+	tpause edi, edx, eax
--- a/gas/testsuite/gas/i386/x86-64-waitpkg-intel.d
+++ b/gas/testsuite/gas/i386/x86-64-waitpkg-intel.d
@@ -13,11 +13,11 @@  Disassembly of section \.text:
 [ 	]*[a-f0-9]+:[ 	]*f3 41 0f ae f2[ 	]*umonitor r10
 [ 	]*[a-f0-9]+:[ 	]*67 f3 41 0f ae f2[ 	]*umonitor r10d
 [ 	]*[a-f0-9]+:[ 	]*f2 0f ae f1[ 	]*umwait ecx
-[ 	]*[a-f0-9]+:[ 	]*f2 0f ae f1[ 	]*umwait ecx
-[ 	]*[a-f0-9]+:[ 	]*f2 41 0f ae f2[ 	]*umwait r10d
 [ 	]*[a-f0-9]+:[ 	]*f2 41 0f ae f2[ 	]*umwait r10d
+[ 	]*[a-f0-9]+:[ 	]*f2 0f ae f7[ 	]*umwait edi
 [ 	]*[a-f0-9]+:[ 	]*66 0f ae f1[ 	]*tpause ecx
-[ 	]*[a-f0-9]+:[ 	]*66 0f ae f1[ 	]*tpause ecx
-[ 	]*[a-f0-9]+:[ 	]*66 41 0f ae f2[ 	]*tpause r10d
 [ 	]*[a-f0-9]+:[ 	]*66 41 0f ae f2[ 	]*tpause r10d
+[ 	]*[a-f0-9]+:[ 	]*66 0f ae f7[ 	]*tpause edi
+[ 	]*[a-f0-9]+:[ 	]*f2 0f ae f6[ 	]*umwait esi
+[ 	]*[a-f0-9]+:[ 	]*66 0f ae f6[ 	]*tpause esi
 #pass
--- a/gas/testsuite/gas/i386/x86-64-waitpkg.d
+++ b/gas/testsuite/gas/i386/x86-64-waitpkg.d
@@ -13,11 +13,11 @@  Disassembly of section \.text:
 [ 	]*[a-f0-9]+:[ 	]*f3 41 0f ae f2[ 	]*umonitor %r10
 [ 	]*[a-f0-9]+:[ 	]*67 f3 41 0f ae f2[ 	]*umonitor %r10d
 [ 	]*[a-f0-9]+:[ 	]*f2 0f ae f1[ 	]*umwait %ecx
-[ 	]*[a-f0-9]+:[ 	]*f2 0f ae f1[ 	]*umwait %ecx
-[ 	]*[a-f0-9]+:[ 	]*f2 41 0f ae f2[ 	]*umwait %r10d
 [ 	]*[a-f0-9]+:[ 	]*f2 41 0f ae f2[ 	]*umwait %r10d
+[ 	]*[a-f0-9]+:[ 	]*f2 0f ae f7[ 	]*umwait %edi
 [ 	]*[a-f0-9]+:[ 	]*66 0f ae f1[ 	]*tpause %ecx
-[ 	]*[a-f0-9]+:[ 	]*66 0f ae f1[ 	]*tpause %ecx
-[ 	]*[a-f0-9]+:[ 	]*66 41 0f ae f2[ 	]*tpause %r10d
 [ 	]*[a-f0-9]+:[ 	]*66 41 0f ae f2[ 	]*tpause %r10d
+[ 	]*[a-f0-9]+:[ 	]*66 0f ae f7[ 	]*tpause %edi
+[ 	]*[a-f0-9]+:[ 	]*f2 0f ae f6[ 	]*umwait %esi
+[ 	]*[a-f0-9]+:[ 	]*66 0f ae f6[ 	]*tpause %esi
 #pass
--- a/gas/testsuite/gas/i386/x86-64-waitpkg.s
+++ b/gas/testsuite/gas/i386/x86-64-waitpkg.s
@@ -6,10 +6,13 @@  _start:
 	umonitor %r10
 	umonitor %r10d
 	umwait %ecx
-	umwait %rcx
-	umwait %r10
 	umwait %r10d
+	umwait %edi, %edx, %eax
 	tpause %ecx
-	tpause %rcx
-	tpause %r10
 	tpause %r10d
+	tpause %edi, %edx, %eax
+
+	.intel_syntax noprefix
+
+	umwait esi, edx, eax
+	tpause esi, edx, eax
--- a/opcodes/i386-opc.tbl
+++ b/opcodes/i386-opc.tbl
@@ -4763,10 +4763,10 @@  pconfig, 0, 0x0f01c5, None, 3, CpuPCONFI
 // WAITPKG instructions.
 
 umonitor, 1, 0xf30fae, 0x6, 2, CpuWAITPKG, Modrm|AddrPrefixOpReg, { Reg16|Reg32|Reg64 }
-
-tpause, 1, 0x660fae, 0x6, 2, CpuWAITPKG, Modrm|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|NoRex64, { Reg32|Reg64 }
-
-umwait, 1, 0xf20fae, 0x6, 2, CpuWAITPKG, Modrm|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf|NoRex64, { Reg32|Reg64 }
+tpause, 1, 0x660fae, 0x6, 2, CpuWAITPKG, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Reg32 }
+tpause, 3, 0x660fae, 0x6, 2, CpuWAITPKG, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Reg32, RegD|Dword, Acc|Dword }
+umwait, 1, 0xf20fae, 0x6, 2, CpuWAITPKG, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Reg32 }
+umwait, 3, 0xf20fae, 0x6, 2, CpuWAITPKG, Modrm|IgnoreSize|No_bSuf|No_wSuf|No_lSuf|No_sSuf|No_qSuf|No_ldSuf, { Reg32, RegD|Dword, Acc|Dword }
 
 // WAITPKG instructions end.