[PATCH] D15604: Changes in conversion cost model for X86 target

[Cong Hou via llvm-commits]

congh added a comment.

This patch overall looks very nice! Please check my inline comments.


================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:837
@@ +836,3 @@
+      SrcVT = LTSrc.second;
+      SrcSplitFactor = LTSrc.first;
+    } else if (SrcTy.isSimple())
----------------
Those two statements can be merge into one:

std::tie(SrcSplitFactor, SrcVT) = LTSrc;

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:851
@@ +850,3 @@
+    else
+      return BaseT::getCastInstrCost(Opcode, Dst, Dst);
+
----------------
Use a lambda to avoid code duplication?

Also should the last parameter be Src?

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:861
@@ +860,3 @@
+      else if (DstSplitFactor > SrcSplitFactor)
+        // Split DstVT
+        SrcVT = MVT::getVectorVT(SrcVT.getScalarType(),
----------------
Split SrcVT?

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:864
@@ +863,3 @@
+                                 SrcVT.getVectorNumElements() /
+                                 (DstSplitFactor / SrcSplitFactor));
+    }
----------------
Instead of further splitting SrcVT/DstVT, should we do the opposite? For example, given SrcVT = v16i16, and DstVT = v16i32, and on SSE2 we have SrcSplitFactor = 2, and DstSplitFactor = 4. Now suppose we have an cost entry for SrcVT = v8i16 and DstVT = v8i32, should we use this entry to estimate the cost? What you are doing here is making SrcVT = v4i16 and DstVT = v4i32, which gives less precise cost.

================
Comment at: ../lib/Target/X86/X86TargetTransformInfo.cpp:867
@@ +866,3 @@
+    else
+      CheckOriginalTypes = false;
+  }
----------------
Is this necessary? When we reach here, SrcVT/DstVT should be the same as OrgSimpleSrcVT/OrgSimpleDstVT, so I think it is safe to let CheckOriginalTypes keep its true value.


Repository:
  rL LLVM

http://reviews.llvm.org/D15604