Page MenuHomePhabricator

Clean up usages of asserting vector getters in Type
ClosedPublic

Authored by ctetreau on Apr 1 2020, 4:47 PM.

Details

Summary

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.

Diff Detail

Event Timeline

ctetreau created this revision.Apr 1 2020, 4:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2020, 4:47 PM
ctetreau updated this revision to Diff 254589.Apr 2 2020, 12:08 PM

Update to mention that this change is NFC

ctetreau updated this revision to Diff 255825.Apr 7 2020, 2:53 PM

update commit message, rebase

sdesmalen added inline comments.Apr 10 2020, 7:11 AM
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.
A quick scan through the codebase leads me to believe that Tp is always a VectorType.

2869

cast<VectorType>(ValTy) could also fit into a separate variable. I expect ValTy is always of type VectorType.

ctetreau updated this revision to Diff 256600.Apr 10 2020, 10:10 AM

rebase, address code review issues

ctetreau updated this revision to Diff 256606.Apr 10 2020, 10:37 AM
ctetreau marked an inline comment as done.

add FIXME

efriedma added inline comments.Apr 10 2020, 10:52 AM
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.

ctetreau updated this revision to Diff 256667.Apr 10 2020, 2:21 PM

address code review issues

ctetreau marked 2 inline comments as done.Apr 13 2020, 9:58 AM
ctetreau added inline comments.
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.

ctetreau marked an inline comment as done.Apr 13 2020, 9:58 AM
RKSimon added inline comments.Apr 13 2020, 10:18 AM
llvm/lib/Target/X86/X86TargetTransformInfo.cpp
2869

In which case, please can you raise a bug listing the functions that need changing to VectorType?

ctetreau marked an inline comment as done.Apr 13 2020, 12:48 PM
ctetreau added inline comments.
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.

ctetreau marked an inline comment as done.Apr 14 2020, 9:18 AM
ctetreau marked an inline comment as done.Apr 14 2020, 1:03 PM
ctetreau added inline comments.
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.

ctetreau marked an inline comment as done.Apr 15 2020, 2:04 PM
ctetreau added inline comments.
llvm/lib/Target/X86/X86TargetTransformInfo.cpp
2869

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 revision is now accepted and ready to land.Apr 16 2020, 3:59 PM
This revision was automatically updated to reflect the committed changes.