[PATCH] D100684: [X86][CostModel] X86TTIImpl::getMemoryOpCost(): rewrite vector handling again

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

This looks like it needs further cleanup tbh - I've made a few comments but there's more.



================
Comment at: llvm/lib/Target/X86/X86TargetTransformInfo.cpp:3245
+
+  const int EltTyBits = DL.getTypeSizeInBits(VTy->getElementType());
+  assert(EltTyBits > 0 && "Sizeless type?");
----------------
Repeated VTy->getElementType() ?


================
Comment at: llvm/lib/Target/X86/X86TargetTransformInfo.cpp:3247
+  assert(EltTyBits > 0 && "Sizeless type?");
+  assert(EltTyBits % 8 == 0 && "Non-byte-sized type?");
+  const int EltTyBytes = EltTyBits / 8;
----------------
Merge these
```
assert((EltTyBits > 0) && ((EltTyBits % 8) == 0) && "Expected byte-size types");
```


================
Comment at: llvm/lib/Target/X86/X86TargetTransformInfo.cpp:3249
+  const int EltTyBytes = EltTyBits / 8;
+  assert(EltTyBytes != 0);
+
----------------
Can this assert happen?


================
Comment at: llvm/lib/Target/X86/X86TargetTransformInfo.cpp:3258
+  int NumEltRemaining = SrcNumElt;
+  auto NumEltDone = [&]() { return SrcNumElt - NumEltRemaining; };
+
----------------
This feels like it should be unnecessary.....


================
Comment at: llvm/lib/Target/X86/X86TargetTransformInfo.cpp:3262
+  const int MaxLegalOpSizeBytes = LT.second.getSizeInBits() / 8;
+  assert(MaxLegalOpSizeBytes != 0);
+
----------------
assert message?


================
Comment at: llvm/lib/Target/X86/X86TargetTransformInfo.cpp:3316
+        if (!Is0thSubVec)
+          Cost += getShuffleCost(Opcode == Instruction::Load
+                                     ? TTI::ShuffleKind::SK_InsertSubvector
----------------
maybe pull out "bool IsLoad = Opcode == Instruction::Load;" style bools?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100684