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

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n marked 2 inline comments as done.
goldstein.w.n 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)
----------------
pengfei wrote:
> 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;
> }
> ```
> 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.




================
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
----------------
pengfei wrote:
> In which case will `vmovlhps` be transformed into `vshufps r, r, 0xee`?
> In which case will `vmovlhps` be transformed into `vshufps r, r, 0xee`?

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144570