[X86] Prefer VEX encoding in X86 assembler.

[LuoYuanke]

Summary

This patch is to order the AVX instructions ahead of AVX512 instructions
in the matching table so that the AVX instructions can be matched first.
Thanks Craig for the idea.

[craig.topper]

Does this pass tests on all targets? I didn't check myself.

[LuoYuanke]

In D111538#3054803, @craig.topper wrote:

Does this pass tests on all targets? I didn't check myself.

It passes X86 target. I'll test it on all targets.

[LuoYuanke]

There are 16 test case fails.

Failed Tests (16):
  LLVM :: MC/AMDGPU/expressions.s
  LLVM :: MC/AMDGPU/gfx1011_dlops.s
  LLVM :: MC/AMDGPU/gfx10_asm_smem.s
  LLVM :: MC/AMDGPU/gfx7_asm_mubuf.s
  LLVM :: MC/AMDGPU/gfx8_asm_mubuf.s
  LLVM :: MC/AMDGPU/gfx90a_asm_features.s
  LLVM :: MC/AMDGPU/gfx90a_ldst_acc.s
  LLVM :: MC/AMDGPU/gfx9_asm_mubuf.s
  LLVM :: MC/AMDGPU/gfx9_asm_smem.s
  LLVM :: MC/AMDGPU/literalv216.s
  LLVM :: MC/AMDGPU/mubuf.s
  LLVM :: MC/AMDGPU/smem.s
  LLVM :: MC/AMDGPU/trap.s
  LLVM :: MC/AMDGPU/vop3-literal.s
  LLVM :: MC/Mips/micromips32r6/valid.s
  LLVM :: MC/WebAssembly/tables.s

[LuoYuanke]

llvm/utils/TableGen/AsmMatcherEmitter.cpp

648

Is there any approach to check it is X86 target?

[skan]

llvm/utils/TableGen/AsmMatcherEmitter.cpp

648

Sure. You can check TheDef->isSubClassOf("X86Inst")

[LuoYuanke]

llvm/utils/TableGen/AsmMatcherEmitter.cpp

648

Great! I'll update the patch.

[LuoYuanke]

Check "X86Inst" to order the match table for X86 specificly.

[LuoYuanke]

The test case passes on all target.

[LuoYuanke]

I personally prefer this patch, because it is more general and is easier to implement.

[skan]

llvm/utils/TableGen/AsmMatcherEmitter.cpp

639–640

Suggestion: This is X86-specific code in target-independent file. Please add some comments about why you specialize the code for X86 here and how the strict total order of ID is defined.

[LuoYuanke]

Address Shengchen's comments.

[lebedev.ri]

Having something specific to some specific target in a generic code
(esp. if there is no precedent for it) seems conceptually wrong.

[LuoYuanke]

In D111538#3061027, @lebedev.ri wrote:

Having something specific to some specific target in a generic code
(esp. if there is no precedent for it) seems conceptually wrong.

We can see in line 636, it also do some specific things for ARM. We can also see ReportMultipleNearMisses is added for ARM.

[lebedev.ri]

In D111538#3061264, @LuoYuanke wrote:

In D111538#3061027, @lebedev.ri wrote:

Having something specific to some specific target in a generic code
(esp. if there is no precedent for it) seems conceptually wrong.

We can see in line 636, it also do some specific things for ARM. We can also see ReportMultipleNearMisses is added for ARM.

I think you understand that i'm not talking about doing specific for target X - adding new hooks is fine,
but specifically about checking for the subclass name and doing different stuff based on that.

[LuoYuanke]

In D111538#3061274, @lebedev.ri wrote:

In D111538#3061264, @LuoYuanke wrote:

In D111538#3061027, @lebedev.ri wrote:

Having something specific to some specific target in a generic code
(esp. if there is no precedent for it) seems conceptually wrong.

We can see in line 636, it also do some specific things for ARM. We can also see ReportMultipleNearMisses is added for ARM.

I think you understand that i'm not talking about doing specific for target X - adding new hooks is fine,
but specifically about checking for the subclass name and doing different stuff based on that.

Do you have any suggestions? I don't know if there is any mechanism that can invoke target specific function in generic code in tablegen.

[lebedev.ri]

In D111538#3061306, @LuoYuanke wrote:

