[X86][CodeGen] Fix crash when commute operands of Instruction for code size

[KanRobert]

Reported in 134fcc6

Incorrect opcode is used b/c there is a [[fallthrough]] at line 2386.

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: Shengchen Kan (KanRobert)

Changes

Reported in 134fcc6

---

Full diff: https://github.com/llvm/llvm-project/pull/79245.diff

1 Files Affected:

• (modified) llvm/lib/Target/X86/X86InstrInfo.cpp (+6-5)

diff --git a/llvm/lib/Target/X86/X86InstrInfo.cpp b/llvm/lib/Target/X86/X86InstrInfo.cpp
index d6f9aa6d6acec09..6cdfecf1fe4d99e 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.cpp
+++ b/llvm/lib/Target/X86/X86InstrInfo.cpp
@@ -2355,29 +2355,30 @@ MachineInstr *X86InstrInfo::commuteInstructionImpl(MachineInstr &MI, bool NewMI,
     // If we're optimizing for size, try to use MOVSD/MOVSS.
     if (MI.getParent()->getParent()->getFunction().hasOptSize()) {
       unsigned Mask;
+      unsigned NewOpc;
       switch (Opc) {
       default:
         llvm_unreachable("Unreachable!");
       case X86::BLENDPDrri:
-        Opc = X86::MOVSDrr;
+        NewOpc = X86::MOVSDrr;
         Mask = 0x03;
         break;
       case X86::BLENDPSrri:
-        Opc = X86::MOVSSrr;
+        NewOpc = X86::MOVSSrr;
         Mask = 0x0F;
         break;
       case X86::VBLENDPDrri:
-        Opc = X86::VMOVSDrr;
+        NewOpc = X86::VMOVSDrr;
         Mask = 0x03;
         break;
       case X86::VBLENDPSrri:
-        Opc = X86::VMOVSSrr;
+        NewOpc = X86::VMOVSSrr;
         Mask = 0x0F;
         break;
       }
       if ((MI.getOperand(3).getImm() ^ Mask) == 1) {
         WorkingMI = CloneIfNew(MI);
-        WorkingMI->setDesc(get(Opc));
+        WorkingMI->setDesc(get(NewOpc));
         WorkingMI->removeOperand(3);
         break;
       }

[KanRobert]

Difficult for me to create a test for this, maybe @gulfemsavrun could provide a reproducer later.

[phoebewang]

Difficult for me to create a test for this, maybe @gulfemsavrun could provide a reproducer later.

$ cat tmp.ll
define <4 x float> @foo(<4 x float> %a, <4 x float> %b) optsize {
  %r = shufflevector <4 x float> %b, <4 x float> %a, <4 x i32> <i32 0, i32 5, i32 2, i32 3>
  ret <4 x float> %r
}

$ llc < tmp.ll -mattr=+avx2
        .text
        .file   "<stdin>"
Unreachable!
UNREACHABLE executed at /export/users/pengfeiw/llvm-project/llvm/lib/Target/X86/X86InstrInfo.cpp:2396!

[KanRobert]

Difficult for me to create a test for this, maybe @gulfemsavrun could provide a reproducer later.

$ cat tmp.ll
define <4 x float> @foo(<4 x float> %a, <4 x float> %b) optsize {
  %r = shufflevector <4 x float> %b, <4 x float> %a, <4 x i32> <i32 0, i32 5, i32 2, i32 3>
  ret <4 x float> %r
}

$ llc < tmp.ll -mattr=+avx2
        .text
        .file   "<stdin>"
Unreachable!
UNREACHABLE executed at /export/users/pengfeiw/llvm-project/llvm/lib/Target/X86/X86InstrInfo.cpp:2396!

Awesome! Just added.

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 71d64ed80f8b7556be6954b2c4d663c7d89f476d ebc5996aba54ff30bef1c6f9af9811cb582ca98c -- llvm/lib/Target/X86/X86InstrInfo.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Target/X86/X86InstrInfo.cpp b/llvm/lib/Target/X86/X86InstrInfo.cpp
index 975d12d271..7cc9bd9c2e 100644
--- a/llvm/lib/Target/X86/X86InstrInfo.cpp
+++ b/llvm/lib/Target/X86/X86InstrInfo.cpp
@@ -2326,7 +2326,8 @@ MachineInstr *X86InstrInfo::commuteInstructionImpl(MachineInstr &MI, bool NewMI,
   case X86::VBLENDPSrri:
     // If we're optimizing for size, try to use MOVSD/MOVSS.
     if (MI.getParent()->getParent()->getFunction().hasOptSize()) {
-      unsigned Mask = (Opc == X86::BLENDPDrri || Opc == X86::VBLENDPDrri) ? 0x03: 0x0F;
+      unsigned Mask =
+          (Opc == X86::BLENDPDrri || Opc == X86::VBLENDPDrri) ? 0x03 : 0x0F;
       if ((MI.getOperand(3).getImm() ^ Mask) == 1) {
 #define FROM_TO(FROM, TO)                                                      \
   case X86::FROM:                                                              \
@@ -2335,10 +2336,10 @@ MachineInstr *X86InstrInfo::commuteInstructionImpl(MachineInstr &MI, bool NewMI,
         switch (Opc) {
         default:
           llvm_unreachable("Unreachable!");
-        FROM_TO(BLENDPDrri, MOVSDrr)
-        FROM_TO(BLENDPSrri, MOVSSrr)
-        FROM_TO(VBLENDPDrri, VMOVSDrr)
-        FROM_TO(VBLENDPSrri, VMOVSSrr)
+          FROM_TO(BLENDPDrri, MOVSDrr)
+          FROM_TO(BLENDPSrri, MOVSSrr)
+          FROM_TO(VBLENDPDrri, VMOVSDrr)
+          FROM_TO(VBLENDPSrri, VMOVSSrr)
         }
         WorkingMI = CloneIfNew(MI);
         WorkingMI->setDesc(get(Opc));

[RKSimon]

For 18.x are we better off reverting 134fcc6 or cherry picking this patch when its completed? For safety I'm currently preferring reversion.

[phoebewang]

Nit, using FROM, TO to replace A, B

[phoebewang]

LGTM.

[KanRobert]

For 18.x are we better off reverting 134fcc6 or cherry picking this patch when its completed? For safety I'm currently preferring reversion.

I think it would not be a clean revert. I prefer a cherry-pick @RKSimon

[gulfemsavrun]

I confirm that this fixed the issue that I reported in 134fcc6.