[llvm-dev] [RFC] Refactor class hierarchy of VectorType in the IR

[John McCall via llvm-dev]

On 22 May 2020, at 3:15, Chris Tetreault wrote:
> John,
>
>    For the last several months, those of us working on the scalable 
> vectors feature have been examining the codebase, identifying places 
> where llvm::VectorType is used incorrectly, and fixing them. The fact 
> is that there are many places where VectorType is correctly taken to 
> be the generic “any vector” type. getNumElements may be being 
> called, but it’s being called in accordance with the previously 
> documented semantics. There are many places where it isn’t as well, 
> and many people add new usages that are incorrect.
>
>    This puts us in an unfortunate situation: if we were to take your 
> proposal and have VectorType be the fixed width vector type, then all 
> of this work is undone. Every place that has been fixed up to 
> correctly have VectorType be used as a universal vector type will now 
> incorrectly have the fixed width vector type being used as the 
> universal vector type. Since VectorType will inherit from 
> BaseVectorType, it will have inherited the getElementCount(), so the 
> compiler will happily continue to compile this code. However, none of 
> this code will even work with scalable vectors because the bool will 
> always be false. There will be no compile time indication that this is 
> going on, functions will just start mysteriously returning nullptr. 
> Earlier this afternoon, I set about seeing how much work it would be 
> to change the type names as you have suggested. I do not see any way 
> forward other than painstakingly auditing the code.

If you define `getElementCount() = delete` in `VectorType`, you can 
easily find the places that are doing this and update them to use 
`VectorBaseType`.  You wouldn’t actually check that in, of course; 
it’s a tool for doing the audit in a way that’s no more painstaking 
than what you’re already doing with `getNumElements()`.  And in the 
meantime, the code that you haven’t audited — the code that’s 
currently unconditionally calling `getNumElements()` on a `VectorType` 
— will just conservatively not trigger on scalable vectors, which for 
most of LLVM is a better result than crashing if a scalable vector comes 
through until your audit gets around to updating it.

>    Additionally, for those who disagree that the LLVM developer policy 
> is to disregard the needs of downstream codebases when making changes 
> to upstream, I submit that not throwing away months of work by 
> everybody working to fix the codebase to handle scalable vectors 
> represents a real expected benefit. I personally have been spending 
> most of my time on this since January.

I’m responding to this as soon as I heard about it.  I’ll accept 
that ideally I would have seen it when you raised the RFC in March, 
although in practice it’s quite hard to proactively keep up with 
llvmdev, and as a community I think we really need to figure out a 
better process for IR design.  I’m not going to feel guilty about work 
you did for over a month without raising an RFC.  And I really don’t 
think you have in any way wasted your time; I am asking for a large but 
fairly mechanical change to the code you’ve already been updating.

But most of your arguments are not based on how much work you’ve done 
on your current audit, they’re based on the fact that scalable vectors 
were initially implemented as a flag on `VectorType`.  So part of my 
problem here is that you’re basically arguing that, as soon as that 
was accepted, the generalization of `VectorType` was irreversible; and 
that’s a real problem, because it’s very common for early prototype 
work to not worry much about representations, and so they stumble into 
this kind of problematic representation.

My concern is really only ~50% that this is going to force a lot of 
unnecessary mechanical changes for downstream projects and 50% that 
generalizing `VectorType` to include scalable vectors, as the initial 
prototype did, is the wrong polarity and makes a lot of existing code 
broken if it ever sees a scalable vector.  Your hierarchy change only 
solves this in the specific case that there’s an immediate call to 
`getNumElements()`.

Of course, if the community generally disagrees with me that this is 
necessary, I’ll accept that.  But right now I’m just hearing from 
people that are part of your project.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200522/12d86847/attachment.html>