Remove usages of asserting vector getters in Type in preparation for the
VectorType refactor. The existence of these functions complicates the
refactor while adding little value.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Target/X86/X86TargetTransformInfo.cpp | ||
---|---|---|
973 | This code calls cast<VectorType>(Tp) several times, so it's probably worth putting that in a separate VectorType *VTp. | |
2869 | cast<VectorType>(ValTy) could also fit into a separate variable. I expect ValTy is always of type VectorType. |
llvm/lib/Target/X86/X86TargetTransformInfo.cpp | ||
---|---|---|
2869 | 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. |
llvm/lib/Target/X86/X86TargetTransformInfo.cpp | ||
---|---|---|
2869 | That is the endgame that I'm hoping for, but probably not something I can personally justify doing on the company's dime. |
llvm/lib/Target/X86/X86TargetTransformInfo.cpp | ||
---|---|---|
2869 | In which case, please can you raise a bug listing the functions that need changing to VectorType? |
llvm/lib/Target/X86/X86TargetTransformInfo.cpp | ||
---|---|---|
2869 | 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. |
llvm/lib/Target/X86/X86TargetTransformInfo.cpp | ||
---|---|---|
2869 | 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. |
llvm/lib/Target/X86/X86TargetTransformInfo.cpp | ||
---|---|---|
2869 | I've opened https://bugs.llvm.org/show_bug.cgi?id=45562 for this issue |
Would it be possible to get this code reviewed soon please? This is the last commit that needs to be merged before the commit that removes the getters can be merged.
This code calls cast<VectorType>(Tp) several times, so it's probably worth putting that in a separate VectorType *VTp.
A quick scan through the codebase leads me to believe that Tp is always a VectorType.