[X86][MC] Optimize more instructions from VEX3 to VEX2 and fix the incorrect control flow in X86MCInstLower

[skan]

Summary

X86MCInstLower::Lower has a huge switch-cases, from which D150068 removed
VMOVSS. It caused the unoptimized VMOVSS to wrongly fall
to the default label. The default label exchanged the 1st and 2nd
operand, but we should exchange the 0th and 2nd operand.

In this patch, we move the code in the default lable about "VEX3 to
VEX2" to the function X86::optimizeInstFromVEX3ToVEX2 to fix the bug.

Since the function is shared, a side effect is that more encoding
optimizations are done on the Asmparser side. Considering we already
use reverse-encoding for optimization in AsmParser before this patch,
I believe the change is positive and expected.

[alexfh]

The patch seems to fix the problem, but it looks like it potentially changes generated code in more cases, and thus it might introduce more issues. Can we go the safe route: revert https://reviews.llvm.org/D150068 first and then land it together with the fix after it gets sufficient review?

Apart from that, the pre-merge checks are failing.

[skan]

In D150440#4337670, @alexfh wrote:

The patch seems to fix the problem, but it looks like it potentially changes generated code in more cases, and thus it might introduce more issues. Can we go the safe route: revert https://reviews.llvm.org/D150068 first and then land it together with the fix after it gets sufficient review?

Apart from that, the pre-merge checks are failing.

It seems that the pre-merge fails due to commits related to f470922a299f6036259c703a534a9b1e60573df7, not this.

And the tests pass after rebasing to ee75422ce1854208b12eb114e62ff9e3042570c7

[skan]

Rebase

[skan]

@bgraur I will revert D150068 first, so that we won't have to revert two separate commits if there is a coming issue. The fix in this patch seems straightforward to me and since D150068 was reviewed already, I will try to reland it with this fix.

[bgraur]

In D150440#4337821, @skan wrote:

@bgraur I will revert D150068 first, so that we won't have to revert two separate commits if there is a coming issue. The fix in this patch seems straightforward to me and since D150068 was reviewed already, I will try to reland it with this fix.

Perfect @skan Thanks a lot for reverting first

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

