[llvm-dev] [RFC] Refactor class hierarchy of VectorType in the IR
Philip Reames via llvm-dev
llvm-dev at lists.llvm.org
Fri May 22 16:45:52 PDT 2020
Just to chime in here, I generally agree with Eli's framing here.
Additionally, I, as an out of tree user, don't really care about what
names we end up with. Pick what makes sense for the project, we'll adapt.
Personally, I have no problem with the current state in tree. I'd prefer
a bit of migration support in the public C API, but we don't guarantee
perfect stability, so the fact that downstream users might need to
change doesn't particularly bug me.
Philip
On 5/22/20 3:34 PM, Eli Friedman via llvm-dev wrote:
>
> (reply inline)
>
> *From:* llvm-dev <llvm-dev-bounces at lists.llvm.org> *On Behalf Of *John
> McCall via llvm-dev
> *Sent:* Friday, May 22, 2020 12:59 PM
> *To:* Chris Tetreault <ctetreau at quicinc.com>
> *Cc:* llvm-dev at lists.llvm.org
> *Subject:* [EXT] Re: [llvm-dev] [RFC] Refactor class hierarchy of
> VectorType in the IR
>
> 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.
>
> I think there are two separate aspects here, that we shouldn’t mix
> together:
>
> 1. How do we get to a consistent state in-tree, in llvm-project,
> where code that requires fixed-length vectors only handles
> fixed-length vectors?
> 2. What names do we expose for out-of-tree users?
>
> If we want to simply rename the types that currently exist in-tree
> (VectorType/FixedVectorType/ScalableVectorType), we can do that
> mechanically in a few patches; we can use a few “sed” invocations,
> then clang-format the result. This would allow us to preserve the old
> meaning of the name “VectorType” for out-of-tree code. I don’t think
> this is particularly valuable; the names on trunk seem fine, and
> out-of-tree code can equally use “sed” in the opposite direction.
>
> In terms of semantic changes, I see three alternatives:
>
> 1. We continue as we are: VectorType is the base class, and we plan
> to change any code which expects a fixed-width vector to use
> FixedVectorType instead.
> 2. We “typedef VectorType BaseVectorType;”, go through and change all
> the places where we expect a BaseVectorType, then change the
> meaning of VectorType back to its original meaning of a
> fixed-width vector. I think this is problematic, though. As this
> work is in progress, it would be hard to keep track of whether
> code in the tree using the name VectorType means to use a
> FixedVectorType, or a BaseVectorType. And the patch that actually
> changes the meaning of VectorType would be a big functional change
> (even if it’s not actually a big patch).
> 3. We completely kill off uses of the name “VectorType” in-tree:
> incrementally rename every use of the name to either
> BaseVectorType or FixedVectorType.
>
> The last alternative is sort of more formal: it involves going through
> and explicitly making a choice everywhere. But I don’t think
> continuing as we are is a problem.
>
> 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()|.
>
> I had similar concerns about the “polarity” initially. But we’ve
> found that, in practice, that making the default “wrong” in IR
> optimizations has been helpful for making progress on various aspects
> in parallel. So we can take C code using intrinsics, and produce
> assembly, even though we haven’t fixed all the issues in the bits in
> between.
>
> Practically speaking, almost all places that specifically need a
> fixed-length type either call getNumElements(), or make some sort of
> query about the size of the type. So I don’t think there’s a big
> invisible tail of work even if we have some code that’s temporarily wrong.
>
> -Eli
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200522/96ffcee7/attachment.html>
More information about the llvm-dev
mailing list