[PATCH] D77264: 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/lib/Target/X86/X86TargetTransformInfo.cpp:2760
+  // following. In practice, can this actually happen?
+  auto *ValVTy = dyn_cast<VectorType>(ValTy);
+
----------------
ctetreau wrote:
> RKSimon wrote:
> > ctetreau wrote:
> > > efriedma wrote:
> > > > The type has to be a vector: it's the type of a vector reduction operation.  You can just cast<> here.
> > > > 
> > > > It would probably be worth going through at some point and making the various TTI cost hooks that require a vector take a VectorType instead of a Type.  But that's out of scope here, of course.
> > > That is the endgame that I'm hoping for, but probably not something I can personally justify doing on the company's dime.
> > In which case, please can you raise a bug listing the functions that need changing to VectorType?
> This issue is pervasive throughout the codebase. From what I have seen, most functions that deal in Type objects take and return pointers to the base Type class. There are many functions that immediately cast to VectorType or assert isVectorTy(), but there are many more that logically should only deal in VectorType, but instead operate on the base Type (either by casting to VectorType only in certain branches or by passing these pointers to other functions that logically only work on VectorType) Even within this module, there are many such functions. Some are obvious, many are not.
> 
> It would be a tremendous amount of work to even analyze which functions should operate on on Type subclasses rather than base objects. I do not have the bandwidth to do this analysis, and a bug that basically says "There are functions in [module] that should take VectorType instead of Type as an argument" is not a helpful bug.
> 
> These functions that I am removing are serving as a crutch for this problem. My hope is that forcing the cast will cause more code to just cast once, or take derived objects as function arguments.
efriedma and I discussed it offline, and he agrees that at least opening the "TargetTransformInfoImpl.h has a bunch of functions that take and return base type that should take and return a vector type" bug is a useful thing to do. This file is a particularly egregious offender on this regard.

I'm in the process of getting an account on bugzilla. I will open the bug when that is completed. In the mean time, I would appreciate if we could get this patch merged as it is blocking my work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77264