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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added inline comments.


================
Comment at: llvm/lib/Target/X86/X86FixupInstTuning.cpp:73
+template <typename T>
+static std::optional<bool> CmpOptionals(T NewVal, T CurVal) {
+  if (NewVal != std::nullopt && CurVal != std::nullopt && *NewVal != *CurVal)
----------------
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;
}
```


================
Comment at: llvm/lib/Target/X86/X86FixupInstTuning.cpp:89
+                              bool Lat) -> std::optional<double> {
+    if (SM->hasInstrSchedModel()) {
+      const MCSchedClassDesc *SchedClassDesc =
----------------
Should be better hoist the check into `NewOpcPreferable` or `ProcessVPERMILPSmi` etc?


================
Comment at: llvm/lib/Target/X86/X86FixupInstTuning.cpp:92-93
+          SM->getSchedClassDesc(TII->get(Opcode).getSchedClass());
+      return Lat ? MCSchedModel::computeInstrLatency(*ST, *SchedClassDesc)
+                 : MCSchedModel::getReciprocalThroughput(*ST, *SchedClassDesc);
+    }
----------------
Should be better to sink them into `GetInstTput` and `GetInstLat`?


================
Comment at: llvm/lib/Target/X86/X86FixupInstTuning.cpp:162
+  // `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
----------------
In which case will `vmovlhps` be transformed into `vshufps r, r, 0xee`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144570/new/

https://reviews.llvm.org/D144570