[X86][MC] Support Enc/Dec for EGPR for promoted MOVDIR instruction #74713
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-x86 @llvm/pr-subscribers-mc Author: None (XinWang10) ChangesR16-R31 was added into GPRs in #70958, RFC: https://discourse.llvm.org/t/rfc-design-for-apx-feature-egpr-and-ndd-support/73031/4 Full diff: https://github.com/llvm/llvm-project/pull/74713.diff 11 Files Affected:
diff --git a/llvm/include/llvm/Support/X86DisassemblerDecoderCommon.h b/llvm/include/llvm/Support/X86DisassemblerDecoderCommon.h
index b3d8580e5e56f..58f93199040d6 100644
--- a/llvm/include/llvm/Support/X86DisassemblerDecoderCommon.h
+++ b/llvm/include/llvm/Support/X86DisassemblerDecoderCommon.h
@@ -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 and the OPSIZE prefix 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") \
diff --git a/llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp b/llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp
index d50e6514b86d8..b97b1402bd005 100644
--- a/llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp
+++ b/llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp
@@ -1175,6 +1175,8 @@ static int getInstructionID(struct InternalInstruction *insn,
attrMask |= ATTR_VEXL;
if (l2FromEVEX4of4(insn->vectorExtensionPrefix[3]))
attrMask |= ATTR_EVEXL2;
+ if (insn->hasAdSize)
+ attrMask |= ATTR_ADSIZE;
} else if (insn->vectorExtensionType == TYPE_VEX_3B) {
switch (ppFromVEX3of3(insn->vectorExtensionPrefix[2])) {
case VEX_PREFIX_66:
diff --git a/llvm/lib/Target/X86/X86InstrMisc.td b/llvm/lib/Target/X86/X86InstrMisc.td
index 82c079fe2ea82..7b78aa26d1689 100644
--- a/llvm/lib/Target/X86/X86InstrMisc.td
+++ b/llvm/lib/Target/X86/X86InstrMisc.td
@@ -1497,11 +1497,21 @@ 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]>;
+let CD8_Scale = 0 in {
+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, T_MAP4PS, Requires<[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, T_MAP4PS, Requires<[In64BitMode, HasMOVDIRI, HasEGPR]>;
+}
} // SchedRW
//===----------------------------------------------------------------------===//
@@ -1514,11 +1524,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
//===----------------------------------------------------------------------===//
diff --git a/llvm/test/MC/Disassembler/X86/apx/movdir64b.txt b/llvm/test/MC/Disassembler/X86/apx/movdir64b.txt
new file mode 100644
index 0000000000000..81d8f49dbf69d
--- /dev/null
+++ b/llvm/test/MC/Disassembler/X86/apx/movdir64b.txt
@@ -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
+
+# 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
diff --git a/llvm/test/MC/Disassembler/X86/apx/movdiri.txt b/llvm/test/MC/Disassembler/X86/apx/movdiri.txt
new file mode 100644
index 0000000000000..997d016f0d222
--- /dev/null
+++ b/llvm/test/MC/Disassembler/X86/apx/movdiri.txt
@@ -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
diff --git a/llvm/test/MC/X86/apx/movdir64b-att.s b/llvm/test/MC/X86/apx/movdir64b-att.s
new file mode 100644
index 0000000000000..bc8f1a90c9ed6
--- /dev/null
+++ b/llvm/test/MC/X86/apx/movdir64b-att.s
@@ -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
diff --git a/llvm/test/MC/X86/apx/movdir64b-intel.s b/llvm/test/MC/X86/apx/movdir64b-intel.s
new file mode 100644
index 0000000000000..b34efefeba2da
--- /dev/null
+++ b/llvm/test/MC/X86/apx/movdir64b-intel.s
@@ -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]
diff --git a/llvm/test/MC/X86/apx/movdiri-att.s b/llvm/test/MC/X86/apx/movdiri-att.s
new file mode 100644
index 0000000000000..8bdabf232f5de
--- /dev/null
+++ b/llvm/test/MC/X86/apx/movdiri-att.s
@@ -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)
diff --git a/llvm/test/MC/X86/apx/movdiri-intel.s b/llvm/test/MC/X86/apx/movdiri-intel.s
new file mode 100644
index 0000000000000..1a38384f9f96e
--- /dev/null
+++ b/llvm/test/MC/X86/apx/movdiri-intel.s
@@ -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
diff --git a/llvm/utils/TableGen/X86DisassemblerTables.cpp b/llvm/utils/TableGen/X86DisassemblerTables.cpp
index f879a9f5e4094..23c5362e66cdf 100644
--- a/llvm/utils/TableGen/X86DisassemblerTables.cpp
+++ b/llvm/utils/TableGen/X86DisassemblerTables.cpp
@@ -104,6 +104,7 @@ static inline bool inheritsFrom(InstructionContext child,
case IC_64BIT_ADSIZE:
return (noPrefix && inheritsFrom(child, IC_64BIT_OPSIZE_ADSIZE, noPrefix));
case IC_64BIT_OPSIZE_ADSIZE:
+ case IC_EVEX_OPSIZE_ADSIZE:
return false;
case IC_XD:
return inheritsFrom(child, IC_64BIT_XD);
@@ -878,7 +879,10 @@ 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_64BIT) &&
+ (index & ATTR_OPSIZE) && (index & ATTR_ADSIZE))
+ o << "IC_EVEX_OPSIZE_ADSIZE";
+ else if ((index & ATTR_EVEX) || (index & ATTR_VEX) || (index & ATTR_VEXL)) {
if (index & ATTR_EVEX)
o << "IC_EVEX";
else
@@ -898,8 +902,8 @@ void DisassemblerTables::emitContextTable(raw_ostream &o, unsigned &i) const {
o << "_XD";
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)
diff --git a/llvm/utils/TableGen/X86RecognizableInstr.cpp b/llvm/utils/TableGen/X86RecognizableInstr.cpp
index 6e03fc11d6d9d..06f2c63f4cd58 100644
--- a/llvm/utils/TableGen/X86RecognizableInstr.cpp
+++ b/llvm/utils/TableGen/X86RecognizableInstr.cpp
@@ -265,8 +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::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)
|
|
KanRobert
added a commit
that referenced
this pull request
Dec 7, 2023
KanRobert
reviewed
Dec 7, 2023
KanRobert
reviewed
Dec 7, 2023
KanRobert
reviewed
Dec 7, 2023
KanRobert
reviewed
Dec 7, 2023
KanRobert
reviewed
Dec 8, 2023
@XinWang10 Be careful. It seems the brace is not balanced. |
phoebewang
requested changes
Dec 9, 2023
|
|
||
| # 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ha! we have specail handling here https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/Disassembler/X86Disassembler.cpp#L1326
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
i512mem_GR32 could also for encoding, I added to make all the reg used by this instruction have same size.
There was a problem hiding this comment.
@KanRobert Test case will do. Thanks for info.
There was a problem hiding this comment.
@phoebewang This version could be more correct :)
XinWang10
force-pushed
the
APXMovdirEnc
branch
from
7ed866a
to
af9a423
Compare
KanRobert
reviewed
Dec 14, 2023
| @@ -885,7 +887,10 @@ 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_64BIT) && (index & ATTR_OPSIZE) && | |||
There was a problem hiding this comment.
Could we remove the (index & ATTR_64BIT)? From the name IC_EVEX_OPSIZE_ADSIZE, it should not check this.
There was a problem hiding this comment.
ATTR_64BIT is not needed
KanRobert
reviewed
Dec 14, 2023
| @@ -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->vectorExtensionType == TYPE_EVEX && insn->opcode == 0xF8)) { | |||
There was a problem hiding this comment.
Why do we need line 1334? Isn't extended MOVDIR64B's opcodeType THREEBYTE_38?
There was a problem hiding this comment.
Never mind. I forgot it's in MAP4.
There was a problem hiding this comment.
Should we add the limitation to MAP4? I think we should have other insn->opcode == 0xF8 for EVEX encoding.
There was a problem hiding this comment.
+1, and then remove insn->vectorExtensionType == TYPE_EVEX since MAP4 can only be encoded by EVEX.
There was a problem hiding this comment.
Yes, we do have other F8, will do.
phoebewang
reviewed
Dec 15, 2023
Comment on lines
+890
to
+891
| if ((index & ATTR_EVEX) && (index & ATTR_OPSIZE) && (index & ATTR_ADSIZE)) | ||
| o << "IC_EVEX_OPSIZE_ADSIZE"; |
There was a problem hiding this comment.
Do we still need this since we have special handing?
There was a problem hiding this comment.
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.
|
Thanks for your review~~~ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
R16-R31 was added into GPRs in #70958,
This patch supports the encoding/decoding for promoted MOVDIR instruction in EVEX space.
RFC: https://discourse.llvm.org/t/rfc-design-for-apx-feature-egpr-and-ndd-support/73031/4