[PATCH] D45720: [X86] Lowering PACK*S (pack with saturation) intrinsics to native IR (clang side)

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8420
+  if (IsUnsigned) {
+    MinVal = (IsDW) ? llvm::APInt::getMinValue(16).getZExtValue()
+                    : llvm::APInt::getMinValue(8).getZExtValue();
----------------
Why can't these just be APInts instead of uint64_t? Is this so that APInt widths don't have to match RTy below? I'd rather you just created the narrow APInt and then called sext/zext on it to get it to the right width.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8420
+  if (IsUnsigned) {
+    MinVal = (IsDW) ? llvm::APInt::getMinValue(16).getZExtValue()
+                    : llvm::APInt::getMinValue(8).getZExtValue();
----------------
craig.topper wrote:
> Why can't these just be APInts instead of uint64_t? Is this so that APInt widths don't have to match RTy below? I'd rather you just created the narrow APInt and then called sext/zext on it to get it to the right width.
Pre-select the 8 or 16 based on IsDW. Then you don't need to check IsDW 4 times. You just need to pass the right width.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8432
+  SmallVector<uint32_t, 16> ShuffleMask;
+  ShuffleMask.clear();
+  for (int i = 0, i1 = 0, i2 = 0, d = (IsDW) ? 4 : 8; i < NumElts; ++i)
----------------
Clearing isn't necessary if you just created it.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8433
+  ShuffleMask.clear();
+  for (int i = 0, i1 = 0, i2 = 0, d = (IsDW) ? 4 : 8; i < NumElts; ++i)
+    if ((i / d) & 1)
----------------
This loop could probably use some comments. The multiple variables make the logic hard to follow


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8443
+  Value *MaxVec = llvm::ConstantInt::get(RTy, MaxVal);
+  Res = EmitX86MinMax(CGF, ICmpInst::ICMP_SLT, {Res, MaxVec});
+  Res = EmitX86MinMax(CGF, ICmpInst::ICMP_SGT, {Res, MinVec});
----------------
Why arent' these unsigned compares for Unsigned?


================
Comment at: lib/CodeGen/CGBuiltin.cpp:8446
+  llvm::Type *VTy = llvm::VectorType::get(
+      (IsDW) ? CGF.Builder.getInt16Ty() : CGF.Builder.getInt8Ty(), NumElts);
+  return CGF.Builder.CreateTrunc(Res, VTy);
----------------
If you have the 8 or 16 selected above, you can use getIntNTy here I think.


Repository:
  rC Clang

https://reviews.llvm.org/D45720