bool X86AsmParser::ParseInstruction(ParseInstructionInfo &Info, StringRef Name,
 
   if (Flags)
     Operands.push_back(X86Operand::CreatePrefix(Flags, NameLoc, NameLoc));
   return false;
 }
 
 bool X86AsmParser::processInstruction(MCInst &Inst, const OperandVector &Ops) {
   if (ForcedVEXEncoding != VEXEncoding_VEX3 &&
-      X86::optimizeInstFromVEX3ToVEX2(Inst))
+      X86::optimizeInstFromVEX3ToVEX2(Inst, MII.get(Inst.getOpcode())))
     return true;
 
   if (X86::optimizeShiftRotateWithImmediateOne(Inst))
     return true;
 
   switch (Inst.getOpcode()) {
   default: return false;
   case X86::JMP_1:

llvm/lib/Target/X86/MCTargetDesc/X86EncodingOptimization.h

 //===-- X86EncodingOptimization.h - X86 Encoding optimization ---*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains the declarations of the X86 encoding optimization
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_X86_X86ENCODINGOPTIMIZATION_H
 #define LLVM_LIB_TARGET_X86_X86ENCODINGOPTIMIZATION_H
 namespace llvm {
 class MCInst;
+class MCInstrDesc;
 namespace X86 {
-bool optimizeInstFromVEX3ToVEX2(MCInst &MI);
+bool optimizeInstFromVEX3ToVEX2(MCInst &MI, const MCInstrDesc &Desc);
 bool optimizeShiftRotateWithImmediateOne(MCInst &MI);
 } // namespace X86
 } // namespace llvm
 #endif

llvm/lib/Target/X86/MCTargetDesc/X86EncodingOptimization.cpp

 //===-- X86EncodingOptimization.cpp - X86 Encoding optimization -*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file contains the implementation of the X86 encoding optimization
 //
 //===----------------------------------------------------------------------===//
 
 #include "X86EncodingOptimization.h"
 #include "X86BaseInfo.h"
 #include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCInstrDesc.h"
 
 using namespace llvm;
 
-bool X86::optimizeInstFromVEX3ToVEX2(MCInst &MI) {
+static bool shouldExchange(const MCInst &MI, unsigned OpIdx1, unsigned OpIdx2) {
+  return !X86II::isX86_64ExtendedReg(MI.getOperand(OpIdx1).getReg()) &&
+         X86II::isX86_64ExtendedReg(MI.getOperand(OpIdx2).getReg());
+}
+
+bool X86::optimizeInstFromVEX3ToVEX2(MCInst &MI, const MCInstrDesc &Desc) {
   unsigned OpIdx1, OpIdx2;
   unsigned NewOpc;
+  unsigned Opcode = MI.getOpcode();
 #define FROM_TO(FROM, TO, IDX1, IDX2)                                          \
   case X86::FROM:                                                              \
     NewOpc = X86::TO;                                                          \
     OpIdx1 = IDX1;                                                             \
     OpIdx2 = IDX2;                                                             \
     break;
 #define TO_REV(FROM) FROM_TO(FROM, FROM##_REV, 0, 1)
   switch (MI.getOpcode()) {
-  default:
+  default: {
+    // If the instruction is a commutable arithmetic instruction we might be
+    // able to commute the operands to get a 2 byte VEX prefix.
+    uint64_t TSFlags = Desc.TSFlags;
+    if (!Desc.isCommutable() || (TSFlags & X86II::EncodingMask) != X86II::VEX ||
+        (TSFlags & X86II::OpMapMask) != X86II::TB ||
+        (TSFlags & X86II::FormMask) != X86II::MRMSrcReg ||
+        (TSFlags & X86II::REX_W) || !(TSFlags & X86II::VEX_4V) ||
+        MI.getNumOperands() != 3)
+      return false;
+    // These two are not truly commutable.
+    if (Opcode == X86::VMOVHLPSrr || Opcode == X86::VUNPCKHPDrr)
       return false;
+    OpIdx1 = 1;
+    OpIdx2 = 2;
+    if (!shouldExchange(MI, OpIdx1, OpIdx2))
+      return false;
+    std::swap(MI.getOperand(OpIdx1), MI.getOperand(OpIdx2));
+    return true;
+  }
     // Commute operands to get a smaller encoding by using VEX.R instead of
     // VEX.B if one of the registers is extended, but other isn't.
     FROM_TO(VMOVZPQILo2PQIrr, VMOVPQI2QIrr, 0, 1)
     TO_REV(VMOVAPDrr)
     TO_REV(VMOVAPDYrr)
     TO_REV(VMOVAPSrr)
     TO_REV(VMOVAPSYrr)
     TO_REV(VMOVDQArr)
     TO_REV(VMOVDQAYrr)
     TO_REV(VMOVDQUrr)
     TO_REV(VMOVDQUYrr)
     TO_REV(VMOVUPDrr)
     TO_REV(VMOVUPDYrr)
     TO_REV(VMOVUPSrr)
     TO_REV(VMOVUPSYrr)
 #undef TO_REV
 #define TO_REV(FROM) FROM_TO(FROM, FROM##_REV, 0, 2)
     TO_REV(VMOVSDrr)
     TO_REV(VMOVSSrr)
 #undef TO_REV
 #undef FROM_TO
   }
-  if (X86II::isX86_64ExtendedReg(MI.getOperand(OpIdx1).getReg()) ||
-      !X86II::isX86_64ExtendedReg(MI.getOperand(OpIdx2).getReg()))
+  if (!shouldExchange(MI, OpIdx1, OpIdx2))
     return false;
   MI.setOpcode(NewOpc);
   return true;
 }
 
 // NOTE: We may write this as an InstAlias if it's only used by AsmParser. See
 // validateTargetOperandClass.
 bool X86::optimizeShiftRotateWithImmediateOne(MCInst &MI) {

llvm/lib/Target/X86/X86MCInstLower.cpp

 
 void X86MCInstLower::Lower(const MachineInstr *MI, MCInst &OutMI) const {
   OutMI.setOpcode(MI->getOpcode());
 
   for (const MachineOperand &MO : MI->operands())
     if (auto MaybeMCOp = LowerMachineOperand(MI, MO))
       OutMI.addOperand(*MaybeMCOp);
 
-  if (X86::optimizeInstFromVEX3ToVEX2(OutMI))
+  if (X86::optimizeInstFromVEX3ToVEX2(OutMI, MI->getDesc()))
     return;
 
   // Handle a few special cases to eliminate operand modifiers.
   switch (OutMI.getOpcode()) {
   case X86::LEA64_32r:
   case X86::LEA64r:
   case X86::LEA16r:
   case X86::LEA32r:
if (!X86II::isX86_64ExtendedReg(OutMI.getOperand(1).getReg()) &&
       case 0x04: // NOT EQUAL
       case 0x07: // ORDERED
         std::swap(OutMI.getOperand(1), OutMI.getOperand(2));
         break;
       }
     }
     break;
   }
-
-  case X86::VMOVHLPSrr:
-  case X86::VUNPCKHPDrr:
-    // These are not truly commutable so hide them from the default case.
-    break;
-
   case X86::MASKMOVDQU:
   case X86::VMASKMOVDQU:
     if (AsmPrinter.getSubtarget().is64Bit())
       OutMI.setFlags(X86::IP_HAS_AD_SIZE);
     break;
 
   default: {
-    // If the instruction is a commutable arithmetic instruction we might be
-    // able to commute the operands to get a 2 byte VEX prefix.
-    uint64_t TSFlags = MI->getDesc().TSFlags;
-    if (MI->getDesc().isCommutable() &&
-        (TSFlags & X86II::EncodingMask) == X86II::VEX &&
-        (TSFlags & X86II::OpMapMask) == X86II::TB &&
-        (TSFlags & X86II::FormMask) == X86II::MRMSrcReg &&
-        !(TSFlags & X86II::REX_W) && (TSFlags & X86II::VEX_4V) &&
-        OutMI.getNumOperands() == 3) {
-      if (!X86II::isX86_64ExtendedReg(OutMI.getOperand(1).getReg()) &&
-          X86II::isX86_64ExtendedReg(OutMI.getOperand(2).getReg()))
-        std::swap(OutMI.getOperand(1), OutMI.getOperand(2));
-    }
     // Add an REP prefix to BSF instructions so that new processors can
     // recognize as TZCNT, which has better performance than BSF.
     if (X86::isBSF(OutMI.getOpcode()) && !MF.getFunction().hasOptSize()) {
       // BSF and TZCNT have different interpretations on ZF bit. So make sure
       // it won't be used later.
       const MachineOperand *FlagDef = MI->findRegisterDefOperand(X86::EFLAGS);
       if (FlagDef && FlagDef->isDead())
         OutMI.setFlags(X86::IP_HAS_REPEAT);

llvm/test/MC/X86/x86_64-avx-encoding.s

 // CHECK: vdivps  %ymm12, %ymm4, %ymm6
 // CHECK: encoding: [0xc4,0xc1,0x5c,0x5e,0xf4]
           vdivps  %ymm12, %ymm4, %ymm6
 
 // CHECK: vdivpd  %ymm12, %ymm4, %ymm6
 // CHECK: encoding: [0xc4,0xc1,0x5d,0x5e,0xf4]
           vdivpd  %ymm12, %ymm4, %ymm6
 
-// CHECK: vaddps  %ymm12, %ymm4, %ymm6
-// CHECK: encoding: [0xc4,0xc1,0x5c,0x58,0xf4]
+// CHECK: vaddps  %ymm4, %ymm12, %ymm6
+// CHECK: encoding: [0xc5,0x9c,0x58,0xf4]
           vaddps  %ymm12, %ymm4, %ymm6
 
-// CHECK: vaddpd  %ymm12, %ymm4, %ymm6
-// CHECK: encoding: [0xc4,0xc1,0x5d,0x58,0xf4]
+// CHECK: vaddpd  %ymm4, %ymm12, %ymm6
+// CHECK: encoding: [0xc5,0x9d,0x58,0xf4]
           vaddpd  %ymm12, %ymm4, %ymm6
 
-// CHECK: vmulps  %ymm12, %ymm4, %ymm6
-// CHECK: encoding: [0xc4,0xc1,0x5c,0x59,0xf4]
+// CHECK: vmulps  %ymm4, %ymm12, %ymm6
+// CHECK: encoding: [0xc5,0x9c,0x59,0xf4]
           vmulps  %ymm12, %ymm4, %ymm6
 
-// CHECK: vmulpd  %ymm12, %ymm4, %ymm6
-// CHECK: encoding: [0xc4,0xc1,0x5d,0x59,0xf4]
+// CHECK: vmulpd  %ymm4, %ymm12, %ymm6
+// CHECK: encoding: [0xc5,0x9d,0x59,0xf4]
           vmulpd  %ymm12, %ymm4, %ymm6
 
 // CHECK: vmaxps  (%rax), %ymm4, %ymm6
 // CHECK: encoding: [0xc5,0xdc,0x5f,0x30]
           vmaxps  (%rax), %ymm4, %ymm6
 
 // CHECK: vmaxpd  (%rax), %ymm4, %ymm6
 // CHECK: encoding: [0xc5,0xdd,0x5f,0x30]

llvm/test/tools/llvm-mca/X86/show-encoding.s

 # NORMAL-NEXT:         1      3     1.00    *                   movq  376(%rbp), %r11
 # NORMAL-NEXT:         1      5     1.00    *                   vmovups (%r10,%r9,4), %xmm3
 # NORMAL-NEXT:         1      1     0.50                        vpslldq $4, %xmm3, %xmm2
 # NORMAL-NEXT:         1      1     0.50                        vpslldq $4, %xmm3, %xmm4
 # NORMAL-NEXT:         1      3     1.00                        vaddps  %xmm2, %xmm3, %xmm6
 # NORMAL-NEXT:         1      1     0.50                        vpslldq $12, %xmm3, %xmm5
 # NORMAL-NEXT:         1      3     1.00                        vaddps  %xmm4, %xmm5, %xmm7
 # NORMAL-NEXT:         1      3     1.00                        vaddps  %xmm6, %xmm7, %xmm8
-# NORMAL-NEXT:         1      3     1.00                        vaddps  %xmm8, %xmm0, %xmm9
+# NORMAL-NEXT:         1      3     1.00                        vaddps  %xmm0, %xmm8, %xmm9
 # NORMAL-NEXT:         1      1     0.50                        vshufps $255, %xmm9, %xmm9, %xmm0
 # NORMAL-NEXT:         1      1     1.00           *            vmovups %xmm9, (%r11,%r9,4)
 # NORMAL-NEXT:         1      1     0.50                        cmpl  %r8d, %esi
 # NORMAL-NEXT:         1      1     0.50                        jl  -90
 
 # WITHENCODINGS:      [1]    [2]    [3]    [4]    [5]    [6]    [7]    Encodings:                    Instructions:
 # WITHENCODINGS-NEXT:  1      3     1.00    *                    7     4c 8b 95 70 01 00 00          movq 368(%rbp), %r10
 # WITHENCODINGS-NEXT:  1      2     1.00                         4     47 8d 0c 40                   leal (%r8,%r8,2), %r9d
 # WITHENCODINGS-NEXT:  1      1     0.50                         3     4d 63 c9                      movslq %r9d, %r9
 # WITHENCODINGS-NEXT:  1      1     0.50                         3     41 ff c0                      incl %r8d
 # WITHENCODINGS-NEXT:  1      3     1.00    *                    7     4c 8b 9d 78 01 00 00          movq 376(%rbp), %r11
 # WITHENCODINGS-NEXT:  1      5     1.00    *                    6     c4 81 78 10 1c 8a             vmovups  (%r10,%r9,4), %xmm3
 # WITHENCODINGS-NEXT:  1      1     0.50                         5     c5 e9 73 fb 04                vpslldq  $4, %xmm3, %xmm2
 # WITHENCODINGS-NEXT:  1      1     0.50                         5     c5 d9 73 fb 04                vpslldq  $4, %xmm3, %xmm4
 # WITHENCODINGS-NEXT:  1      3     1.00                         4     c5 e0 58 f2                   vaddps %xmm2, %xmm3, %xmm6
 # WITHENCODINGS-NEXT:  1      1     0.50                         5     c5 d1 73 fb 0c                vpslldq  $12, %xmm3, %xmm5
 # WITHENCODINGS-NEXT:  1      3     1.00                         4     c5 d0 58 fc                   vaddps %xmm4, %xmm5, %xmm7
 # WITHENCODINGS-NEXT:  1      3     1.00                         4     c5 40 58 c6                   vaddps %xmm6, %xmm7, %xmm8
-# WITHENCODINGS-NEXT:  1      3     1.00                         5     c4 41 78 58 c8                vaddps  %xmm8, %xmm0, %xmm9
+# WITHENCODINGS-NEXT:  1      3     1.00                         4     c5 38 58 c8                   vaddps  %xmm0, %xmm8, %xmm9
 # WITHENCODINGS-NEXT:  1      1     0.50                         6     c4 c1 30 c6 c1 ff             vshufps  $255, %xmm9, %xmm9, %xmm0
 # WITHENCODINGS-NEXT:  1      1     1.00           *             6     c4 01 78 11 0c 8b             vmovups  %xmm9, (%r11,%r9,4)
 # WITHENCODINGS-NEXT:  1      1     0.50                         3     44 39 c6                      cmpl %r8d, %esi
 # WITHENCODINGS-NEXT:  1      1     0.50                         6     0f 8c 00 00 00 00             jl -90