Page MenuHomePhabricator

Remove SequentialType from the type heirarchy.
ClosedPublic

Authored by efriedma on Mar 4 2020, 6:49 PM.

Details

Summary

Now that we have scalable vectors, there's a distinction that isn't getting captured in the original SequentialType: some vectors don't have a known element count, so counting the number of elements doesn't make sense.

In some cases, there's a better way to express the commonality using other methods. If we're dealing with GEPs, there's GEP methods; if we're dealing with a ConstantDataSequential, we can query its element type directly.

In the relatively few remaining cases, I just decided to write out the type checks. We're talking about relatively few places, and I think the abstraction doesn't really carry its weight. (See thread "[RFC] Refactor class hierarchy of VectorType in the IR" on llvmdev.)

Diff Detail

Event Timeline

efriedma created this revision.Mar 4 2020, 6:49 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 4 2020, 6:49 PM
huihuiz added a subscriber: huihuiz.Mar 5 2020, 1:32 PM
efriedma updated this revision to Diff 250910.Mar 17 2020, 2:40 PM

Rebase. Fix a test failure. A few other minor tweaks.

efriedma marked an inline comment as done.Mar 17 2020, 2:45 PM

I looked at getting rid of getSequentialNumElements/getSequentialElementType, but I'm not sure that would actually make the code more clear. I avoided them in cases where they clearly weren't helpful.

clang/lib/CodeGen/CGDecl.cpp
1053 ↗(On Diff #250910)

Despite the appearance, this doesn't actually affect the result for vectors: we try to recursively rewrite the elements, but we don't actually support any rewrites for types other than StructType and ArrayType, so the rewrite just produces the input constant.

efriedma updated this revision to Diff 250928.Mar 17 2020, 3:37 PM

Minor fix to AMDGPU changes.

The AMDGPU changes LGTM, modulo a stylistic nitpick.

As for the overall change, I'm perfectly fine with the general direction of SequentialType, though I don't know where we stand in terms of enough people having had a chance to look at it.

I'm concerned though about having getSequentialElementType and getSequentialNumElements apply to different sets of types. That's bound to lead to confusion which causes bugs. Can we have one name for all types that are homogenous sequences of the same element type, and a different name for all types which are *fixed-length* homogenous sequences?

llvm/lib/Target/AMDGPU/AMDGPUPromoteAlloca.cpp
370–372

Please put braces for the outer if (multi-line body).

I guess the API names could be a bit confusing, yes. Any suggestions for better names?

I'm concerned though about having getSequentialElementType and getSequentialNumElements apply to different sets of types. That's bound to lead to confusion which causes bugs. Can we have one name for all types that are homogenous sequences of the same element type, and a different name for all types which are *fixed-length* homogenous sequences?

isSequentialType() returns true for VectorType and ArrayType. It should be changed to only return true if the vector type is not scalable, and getSequentialNumElements() should assert isSequentialType() before attempting the casts.

In the future, when my proposed FixedVectorType is added, isSequentialType will return true for ArrayType and FixedVectorType only.

efriedma updated this revision to Diff 251218.Mar 18 2020, 4:56 PM

I went through the exercise of completely removing the notion of a "sequential" type; the result comes out something like this.

Broadly, the places that might be able to use some notion of "sequential" mostly fall into two categories:

  1. Places where we're examining indexes of a GEP.
  2. Constants of array/vector type.

For GEPs, I think using GEP-specific API makes sense, to make it clear what notion of "element" we're considering. For the other cases, not really sure if an abstraction would be helpful.

sdesmalen accepted this revision.Mar 19 2020, 3:40 PM

Thank you for this patch! Personally I find the code more readable without the SequentialType abstraction and the use of the GEP interface (getTypeAtIndex(Type, Idx)) you added in D75660.
Like @nhaehnle I don't know whether this patch needs some more eyes before you commit this, but it looks good to me!

llvm/lib/Transforms/IPO/GlobalOpt.cpp
442

nit: With only two uses, I'm not sure these functions make the code much more readable than just expanding it in their uses below.

This revision is now accepted and ready to land.Mar 19 2020, 3:40 PM
ctetreau added inline comments.Mar 20 2020, 8:38 AM
clang/lib/CodeGen/CGDecl.cpp
1059 ↗(On Diff #251218)

I assume the reasoning here is that [-0.0f, ...] isn't the zero initialized value, but why make this change now? Seems unrelated.

1078 ↗(On Diff #251218)

The fact that you have to ask this question tells me that you should probably just make this handle vectors.

You could add a templated helper function above this function that is basically just the original body of the SequentialType branch.

efriedma marked 2 inline comments as done.Mar 20 2020, 10:40 AM
efriedma added inline comments.
clang/lib/CodeGen/CGDecl.cpp
1059 ↗(On Diff #251218)

I'll post this separately for review, since it's a little complicated.

1078 ↗(On Diff #251218)

Well, no, the original code doesn't handle vectors either. The issue here would be that we need to pad out the vector with additional elements, or something like that.

ctetreau added inline comments.Mar 20 2020, 11:20 AM
clang/lib/CodeGen/CGDecl.cpp
1078 ↗(On Diff #251218)

At a cursory glance, it seemed to me that the addition of this comment implied that it used to handle the tail padding. However, after having dug into what this function does, assuming that vectors only ever actually contain scalars, then it seems that this does currently just return the original vector and that this patch does not change the behavior.

Given this, your comment really kind of just says "FIXME: is this function correct?", a question that applies to basically every function. If you have reason to believe that the answer to your question is yes, you should probably add why you think so to this comment. Otherwise, I think you should remove it.

efriedma updated this revision to Diff 252432.Mar 24 2020, 2:55 PM
efriedma edited the summary of this revision. (Show Details)

Rebased. (CGDecl.cpp changes landed separately.)

I'd like the see the clang-format complaints actually addressed, but don't bother unless you need to upload another patch for some other reason anyways.

That aside, LGTM. This has been looked at by 3 people (plus yourself). So far, nobody has had any serious objections. Perhaps enough has been enough?

efriedma updated this revision to Diff 252645.Mar 25 2020, 12:13 PM

Address lint comments

efriedma updated this revision to Diff 252659.Mar 25 2020, 1:31 PM

Fix bug in GlobalOpt. Add fixes for MLIR.

ctetreau added inline comments.Mar 25 2020, 2:00 PM
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
81–89

Readability: This works, but is a little strange. I'd rewrite:

static llvm::Type *getInnermostElementType(llvm::Type *type) {
  while (isa<llvm::ArrayType>(type) || isa<llvm::VectorType>(type)) {
    if (auto *arrayTy = dyn_cast<llvm::ArrayType>(type))
      type = arrayTy->getElementType();
    else
      type = cast<llvm::VectorType>(type)->getElementType();
  }
  return type;
}

What's the status on this?

This revision was automatically updated to reflect the committed changes.
ftynse added a subscriber: ftynse.Apr 7 2020, 1:59 AM

LGTM for MLIR part.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
60

Nit: can we update the error message not to mention "sequential LLVM type" anymore if sequential type is no longer a thing?

efriedma marked an inline comment as done.Apr 7 2020, 10:40 AM
efriedma added inline comments.
mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
60

Other MLIR code in this file still refers to them as sequential types, but I guess it might make sense to change in this context.

Is it realistically possible to hit this path? I'm not sure how to write a testcase.