[PATCH] D77272: Clean up usages of asserting vector getters in Type

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau marked an inline comment as done.
ctetreau added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:85
   unsigned getBroadcastShuffleOverhead(Type *Ty) {
-    assert(Ty->isVectorTy() && "Can only shuffle vectors");
+    auto *VTy = dyn_cast<VectorType>(Ty);
+    assert(VTy && "Can only shuffle vectors");
----------------
sdesmalen wrote:
> ctetreau wrote:
> > sdesmalen wrote:
> > > Can we just use `cast<>` and get rid of the separate assert below? We'd lose the message, but I'm not sure how useful that is, given that the context makes it quite obvious.
> > I spoke to this a bit in D77259.
> > 
> > Realistically, for this example, in debug you'll get a slightly more readable assert message. In release, you'll get a null dereference in getVectorInstrCost vs (I assume) a template error in the cast. From a debugging perspective, the second situation is almost certainly preferable, but just doing the cast will be better for performance. I'll fix it.
> Thanks! In this case the only difference for a release build would be that `VTy` would be `nullptr` when using `dyn_cast`, and would be `Ty` when using `cast`. I don't think the use of `cast` gives a template error.
I guess in a just world, you'd get a "types unrelated" error. But since it just does a c-style cast, it's probably just UB.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77272