[X86] Add support for using Sched/Codesize information to `X86FixupInstTuning` Pass.

[goldstein.w.n]

Summary

Use this to handle new transform: {v}unpck{l|h}pd -> {v}shufps. We
need the sched information here as {v}shufps is 1 more byte of code
size, so we only want to make this transformation if {v}shufps is
actually faster.

[pengfei]

llvm/test/CodeGen/X86/tuning-shuffle-unpckpd.ll

63–66 ↗

(On Diff #499546)

This seems a bad check. Same below.

[goldstein.w.n]

llvm/test/CodeGen/X86/tuning-shuffle-unpckpd.ll

63–66 ↗

(On Diff #499546)

Fixed, sorry about that.

[goldstein.w.n]

Fix broken checks

[goldstein.w.n]

Rebase

[goldstein.w.n]

Fixup comment

[goldstein.w.n]

Rebase

[pengfei]

llvm/lib/Target/X86/X86FixupInstTuning.cpp

73

Don't see much value to use optinal, should be better to use bool directly?

template <typename T>
static bool CmpOptionals(T NewVal, T CurVal) {
  if (NewVal && CurVal)
    return *NewVal < *CurVal;

  return false;
}

89

Should be better hoist the check into NewOpcPreferable or ProcessVPERMILPSmi etc?

92–93

Should be better to sink them into GetInstTput and GetInstLat?

179

In which case will vmovlhps be transformed into vshufps r, r, 0xee?

[goldstein.w.n]

llvm/lib/Target/X86/X86FixupInstTuning.cpp

73

Don't see much value to use optinal, should be better to use bool directly?

template <typename T>
static bool CmpOptionals(T NewVal, T CurVal) {
  if (NewVal && CurVal)
    return *NewVal < *CurVal;

  return false;
}

The thing is we need three states:

1. <true> New Val better
2. <false> Old Val better
3. <nullopt> No difference (either unable to get information i.e no schedmodel) or the value are equal.

<true> -> make change
<false> -> don't make change
<nullopt> -> check next metric.

179

In which case will vmovlhps be transformed into vshufps r, r, 0xee?

see the define <4 x float> @transform_VUNPCKLPDrr test case.

[goldstein.w.n]

Make lat/tput cleaner

[goldstein.w.n]

Rebase

[goldstein.w.n]

Rebase

[goldstein.w.n]

Rebase (after D144832, no dep on D143786)

[RKSimon]

Maybe use has_value() instead of nullopt comparisons?

llvm/lib/Target/X86/X86FixupInstTuning.cpp

77

Maybe?
if (NewVal.has_value() && CurVal.has_value() && *NewVal != *CurVal)

[goldstein.w.n]

In D144570#4171129, @RKSimon wrote:

Maybe use has_value() instead of nullopt comparisons?

Done.

[goldstein.w.n]

Use .has_value() instead of nullopt comparison

[RKSimon]

Add ICX test runs to tuning-shuffle-unpckpd.ll so we have test coverage ?

llvm/lib/Target/X86/X86FixupInstTuning.cpp

101

if (unsigned Size = TII->get(Opcode).getSize())
  return Size;

[goldstein.w.n]

In D144570#4174550, @RKSimon wrote:

Add ICX test runs to tuning-shuffle-unpckpd.ll so we have test coverage ?

Done. See D145531

[goldstein.w.n]

Nit + ICX

[RKSimon]

LGTM with one minor

llvm/test/CodeGen/X86/tuning-shuffle-unpckpd.ll

3 ↗

(On Diff #503182)

SKX/v3 can probably share a CHECK-AVX2 common prefix:

--check-prefixes=CHECK,CHECK-AVX2,CHECK-SKL
--check-prefixes=CHECK,CHECK-AVX2,CHECK-V3

[goldstein.w.n]

Add common check for AVX2

llvm/lib/Target/X86/X86FixupInstTuning.cpp

public:
   MachineFunctionProperties getRequiredProperties() const override {
     return MachineFunctionProperties().set(
         MachineFunctionProperties::Property::NoVRegs);
   }
 
 private:
   const X86InstrInfo *TII = nullptr;
   const X86Subtarget *ST = nullptr;
+  const MCSchedModel *SM = nullptr;
 };
 } // end anonymous namespace
 
 char X86FixupInstTuningPass::ID = 0;
 
 INITIALIZE_PASS(X86FixupInstTuningPass, DEBUG_TYPE, DEBUG_TYPE, false, false)
 
 FunctionPass *llvm::createX86FixupInstTuning() {
   return new X86FixupInstTuningPass();
 }
 
+template <typename T>
+static std::optional<bool> CmpOptionals(T NewVal, T CurVal) {
+  if (NewVal != std::nullopt && CurVal != std::nullopt && *NewVal != *CurVal)
+    return *NewVal < *CurVal;
+
+  return std::nullopt;
+}
+
 bool X86FixupInstTuningPass::processInstruction(
     MachineFunction &MF, MachineBasicBlock &MBB,
     MachineBasicBlock::iterator &I) {
   MachineInstr &MI = *I;
   unsigned Opc = MI.getOpcode();
   unsigned NumOperands = MI.getDesc().getNumOperands();
 
+  auto GetInstTput = [&](unsigned Opcode) -> std::optional<double> {
+    // We already checked that SchedModel exists in `NewOpcPreferable`.
+    return MCSchedModel::getReciprocalThroughput(
+        *ST, *(SM->getSchedClassDesc(TII->get(Opcode).getSchedClass())));
+  };
+
+  auto GetInstLat = [&](unsigned Opcode) -> std::optional<double> {
+    // We already checked that SchedModel exists in `NewOpcPreferable`.
+    return MCSchedModel::computeInstrLatency(
+        *ST, *(SM->getSchedClassDesc(TII->get(Opcode).getSchedClass())));
+  };
+
+  auto GetInstSize = [&](unsigned Opcode) -> std::optional<unsigned> {
+    unsigned Size = TII->get(Opcode).getSize();
+    if (Size)
+      return Size;
+    // Zero size means we where unable to compute it.
+    return std::nullopt;
+  };
+
+  auto NewOpcPreferable = [&](unsigned NewOpc,
+                              bool ReplaceInTie = true) -> bool {
+    std::optional<bool> Res;
+    if (SM->hasInstrSchedModel()) {
+      // Compare tput -> lat -> code size.
+      Res = CmpOptionals(GetInstTput(NewOpc), GetInstTput(Opc));
+      if (Res != std::nullopt)
+        return *Res;
+
+      Res = CmpOptionals(GetInstLat(NewOpc), GetInstLat(Opc));
+      if (Res != std::nullopt)
+        return *Res;
+    }
+
+    Res = CmpOptionals(GetInstSize(Opc), GetInstSize(NewOpc));
+    if (Res != std::nullopt)
+      return *Res;
+
+    // We either have either were unable to get tput/lat/codesize or all values
+    // were equal. Return specified option for a tie.
+    return ReplaceInTie;
+  };
+
   // `vpermilps r, i` -> `vshufps r, r, i`
-  // `vshufps` is always as fast or faster than `vpermilps` and takes 1 less
-  // byte of code size.
+  // `vshufps` is always as fast or faster than
+  // `vpermilps` and takes 1 less byte of code size.
   auto ProcessVPERMILPSri = [&](unsigned NewOpc) -> bool {
+    if (!NewOpcPreferable(NewOpc))
+      return false;
     unsigned MaskImm = MI.getOperand(NumOperands - 1).getImm();
     MI.removeOperand(NumOperands - 1);
     MI.addOperand(MI.getOperand(1));
     MI.setDesc(TII->get(NewOpc));
     MI.addOperand(MachineOperand::CreateImm(MaskImm));
     return true;
   };
 
   // `vpermilps m, i` -> `vpshufd m, i` iff no domain delay penalty on shuffles.
   // `vpshufd` is always as fast or faster than `vpermilps` and takes 1 less
   // byte of code size.
   auto ProcessVPERMILPSmi = [&](unsigned NewOpc) -> bool {
     // TODO: Might be work adding bypass delay if -Os/-Oz is enabled as
     // `vpshufd` saves a byte of code size.
-    if (!ST->hasNoDomainDelayShuffle())
+    if (!ST->hasNoDomainDelayShuffle() &&
+        !NewOpcPreferable(NewOpc, /*ReplaceInTie*/ false))
       return false;
     MI.setDesc(TII->get(NewOpc));
     return true;
   };
 
+  // `vunpcklpd/vmovlhps r, r` -> `vshufps r, r, 0x44`
+  // `vunpckhpd/vmovlhps r, r` -> `vshufps r, r, 0xee`
+  // iff `vshufps` is faster than `vunpck{l|h}pd`. Otherwise stick with
+  // `vunpck{l|h}pd` as it uses less code size.
+  // TODO: Look into using `{VP}UNPCK{L|H}QDQ{...}` instead of `{V}SHUF{...}PS`
+  // as the replacement. `{VP}UNPCK{L|H}QDQ{...}` has no codesize cost.
+  auto ProcessUNPCKPD = [&](unsigned NewOpc, unsigned MaskImm) -> bool {
+    if (!NewOpcPreferable(NewOpc, /*ReplaceInTie*/ false))
+      return false;
+
+    MI.setDesc(TII->get(NewOpc));
+    MI.addOperand(MachineOperand::CreateImm(MaskImm));
+    return true;
+  };
+  auto ProcessUNPCKLPDrr = [&](unsigned NewOpc) -> bool {
+    return ProcessUNPCKPD(NewOpc, 0x44);
+  };
+  auto ProcessUNPCKHPDrr = [&](unsigned NewOpc) -> bool {
+    return ProcessUNPCKPD(NewOpc, 0xee);
+  };
+
   // TODO: Add masked predicate execution variants.
   switch (Opc) {
   case X86::VPERMILPSri:
     return ProcessVPERMILPSri(X86::VSHUFPSrri);
   case X86::VPERMILPSYri:
     return ProcessVPERMILPSri(X86::VSHUFPSYrri);
   case X86::VPERMILPSZ128ri:
     return ProcessVPERMILPSri(X86::VSHUFPSZ128rri);
   case X86::VPERMILPSZ256ri:
     return ProcessVPERMILPSri(X86::VSHUFPSZ256rri);
   case X86::VPERMILPSZri:
     return ProcessVPERMILPSri(X86::VSHUFPSZrri);
   case X86::VPERMILPSmi:
     return ProcessVPERMILPSmi(X86::VPSHUFDmi);
   case X86::VPERMILPSYmi:
     // TODO: See if there is a more generic way we can test if the replacement
     // instruction is supported.
     return ST->hasAVX2() ? ProcessVPERMILPSmi(X86::VPSHUFDYmi) : false;
   case X86::VPERMILPSZ128mi:
     return ProcessVPERMILPSmi(X86::VPSHUFDZ128mi);
   case X86::VPERMILPSZ256mi:
     return ProcessVPERMILPSmi(X86::VPSHUFDZ256mi);
   case X86::VPERMILPSZmi:
     return ProcessVPERMILPSmi(X86::VPSHUFDZmi);
+
+    // TODO: {V}UNPCK{L|H}PD{...} is probably safe to transform to
+    // `{VP}UNPCK{L|H}QDQ{...}` which gets the same perf benefit as
+    // `{V}SHUF{...}PS` but 1) without increasing code size and 2) can also
+    // handle the `mr` case. ICL doesn't have a domain penalty for replacing
+    // float unpck -> int unpck, but at this time, I haven't verified the set of
+    // processors where its safe.
+  case X86::MOVLHPSrr:
+  case X86::UNPCKLPDrr:
+    return ProcessUNPCKLPDrr(X86::SHUFPSrri);
+  case X86::VMOVLHPSrr:
+  case X86::VUNPCKLPDrr:
+    return ProcessUNPCKLPDrr(X86::VSHUFPSrri);
+  case X86::VUNPCKLPDYrr:
+    return ProcessUNPCKLPDrr(X86::VSHUFPSYrri);
+    // VMOVLHPS is always 128 bits.
+  case X86::VMOVLHPSZrr:
+  case X86::VUNPCKLPDZ128rr:
+    return ProcessUNPCKLPDrr(X86::VSHUFPSZ128rri);
+  case X86::VUNPCKLPDZ256rr:
+    return ProcessUNPCKLPDrr(X86::VSHUFPSZ256rri);
+  case X86::VUNPCKLPDZrr:
+    return ProcessUNPCKLPDrr(X86::VSHUFPSZrri);
+  case X86::UNPCKHPDrr:
+    return ProcessUNPCKHPDrr(X86::SHUFPSrri);
+  case X86::VUNPCKHPDrr:
+    return ProcessUNPCKHPDrr(X86::VSHUFPSrri);
+  case X86::VUNPCKHPDYrr:
+    return ProcessUNPCKHPDrr(X86::VSHUFPSYrri);
+  case X86::VUNPCKHPDZ128rr:
+    return ProcessUNPCKHPDrr(X86::VSHUFPSZ128rri);
+  case X86::VUNPCKHPDZ256rr:
+    return ProcessUNPCKHPDrr(X86::VSHUFPSZ256rri);
+  case X86::VUNPCKHPDZrr:
+    return ProcessUNPCKHPDrr(X86::VSHUFPSZrri);
   default:
     return false;
   }
 }
 
 bool X86FixupInstTuningPass::runOnMachineFunction(MachineFunction &MF) {
   LLVM_DEBUG(dbgs() << "Start X86FixupInstTuning\n";);
   bool Changed = false;
   ST = &MF.getSubtarget<X86Subtarget>();
   TII = ST->getInstrInfo();
+  SM = &ST->getSchedModel();
+
   for (MachineBasicBlock &MBB : MF) {
     for (MachineBasicBlock::iterator I = MBB.begin(); I != MBB.end(); ++I) {
       if (processInstruction(MF, MBB, I)) {
         ++NumInstChanges;
         Changed = true;
       }
     }
   }
   LLVM_DEBUG(dbgs() << "End X86FixupInstTuning\n";);
   return Changed;
 }

llvm/test/CodeGen/X86/tuning-shuffle-unpckpd-avx512.ll

 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    vunpckhpd {{.*#+}} zmm0 = zmm0[1],zmm1[1],zmm0[3],zmm1[3],zmm0[5],zmm1[5],zmm0[7],zmm1[7]
 ; CHECK-NEXT:    retq
   %shufp = shufflevector <16 x float> %a, <16 x float> %b, <16 x i32> <i32 2, i32 3, i32 18, i32 19, i32 6, i32 7, i32 22, i32 23, i32 10, i32 11, i32 26, i32 27, i32 14, i32 15, i32 30, i32 31>
   ret <16 x float> %shufp
 }
 
 define <8 x float> @transform_VUNPCKLPDYrr(<8 x float> %a, <8 x float> %b) nounwind {
-; CHECK-LABEL: transform_VUNPCKLPDYrr:
-; CHECK:       # %bb.0:
-; CHECK-NEXT:    vunpcklpd {{.*#+}} ymm0 = ymm0[0],ymm1[0],ymm0[2],ymm1[2]
-; CHECK-NEXT:    retq
+; CHECK-SKX-LABEL: transform_VUNPCKLPDYrr:
+; CHECK-SKX:       # %bb.0:
+; CHECK-SKX-NEXT:    vunpcklpd {{.*#+}} ymm0 = ymm0[0],ymm1[0],ymm0[2],ymm1[2]
+; CHECK-SKX-NEXT:    retq
+;
+; CHECK-ICX-LABEL: transform_VUNPCKLPDYrr:
+; CHECK-ICX:       # %bb.0:
+; CHECK-ICX-NEXT:    vshufps {{.*#+}} ymm0 = ymm0[0,1],ymm1[0,1],ymm0[4,5],ymm1[4,5]
+; CHECK-ICX-NEXT:    retq
+;
+; CHECK-V4-LABEL: transform_VUNPCKLPDYrr:
+; CHECK-V4:       # %bb.0:
+; CHECK-V4-NEXT:    vunpcklpd {{.*#+}} ymm0 = ymm0[0],ymm1[0],ymm0[2],ymm1[2]
+; CHECK-V4-NEXT:    retq
+;
+; CHECK-AVX512-LABEL: transform_VUNPCKLPDYrr:
+; CHECK-AVX512:       # %bb.0:
+; CHECK-AVX512-NEXT:    vunpcklpd {{.*#+}} ymm0 = ymm0[0],ymm1[0],ymm0[2],ymm1[2]
+; CHECK-AVX512-NEXT:    retq
+;
+; CHECK-ZNVER4-LABEL: transform_VUNPCKLPDYrr:
+; CHECK-ZNVER4:       # %bb.0:
+; CHECK-ZNVER4-NEXT:    vunpcklpd {{.*#+}} ymm0 = ymm0[0],ymm1[0],ymm0[2],ymm1[2]
+; CHECK-ZNVER4-NEXT:    retq
   %shufp = shufflevector <8 x float> %a, <8 x float> %b, <8 x i32> <i32 0, i32 1, i32 8, i32 9, i32 4, i32 5, i32 12, i32 13>
   ret <8 x float> %shufp
 }
 
 define <8 x float> @transform_VUNPCKHPDYrr(<8 x float> %a, <8 x float> %b) nounwind {
-; CHECK-LABEL: transform_VUNPCKHPDYrr:
-; CHECK:       # %bb.0:
-; CHECK-NEXT:    vunpckhpd {{.*#+}} ymm0 = ymm0[1],ymm1[1],ymm0[3],ymm1[3]
-; CHECK-NEXT:    retq
+; CHECK-SKX-LABEL: transform_VUNPCKHPDYrr:
+; CHECK-SKX:       # %bb.0:
+; CHECK-SKX-NEXT:    vunpckhpd {{.*#+}} ymm0 = ymm0[1],ymm1[1],ymm0[3],ymm1[3]
+; CHECK-SKX-NEXT:    retq
+;
+; CHECK-ICX-LABEL: transform_VUNPCKHPDYrr:
+; CHECK-ICX:       # %bb.0:
+; CHECK-ICX-NEXT:    vshufps {{.*#+}} ymm0 = ymm0[2,3],ymm1[2,3],ymm0[6,7],ymm1[6,7]
+; CHECK-ICX-NEXT:    retq
+;
+; CHECK-V4-LABEL: transform_VUNPCKHPDYrr:
+; CHECK-V4:       # %bb.0:
+; CHECK-V4-NEXT:    vunpckhpd {{.*#+}} ymm0 = ymm0[1],ymm1[1],ymm0[3],ymm1[3]
+; CHECK-V4-NEXT:    retq
+;
+; CHECK-AVX512-LABEL: transform_VUNPCKHPDYrr:
+; CHECK-AVX512:       # %bb.0:
+; CHECK-AVX512-NEXT:    vunpckhpd {{.*#+}} ymm0 = ymm0[1],ymm1[1],ymm0[3],ymm1[3]
+; CHECK-AVX512-NEXT:    retq
+;
+; CHECK-ZNVER4-LABEL: transform_VUNPCKHPDYrr:
+; CHECK-ZNVER4:       # %bb.0:
+; CHECK-ZNVER4-NEXT:    vunpckhpd {{.*#+}} ymm0 = ymm0[1],ymm1[1],ymm0[3],ymm1[3]
+; CHECK-ZNVER4-NEXT:    retq
   %shufp = shufflevector <8 x float> %a, <8 x float> %b, <8 x i32> <i32 2, i32 3, i32 10, i32 11, i32 6, i32 7, i32 14, i32 15>
   ret <8 x float> %shufp
 }
 
 define <4 x float> @transform_VUNPCKLPDrr(<4 x float> %a, <4 x float> %b) nounwind {
-; CHECK-LABEL: transform_VUNPCKLPDrr:
-; CHECK:       # %bb.0:
-; CHECK-NEXT:    vmovlhps {{.*#+}} xmm0 = xmm0[0],xmm1[0]
-; CHECK-NEXT:    retq
+; CHECK-SKX-LABEL: transform_VUNPCKLPDrr:
+; CHECK-SKX:       # %bb.0:
+; CHECK-SKX-NEXT:    vmovlhps {{.*#+}} xmm0 = xmm0[0],xmm1[0]
+; CHECK-SKX-NEXT:    retq
+;
+; CHECK-ICX-LABEL: transform_VUNPCKLPDrr:
+; CHECK-ICX:       # %bb.0:
+; CHECK-ICX-NEXT:    vshufps {{.*#+}} xmm0 = xmm0[0,1],xmm1[0,1]
+; CHECK-ICX-NEXT:    retq
+;
+; CHECK-V4-LABEL: transform_VUNPCKLPDrr:
+; CHECK-V4:       # %bb.0:
+; CHECK-V4-NEXT:    vmovlhps {{.*#+}} xmm0 = xmm0[0],xmm1[0]
+; CHECK-V4-NEXT:    retq
+;
+; CHECK-AVX512-LABEL: transform_VUNPCKLPDrr:
+; CHECK-AVX512:       # %bb.0:
+; CHECK-AVX512-NEXT:    vmovlhps {{.*#+}} xmm0 = xmm0[0],xmm1[0]
+; CHECK-AVX512-NEXT:    retq
+;
+; CHECK-ZNVER4-LABEL: transform_VUNPCKLPDrr:
+; CHECK-ZNVER4:       # %bb.0:
+; CHECK-ZNVER4-NEXT:    vmovlhps {{.*#+}} xmm0 = xmm0[0],xmm1[0]
+; CHECK-ZNVER4-NEXT:    retq
   %shufp = shufflevector <4 x float> %a, <4 x float> %b, <4 x i32> <i32 0, i32 1, i32 4, i32 5>
   ret <4 x float> %shufp
 }
 
 define <4 x float> @transform_VUNPCKHPDrr(<4 x float> %a, <4 x float> %b) nounwind {
-; CHECK-LABEL: transform_VUNPCKHPDrr:
-; CHECK:       # %bb.0:
-; CHECK-NEXT:    vunpckhpd {{.*#+}} xmm0 = xmm0[1],xmm1[1]
-; CHECK-NEXT:    retq
+; CHECK-SKX-LABEL: transform_VUNPCKHPDrr:
+; CHECK-SKX:       # %bb.0:
+; CHECK-SKX-NEXT:    vunpckhpd {{.*#+}} xmm0 = xmm0[1],xmm1[1]
+; CHECK-SKX-NEXT:    retq
+;
+; CHECK-ICX-LABEL: transform_VUNPCKHPDrr:
+; CHECK-ICX:       # %bb.0:
+; CHECK-ICX-NEXT:    vshufps {{.*#+}} xmm0 = xmm0[2,3],xmm1[2,3]
+; CHECK-ICX-NEXT:    retq
+;
+; CHECK-V4-LABEL: transform_VUNPCKHPDrr:
+; CHECK-V4:       # %bb.0:
+; CHECK-V4-NEXT:    vunpckhpd {{.*#+}} xmm0 = xmm0[1],xmm1[1]
+; CHECK-V4-NEXT:    retq
+;
+; CHECK-AVX512-LABEL: transform_VUNPCKHPDrr:
+; CHECK-AVX512:       # %bb.0:
+; CHECK-AVX512-NEXT:    vunpckhpd {{.*#+}} xmm0 = xmm0[1],xmm1[1]
+; CHECK-AVX512-NEXT:    retq
+;
+; CHECK-ZNVER4-LABEL: transform_VUNPCKHPDrr:
+; CHECK-ZNVER4:       # %bb.0:
+; CHECK-ZNVER4-NEXT:    vunpckhpd {{.*#+}} xmm0 = xmm0[1],xmm1[1]
+; CHECK-ZNVER4-NEXT:    retq
   %shufp = shufflevector <4 x float> %a, <4 x float> %b, <4 x i32> <i32 2, i32 3, i32 6, i32 7>
   ret <4 x float> %shufp
 }
 
 define <8 x double> @transform_VUNPCKLPDZrrkz(<8 x double> %a, <8 x double> %b, i8 %mask_int) nounwind {
 ; CHECK-LABEL: transform_VUNPCKLPDZrrkz:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    kmovd %edi, %k1

 ; CHECK-NEXT:    vmovapd %xmm1, %xmm0
 ; CHECK-NEXT:    retq
   %mask = bitcast i2 %mask_int to <2 x i1>
   %b = load <2 x double>, ptr %pb
   %shufp = shufflevector <2 x double> %a, <2 x double> %b, <2 x i32> <i32 1, i32 3>
   %res = select <2 x i1> %mask, <2 x double> %shufp, <2 x double> %c
   ret <2 x double> %res
 }
-;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
-; CHECK-AVX512: {{.*}}
-; CHECK-ICX: {{.*}}
-; CHECK-SKX: {{.*}}
-; CHECK-V4: {{.*}}
-; CHECK-ZNVER4: {{.*}}