Skip to content

[X86][MC] Support Enc/Dec for EGPR for promoted MOVDIR instruction #74713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Dec 15, 2023
Merged

[X86][MC] Support Enc/Dec for EGPR for promoted MOVDIR instruction #74713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
335b337
base impl
XinWang10 Dec 6, 2023
99d5246
[X86][MC] Support Enc/Dec for EGPR for promoted MOVDIR instruction
XinWang10 Dec 7, 2023
638773b
Use EVEX_NoCD8
XinWang10 Dec 7, 2023
dd2b302
Fix
XinWang10 Dec 7, 2023
855af26
remove redundant tab
XinWang10 Dec 7, 2023
3c27968
update comment
XinWang10 Dec 8, 2023
6304ca0
resolve comments
XinWang10 Dec 8, 2023
db9c6c0
clang format
XinWang10 Dec 8, 2023
af9a423
Mov ADSIZE special for evex movdir64b
XinWang10 Dec 13, 2023
cac38eb
resolve comments
XinWang10 Dec 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions llvm/include/llvm/Support/X86DisassemblerDecoderCommon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ enum attributeBits {
ENUM_ENTRY(IC_EVEX_XS, 2, "requires EVEX and the XS prefix") \
ENUM_ENTRY(IC_EVEX_XD, 2, "requires EVEX and the XD prefix") \
ENUM_ENTRY(IC_EVEX_OPSIZE, 2, "requires EVEX and the OpSize prefix") \
ENUM_ENTRY(IC_EVEX_OPSIZE_ADSIZE, 3, \
"requires EVEX, OPSIZE and the ADSIZE prefix") \
ENUM_ENTRY(IC_EVEX_W, 3, "requires EVEX and the W prefix") \
ENUM_ENTRY(IC_EVEX_W_XS, 4, "requires EVEX, W, and XS prefix") \
ENUM_ENTRY(IC_EVEX_W_XD, 4, "requires EVEX, W, and XD prefix") \
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,7 +1330,8 @@ static int getInstructionID(struct InternalInstruction *insn,
// any position.
if ((insn->opcodeType == ONEBYTE && ((insn->opcode & 0xFC) == 0xA0)) ||
(insn->opcodeType == TWOBYTE && (insn->opcode == 0xAE)) ||
(insn->opcodeType == THREEBYTE_38 && insn->opcode == 0xF8)) {
(insn->opcodeType == THREEBYTE_38 && insn->opcode == 0xF8) ||
(insn->opcodeType == MAP4 && insn->opcode == 0xF8)) {
// Make sure we observed the prefixes in any position.
if (insn->hasAdSize)
attrMask |= ATTR_ADSIZE;
Expand Down
24 changes: 20 additions & 4 deletions llvm/lib/Target/X86/X86InstrMisc.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1497,11 +1497,19 @@ let SchedRW = [WriteStore] in {
def MOVDIRI32 : I<0xF9, MRMDestMem, (outs), (ins i32mem:$dst, GR32:$src),
"movdiri\t{$src, $dst|$dst, $src}",
[(int_x86_directstore32 addr:$dst, GR32:$src)]>,
T8PS, Requires<[HasMOVDIRI]>;
T8PS, Requires<[HasMOVDIRI, NoEGPR]>;
def MOVDIRI64 : RI<0xF9, MRMDestMem, (outs), (ins i64mem:$dst, GR64:$src),
"movdiri\t{$src, $dst|$dst, $src}",
[(int_x86_directstore64 addr:$dst, GR64:$src)]>,
T8PS, Requires<[In64BitMode, HasMOVDIRI]>;
T8PS, Requires<[In64BitMode, HasMOVDIRI, NoEGPR]>;
def MOVDIRI32_EVEX : I<0xF9, MRMDestMem, (outs), (ins i32mem:$dst, GR32:$src),
"movdiri\t{$src, $dst|$dst, $src}",
[(int_x86_directstore32 addr:$dst, GR32:$src)]>,
EVEX_NoCD8, T_MAP4PS, Requires<[In64BitMode, HasMOVDIRI, HasEGPR]>;
def MOVDIRI64_EVEX : RI<0xF9, MRMDestMem, (outs), (ins i64mem:$dst, GR64:$src),
"movdiri\t{$src, $dst|$dst, $src}",
[(int_x86_directstore64 addr:$dst, GR64:$src)]>,
EVEX_NoCD8, T_MAP4PS, Requires<[In64BitMode, HasMOVDIRI, HasEGPR]>;
} // SchedRW

//===----------------------------------------------------------------------===//
Expand All @@ -1514,11 +1522,19 @@ def MOVDIR64B16 : I<0xF8, MRMSrcMem, (outs), (ins GR16:$dst, i512mem_GR16:$src),
def MOVDIR64B32 : I<0xF8, MRMSrcMem, (outs), (ins GR32:$dst, i512mem_GR32:$src),
"movdir64b\t{$src, $dst|$dst, $src}",
[(int_x86_movdir64b GR32:$dst, addr:$src)]>,
T8PD, AdSize32, Requires<[HasMOVDIR64B]>;
T8PD, AdSize32, Requires<[HasMOVDIR64B, NoEGPR]>;
def MOVDIR64B64 : I<0xF8, MRMSrcMem, (outs), (ins GR64:$dst, i512mem_GR64:$src),
"movdir64b\t{$src, $dst|$dst, $src}",
[(int_x86_movdir64b GR64:$dst, addr:$src)]>,
T8PD, AdSize64, Requires<[HasMOVDIR64B, In64BitMode]>;
T8PD, AdSize64, Requires<[HasMOVDIR64B, NoEGPR, In64BitMode]>;
def MOVDIR64B32_EVEX : I<0xF8, MRMSrcMem, (outs), (ins GR32:$dst, i512mem_GR32:$src),
"movdir64b\t{$src, $dst|$dst, $src}",
[(int_x86_movdir64b GR32:$dst, addr:$src)]>,
EVEX_NoCD8, T_MAP4PD, AdSize32, Requires<[HasMOVDIR64B, HasEGPR, In64BitMode]>;
def MOVDIR64B64_EVEX : I<0xF8, MRMSrcMem, (outs), (ins GR64:$dst, i512mem_GR64:$src),
"movdir64b\t{$src, $dst|$dst, $src}",
[(int_x86_movdir64b GR64:$dst, addr:$src)]>,
EVEX_NoCD8, T_MAP4PD, AdSize64, Requires<[HasMOVDIR64B, HasEGPR, In64BitMode]>;
} // SchedRW

//===----------------------------------------------------------------------===//
Expand Down
10 changes: 10 additions & 0 deletions llvm/test/MC/Disassembler/X86/apx/movdir64b.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# RUN: llvm-mc --disassemble %s -triple=x86_64 | FileCheck %s --check-prefixes=ATT
# RUN: llvm-mc --disassemble %s -triple=x86_64 -x86-asm-syntax=intel --output-asm-variant=1 | FileCheck %s --check-prefixes=INTEL

# ATT: movdir64b 291(%r28d,%r29d,4), %r18d
# INTEL: movdir64b r18d, zmmword ptr [r28d + 4*r29d + 291]
0x67,0x62,0x8c,0x79,0x08,0xf8,0x94,0xac,0x23,0x01,0x00,0x00
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why ATTR_ADSIZE only matters to this instruction.
I think either all APX instructions need special handling for address size override prefix or nothing to do here.

I also compared with existing instruction that has ATTR_EVEX and ATTR_OPSIZE, e.g.,

EVEX.128.66.0F.W0 78 /r VCVTTPS2UQQ xmm1 {k1}{z}, xmm2/m64/m32bcst

We can decode correctly w/ and w/o address size override prefix without define a IC_EVEX_OPSIZE_ADSIZE, e.g.,

$ echo '0x62,0xf1,0x7d,0x0a,0x78,0x49,0x10' | llvm-mc --show-encoding --disassemble
        .text
        vcvttps2uqq     128(%rcx), %xmm1 {%k2}  # encoding: [0x62,0xf1,0x7d,0x0a,0x78,0x49,0x10]
$ echo '0x67,0x62,0xf1,0x7d,0x0a,0x78,0x49,0x10' | llvm-mc --show-encoding --disassemble
        .text
        vcvttps2uqq     128(%ecx), %xmm1 {%k2}  # encoding: [0x67,0x62,0xf1,0x7d,0x0a,0x78,0x49,0x10]

Does it still work with this patch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see the reason why you considering it. We used AdSize32 to distinguish the source register size of movdir64b, which is a special use of address size override prefix.
But I think the introduction of i512mem_GR32 was just used to solve the problem. Does it not work in this case? I think we should follow this way and see what's missing for the new case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! we have specail handling here https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp#L1326

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I introduce a new IC context named IC_EVEX_OPSIZE_ADSIZE for promoted movdir64b and just want disassembler to recognize the ADSIZE for movdir64b. But though IC_EVEX_OPSIZE_ADSIZE is just for movdir64b but the logic for ADSIZE in EVEX for disassembler could affect other instructions using ADSIZE. I may just need to expand the special handling for promoted movdir64b.
@phoebewang Thank you for pointing, will update.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IC_EVEX_OPSIZE_ADSIZE is still needed as we added IC_ADSIZE, IC_64BIT_REXW_ADSIZE. Dropping the change in X86Disassembler.cpp may solve the problem since we already specially handle it.

@XinWang10 Please add test mentioned by Phoebe in a separate commit, like https://reviews.llvm.org/D122449

@phoebewang i512mem_GR32 is added for decoding issue only? This surprised me...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i512mem_GR32 could also for encoding, I added to make all the reg used by this instruction have same size.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KanRobert Test case will do. Thanks for info.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phoebewang This version could be more correct :)


# ATT: movdir64b 291(%r28,%r29,4), %r19
# INTEL: movdir64b r19, zmmword ptr [r28 + 4*r29 + 291]
0x62,0x8c,0x79,0x08,0xf8,0x9c,0xac,0x23,0x01,0x00,0x00
10 changes: 10 additions & 0 deletions llvm/test/MC/Disassembler/X86/apx/movdiri.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# RUN: llvm-mc --disassemble %s -triple=x86_64 | FileCheck %s --check-prefixes=ATT
# RUN: llvm-mc --disassemble %s -triple=x86_64 -x86-asm-syntax=intel --output-asm-variant=1 | FileCheck %s --check-prefixes=INTEL

# ATT: movdiri %r18d, 291(%r28,%r29,4)
# INTEL: movdiri dword ptr [r28 + 4*r29 + 291], r18d
0x62,0x8c,0x78,0x08,0xf9,0x94,0xac,0x23,0x01,0x00,0x00

# ATT: movdiri %r19, 291(%r28,%r29,4)
# INTEL: movdiri qword ptr [r28 + 4*r29 + 291], r19
0x62,0x8c,0xf8,0x08,0xf9,0x9c,0xac,0x23,0x01,0x00,0x00
12 changes: 12 additions & 0 deletions llvm/test/MC/X86/apx/movdir64b-att.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# RUN: llvm-mc -triple x86_64 --show-encoding %s | FileCheck %s
# RUN: not llvm-mc -triple i386 -show-encoding %s 2>&1 | FileCheck %s --check-prefix=ERROR

# ERROR-COUNT-2: error:
# ERROR-NOT: error:
# CHECK: movdir64b 291(%r28d,%r29d,4), %r18d
# CHECK: encoding: [0x67,0x62,0x8c,0x79,0x08,0xf8,0x94,0xac,0x23,0x01,0x00,0x00]
movdir64b 291(%r28d,%r29d,4), %r18d

# CHECK: movdir64b 291(%r28,%r29,4), %r19
# CHECK: encoding: [0x62,0x8c,0x79,0x08,0xf8,0x9c,0xac,0x23,0x01,0x00,0x00]
movdir64b 291(%r28,%r29,4), %r19
9 changes: 9 additions & 0 deletions llvm/test/MC/X86/apx/movdir64b-intel.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# RUN: llvm-mc -triple x86_64 -x86-asm-syntax=intel -output-asm-variant=1 --show-encoding %s | FileCheck %s

# CHECK: movdir64b r18d, zmmword ptr [r28d + 4*r29d + 291]
# CHECK: encoding: [0x67,0x62,0x8c,0x79,0x08,0xf8,0x94,0xac,0x23,0x01,0x00,0x00]
movdir64b r18d, zmmword ptr [r28d + 4*r29d + 291]

# CHECK: movdir64b r19, zmmword ptr [r28 + 4*r29 + 291]
# CHECK: encoding: [0x62,0x8c,0x79,0x08,0xf8,0x9c,0xac,0x23,0x01,0x00,0x00]
movdir64b r19, zmmword ptr [r28 + 4*r29 + 291]
12 changes: 12 additions & 0 deletions llvm/test/MC/X86/apx/movdiri-att.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
# RUN: llvm-mc -triple x86_64 --show-encoding %s | FileCheck %s
# RUN: not llvm-mc -triple i386 -show-encoding %s 2>&1 | FileCheck %s --check-prefix=ERROR

# ERROR-COUNT-2: error:
# ERROR-NOT: error:
# CHECK: movdiri %r18d, 291(%r28,%r29,4)
# CHECK: encoding: [0x62,0x8c,0x78,0x08,0xf9,0x94,0xac,0x23,0x01,0x00,0x00]
movdiri %r18d, 291(%r28,%r29,4)

# CHECK: movdiri %r19, 291(%r28,%r29,4)
# CHECK: encoding: [0x62,0x8c,0xf8,0x08,0xf9,0x9c,0xac,0x23,0x01,0x00,0x00]
movdiri %r19, 291(%r28,%r29,4)
9 changes: 9 additions & 0 deletions llvm/test/MC/X86/apx/movdiri-intel.s
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# RUN: llvm-mc -triple x86_64 -x86-asm-syntax=intel -output-asm-variant=1 --show-encoding %s | FileCheck %s

# CHECK: movdiri dword ptr [r28 + 4*r29 + 291], r18d
# CHECK: encoding: [0x62,0x8c,0x78,0x08,0xf9,0x94,0xac,0x23,0x01,0x00,0x00]
movdiri dword ptr [r28 + 4*r29 + 291], r18d

# CHECK: movdiri qword ptr [r28 + 4*r29 + 291], r19
# CHECK: encoding: [0x62,0x8c,0xf8,0x08,0xf9,0x9c,0xac,0x23,0x01,0x00,0x00]
movdiri qword ptr [r28 + 4*r29 + 291], r19
8 changes: 6 additions & 2 deletions llvm/utils/TableGen/X86DisassemblerTables.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ static inline bool inheritsFrom(InstructionContext child,
(WIG && inheritsFrom(child, IC_EVEX_W_OPSIZE)) ||
(VEX_LIG && inheritsFrom(child, IC_EVEX_L_OPSIZE)) ||
(VEX_LIG && inheritsFrom(child, IC_EVEX_L2_OPSIZE));
case IC_EVEX_OPSIZE_ADSIZE:
return false;
case IC_EVEX_K:
return (VEX_LIG && WIG && inheritsFrom(child, IC_EVEX_L_W_K)) ||
(VEX_LIG && WIG && inheritsFrom(child, IC_EVEX_L2_W_K)) ||
Expand Down Expand Up @@ -885,7 +887,9 @@ void DisassemblerTables::emitContextTable(raw_ostream &o, unsigned &i) const {
for (unsigned index = 0; index < ATTR_max; ++index) {
o.indent(i * 2);

if ((index & ATTR_EVEX) || (index & ATTR_VEX) || (index & ATTR_VEXL)) {
if ((index & ATTR_EVEX) && (index & ATTR_OPSIZE) && (index & ATTR_ADSIZE))
o << "IC_EVEX_OPSIZE_ADSIZE";
Comment on lines +890 to +891
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this since we have special handing?

Copy link
Contributor Author

@XinWang10 XinWang10 Dec 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we need new context for MOVDIR64B64_EVEX, otherwise it would have conflict with MOVDIR64B32_EVEX, their context would all be IC_EVEX_OPSIZE. And special handling is to make disassembler could add attribute ADSIZE to movdir64b so that it could match to IC_EVEX_OPSIZE_ADSIZE context.

else if ((index & ATTR_EVEX) || (index & ATTR_VEX) || (index & ATTR_VEXL)) {
if (index & ATTR_EVEX)
o << "IC_EVEX";
else
Expand All @@ -906,7 +910,7 @@ void DisassemblerTables::emitContextTable(raw_ostream &o, unsigned &i) const {
else if (index & ATTR_XS)
o << "_XS";

if ((index & ATTR_EVEX)) {
if (index & ATTR_EVEX) {
if (index & ATTR_EVEXKZ)
o << "_KZ";
else if (index & ATTR_EVEXK)
Expand Down
9 changes: 6 additions & 3 deletions llvm/utils/TableGen/X86RecognizableInstr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,9 +265,12 @@ InstructionContext RecognizableInstr::insnContext() const {
}
}
// No L, no W
else if (OpPrefix == X86Local::PD)
insnContext = EVEX_KB(IC_EVEX_OPSIZE);
else if (OpPrefix == X86Local::XD)
else if (OpPrefix == X86Local::PD) {
if (AdSize == X86Local::AdSize32)
insnContext = IC_EVEX_OPSIZE_ADSIZE;
else
insnContext = EVEX_KB(IC_EVEX_OPSIZE);
} else if (OpPrefix == X86Local::XD)
insnContext = EVEX_KB(IC_EVEX_XD);
else if (OpPrefix == X86Local::XS)
insnContext = EVEX_KB(IC_EVEX_XS);
Expand Down