[PATCH] D27811: [CostModel][X86] Add support for broadcast shuffle costs

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Simon,

I noticed that `getShuffleCost` is becoming quite big. 
Do you think it makes sense to split the logic in `getShuffleCost` in three parts (a function for each supported ShuffleKind)?. In case, `getShuffleCost` could be refactored in a separate commit.



================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:614
+                                              ISD::VECTOR_SHUFFLE, LT.second))
+        return Entry->Cost;
+
----------------
Shouldn't this be `LT.first * Entry->Cost` ?
I think that you are not accounting for the type legalization cost.


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:626
+              CostTableLookup(AVX512ShuffleTbl, ISD::VECTOR_SHUFFLE, LT.second))
+        return Entry->Cost;
+
----------------
Same.


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:640
+              CostTableLookup(AVX2ShuffleTbl, ISD::VECTOR_SHUFFLE, LT.second))
+        return Entry->Cost;
+
----------------
Same (also, see lines 654, 664, 677 and 686).


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:679-686
+    static const CostTblEntry SSE1ShuffleTbl[] = {
+        { ISD::VECTOR_SHUFFLE, MVT::v4f32, 1 }, // shufps
+    };
+
+    if (ST->hasSSE1())
+      if (const auto *Entry =
+              CostTableLookup(SSE1ShuffleTbl, ISD::VECTOR_SHUFFLE, LT.second))
----------------
Can we not just simplify this code into something like this?

```
if (ST->hasSSE1() && LT.second == MVT::v4f32)
  return LT.first;
```

You are basically performing a lookup on a table with just a single entry.



Repository:
  rL LLVM

https://reviews.llvm.org/D27811