Hi,
I just wanted to give an update on the progress of this work. This morning I merged a patch to add the new vector types. I have added a FixedVectorType, as proposed below. I also added a ScalableVectorType. I found during my work that it is useful to be able to query isa(Ty). Additionally, I was concerned that it would become commonplace to take (isa(Ty) && !isa(Ty)) to mean “is a scalable vector”. This is both more verbose, and not future proof if new vector types are ever added.
VectorType::get has been changed to construct a FixedVectorType if scalable = false, or a ScalableVectorType if scalable = true. It is now impossible to construct a base VectorType. Additionally, FixedVectorType::get and ScalableVectorType::get functions have been added that allow you to directly construct these types. I have also added an overload of VectorType::get that takes a type, and a vector. This overload calls getElementCount() on the given vector and uses that to construct a new VectorType. This is a convenience helper for the cases where “I want a vector with the same shape as this other vector, but a different element type.” Calls to VectorType::get(SomeTy, SomeVTy->getNumElements()) that try to implement this case are a very common source of bugs when SomeVTy is scalable, use of this helper will eliminate these bugs while also being more concise.
Currently, VectorType still has its getNumElements() function, and this function is still buggy in all the scenarios I describe below. Now that the types exist in the codebase, I will begin to remove calls to getNumElements through pointers to VectorType. My plan is to assume that all usages of getNumElements() are correct and to unconditionally cast to FixedVectorType and call it through the derived pointer. In places where this assumption is false, the code will fail to cast, rather than silently miscompile. I will fix breakages identified by existing test coverage. I also plan to make obvious fixes where value is high or the amount of work to do so is low. There are too many usage sites for one person to do this work alone. For a great many usage sites, this assumption will be correct; all backend code for architectures that don’t have scalable vectors don’t care about ScalableVectorType and many target independent optimizations can’t meaningfully do anything if they don’t know the actual number of vector lanes. Once the usages are fixed up, I’ll merge https://reviews.llvm.org/D78127 which will largely complete the refactor.
My request is that people please begin to use these types correctly. If your code needs fixed width vectors, please use the FixedVectorType directly, and somehow handle the case that a Type object is an instance of ScalableVectorType. If your code is actually generic to both types of vectors, please call getElementCount() instead of getNumElements(). And if your code only cares about scalable vectors, you can directly use ScalableVectorType. ScalableVectorType has a getMinNumElements() method that gets the Min field from the element count. This is a very minor convenience, but if you have a pointer to a ScalableVectorType, then you don’t really care about the value of getElementCount().Scalable.
Thanks,
Christopher Tetreault
Hi Chris,
this is a bit late, but I want to thank you for the work you're doing
and the careful approach you're taking, e.g. with the warning in
VectorType::getNumElements(). Once you're closer to being able to
remove the method entirely, I'd ask you to please mark it as
deprecated first. This would be a great help for those of us who track
master closely and do a lot of work upstream, but also have a frontend
that is out-of-tree -- we obviously wouldn't ask you to do the work of
removing uses of getNumElements() in our frontend, but going the
deprecation route is hopefully not a big burden and smooths the
upgrade process significantly.
Cheers,
Nicolai
Nicolai,
My plan is to remove getNumElements() as soon as possible. Hopefully within the next few weeks. I just made a patch on my machine that marks it deprecated, and it generates a ton of warnings. Given that some build bots build with -Werror, I don't think we can mark it deprecated unless all the usages are first removed.
It occurs to me now that it might be good to mark it deprecated for some period of time to be nice to downstreams. I maintain a downstream that attempts to stay up to date with master myself, and it would probably make my life easier if getNumElements didn't just suddenly go away. I think the -Werror build bots would prevent most people from re-introducing it, we would just have to be very vigilant to ensure that nobody un-deprecates it.
Since most usages of getNumElements() are either easy to fix (by casting to FixedVectorType), or likely are causing miscompilation, I would prefer to keep the amount of time it remains as short as possible. What do you think would be a reasonable period of time for it to be deprecated? 2 weeks?
I would also like to add that assuming you have integrated https://reviews.llvm.org/rG2dea3f129878e929e5d1f00b91a622eb1ec8be4e, you should probably start fixing up usages of getNumElements() now.
thanks,
Christopher Tetreault
Hi Chris,
My plan is to remove getNumElements() as soon as possible. Hopefully within the next few weeks. I just made a patch on my machine that marks it deprecated, and it generates a ton of warnings. Given that some build bots build with -Werror, I don't think we can mark it deprecated unless all the usages are first removed.
It occurs to me now that it might be good to mark it deprecated for some period of time to be nice to downstreams. I maintain a downstream that attempts to stay up to date with master myself, and it would probably make my life easier if getNumElements didn't just suddenly go away. I think the -Werror build bots would prevent most people from re-introducing it, we would just have to be very vigilant to ensure that nobody un-deprecates it.
Thanks!
Since most usages of getNumElements() are either easy to fix (by casting to FixedVectorType), or likely are causing miscompilation, I would prefer to keep the amount of time it remains as short as possible. What do you think would be a reasonable period of time for it to be deprecated? 2 weeks?
I would also like to add that assuming you have integrated rG2dea3f129878, you should probably start fixing up usages of getNumElements() now.
Yes indeed. And two weeks should be plenty at least for us, since we
usually integrate multiple times a day (with the occasional hiccups
due to breakage, hence my interest in keeping the breakage low :)).
Cheers,
Nicolai
OK. When I finish merging patches to remove calls to VectorType::getNumElements(), I'll merge a patch that marks it deprecated. Two weeks after that patch is merged, I'll merge the patch to remove the function.
Thanks,
Christopher Tetreault