In D111538#3061274, @lebedev.ri wrote:

In D111538#3061264, @LuoYuanke wrote:

In D111538#3061027, @lebedev.ri wrote:

Having something specific to some specific target in a generic code
(esp. if there is no precedent for it) seems conceptually wrong.

We can see in line 636, it also do some specific things for ARM. We can also see ReportMultipleNearMisses is added for ARM.

I think you understand that i'm not talking about doing specific for target X - adding new hooks is fine,
but specifically about checking for the subclass name and doing different stuff based on that.

Do you have any suggestions? I don't know if there is any mechanism that can invoke target specific function in generic code in tablegen.

I guess i would suggest dealing with that in the backend itself.

[pengfei]

llvm/utils/TableGen/AsmMatcherEmitter.cpp

624

I have a question here. AVX512 instructions have larger register class, e.g. VR128X vs. VR128. Why doesn't (Could) the code work for them?

[pengfei]

llvm/utils/TableGen/AsmMatcherEmitter.cpp

624

Oh, it seems they are all the same since only GPRs and memory.

[pengfei]

How about we add a EncodingCost to the base class Instruction and set a high cost for EVEX? Or we can reuse the Size in it, since all X86 instructions set it to 0. We can use it as the encoding cost.

[skan]

In D111538#3061307, @lebedev.ri wrote:

In D111538#3061306, @LuoYuanke wrote:

In D111538#3061274, @lebedev.ri wrote:

In D111538#3061264, @LuoYuanke wrote:

In D111538#3061027, @lebedev.ri wrote:

Having something specific to some specific target in a generic code
(esp. if there is no precedent for it) seems conceptually wrong.

We can see in line 636, it also do some specific things for ARM. We can also see ReportMultipleNearMisses is added for ARM.

I think you understand that i'm not talking about doing specific for target X - adding new hooks is fine,
but specifically about checking for the subclass name and doing different stuff based on that.

Do you have any suggestions? I don't know if there is any mechanism that can invoke target specific function in generic code in tablegen.

I guess i would suggest dealing with that in the backend itself.

@lebedev.ri How about adding a hook such as hasPositionOrder, which means the order in which the instructions appear in TD file affects the order of assembler matching ?

[LuoYuanke]

Address Pengfei and Shengchen's comments. Their ideas are similar.

[pengfei]

llvm/include/llvm/Target/Target.td

656–659

Redundant?

[LuoYuanke]

Address pengfei's comments.

[LuoYuanke]

llvm/include/llvm/Target/Target.td

656–659

Redundant?

Opps! I forget to delete it.

[skan]

LGTM

llvm/include/llvm/Target/Target.td

class Instruction : InstructionEncoding {
   /// uses complex address mode).
   bit UseLogicalOperandMappings = false;
 
   /// Should FastISel ignore this instruction. For certain ISAs, they have
   /// instructions which map to the same ISD Opcode, value type operands and
   /// instruction selection predicates. FastISel cannot handle such cases, but
   /// SelectionDAG can.
   bit FastISelShouldIgnore = false;
+
+  /// HasPositionOrder: Indicate tablegen to sort the instructions by record
+  /// ID, so that instruction that is defined earlier can be sorted earlier
+  /// in the assembly matching table.
+  bit HasPositionOrder = false;
 }
 
 /// Defines an additional encoding that disassembles to the given instruction
 /// Like Instruction, the Inst and SoftFail fields are omitted to allow targets
 // to specify their size.
 class AdditionalEncoding<Instruction I> : InstructionEncoding {
   Instruction AliasOf = I;
 }

llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp

unsigned X86AsmParser::checkTargetMatchPredicate(MCInst &Inst) {
 
   // These instructions are only available with {vex}, {vex2} or {vex3} prefix
   if (MCID.TSFlags & X86II::ExplicitVEXPrefix &&
       (ForcedVEXEncoding != VEXEncoding_VEX &&
        ForcedVEXEncoding != VEXEncoding_VEX2 &&
        ForcedVEXEncoding != VEXEncoding_VEX3))
     return Match_Unsupported;
 
-  // These instructions match ambiguously with their VEX encoded counterparts
-  // and appear first in the matching table. Reject them unless we're forcing
-  // EVEX encoding.
-  // FIXME: We really need a way to break the ambiguity.
-  switch (Opc) {
-  case X86::VCVTSD2SIZrm_Int:
-  case X86::VCVTSD2SI64Zrm_Int:
-  case X86::VCVTSS2SIZrm_Int:
-  case X86::VCVTSS2SI64Zrm_Int:
-  case X86::VCVTTSD2SIZrm:   case X86::VCVTTSD2SIZrm_Int:
-  case X86::VCVTTSD2SI64Zrm: case X86::VCVTTSD2SI64Zrm_Int:
-  case X86::VCVTTSS2SIZrm:   case X86::VCVTTSS2SIZrm_Int:
-  case X86::VCVTTSS2SI64Zrm: case X86::VCVTTSS2SI64Zrm_Int:
-    if (ForcedVEXEncoding != VEXEncoding_EVEX)
-      return Match_Unsupported;
-    break;
-  }
-
   return Match_Success;
 }
 
 bool X86AsmParser::MatchAndEmitATTInstruction(SMLoc IDLoc, unsigned &Opcode,
                                               OperandVector &Operands,
                                               MCStreamer &Out,
                                               uint64_t &ErrorInfo,
                                               bool MatchingInlineAsm) {

llvm/lib/Target/X86/X86InstrFormats.td

class X86Inst<bits<8> opcod, Format f, ImmType i, dag outs, dag ins,
 
   dag OutOperandList = outs;
   dag InOperandList = ins;
   string AsmString = AsmStr;
 
   // If this is a pseudo instruction, mark it isCodeGenOnly.
   let isCodeGenOnly = !eq(!cast<string>(f), "Pseudo");
 
+  let HasPositionOrder = 1;
+
   //
   // Attributes specific to X86 instructions...
   //
   bit ForceDisassemble = 0; // Force instruction to disassemble even though it's
                             // isCodeGenonly. Needed to hide an ambiguous
                             // AsmString from the parser, but still disassemble.
 
   OperandSize OpSize = OpSizeFixed; // Does this instruction's encoding change

llvm/utils/TableGen/AsmMatcherEmitter.cpp

if (int Cmp = Mnemonic.compare_insensitive(RHS.Mnemonic))
       return Cmp == -1;
 
     if (AsmOperands.size() != RHS.AsmOperands.size())
       return AsmOperands.size() < RHS.AsmOperands.size();
 
     // Compare lexicographically by operand. The matcher validates that other
     // orderings wouldn't be ambiguous using \see couldMatchAmbiguouslyWith().
     for (unsigned i = 0, e = AsmOperands.size(); i != e; ++i) {
       if (*AsmOperands[i].Class < *RHS.AsmOperands[i].Class)
         return true;
       if (*RHS.AsmOperands[i].Class < *AsmOperands[i].Class)
         return false;
     }
 
     // Give matches that require more features higher precedence. This is useful
     // because we cannot define AssemblerPredicates with the negation of
     // processor features. For example, ARM v6 "nop" may be either a HINT or
     // MOV. With v6, we want to match HINT. The assembler has no way to
     // predicate MOV under "NoV6", but HINT will always match first because it
     // requires V6 while MOV does not.
     if (RequiredFeatures.size() != RHS.RequiredFeatures.size())
       return RequiredFeatures.size() > RHS.RequiredFeatures.size();
 
+    // For X86 AVX/AVX512 instructions, we prefer vex encoding because the
+    // vex encoding size is smaller. Since X86InstrSSE.td is included ahead
+    // of X86InstrAVX512.td, the AVX instruction ID is less than AVX512 ID.
+    // We use the ID to sort AVX instruction before AVX512 instruction in
+    // matching table.
+    if (TheDef->isSubClassOf("Instruction") &&
+        TheDef->getValueAsBit("HasPositionOrder"))
+      return TheDef->getID() < RHS.TheDef->getID();
+
     return false;
   }
 
   /// couldMatchAmbiguouslyWith - Check whether this matchable could
   /// ambiguously match the same set of operands as \p RHS (without being a
   /// strictly superior match).
   bool couldMatchAmbiguouslyWith(const MatchableInfo &RHS) const {
     // The primary comparator is the instruction mnemonic.
     if (Mnemonic != RHS.Mnemonic)