This is an archive of the discontinued LLVM Phabricator instance.

[1/11][IR] Permit load/store/alloca for struct of the same scalable vector type
ClosedPublic

Authored by eopXD on Mar 25 2023, 7:40 AM.

Details

Summary

This patch-set aims to simplify the existing RVV segment load/store
intrinsics to use a type that represents a tuple of vectors instead.

To achieve this, first we need to relax the current limitation for an
aggregate type to be a target of load/store/alloca when the aggregate
type contains homogeneous scalable vector types. Then to adjust the
prolog of an LLVM function during lowering to clang. Finally we
re-define the RVV segment load/store intrinsics to use the tuple types.

The pull request under the RVV intrinsic specification is
riscv-non-isa/rvv-intrinsic-doc#198


This is the 1st patch of the patch-set. This patch is originated from
D98169.

This patch allows aggregate type (StructType) that contains homogeneous
scalable vector types to be a target of load/store/alloca. The RFC of
this patch was posted in LLVM Discourse.

https://discourse.llvm.org/t/rfc-ir-permit-load-store-alloca-for-struct-of-the-same-scalable-vector-type/69527

The main changes in this patch are:

Extend StructLayout::StructSize from uint64_t to TypeSize to
accommodate an expression of scalable size.

Allow StructType:isSized to also return true for homogeneous
scalable vector types.

Let Type::isScalableTy return true when Type is StructType
and contains scalable vectors

Extra description is added in the LLVM Language Reference Manual on the
relaxation of this patch.

Authored-by: Hsiangkai Wang <kai.wang@sifive.com>
Co-Authored-by: eop Chen <eop.chen@sifive.com>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
eopXD retitled this revision from [WIP][1/N][IR] Permit load/store/alloca for struct of the same scalable vector type to [1/N][IR] Permit load/store/alloca for struct of the same scalable vector type.Mar 25 2023, 8:00 AM
nikic requested changes to this revision.Mar 25 2023, 8:20 AM
nikic added a subscriber: nikic.

isSized() should return true for these structs. They are sized for our definition of sized.

Is there an RFC for this on discourse?

This revision now requires changes to proceed.Mar 25 2023, 8:20 AM
eopXD added a comment.Mar 26 2023, 7:02 AM

Is there an RFC for this on discourse?

Hi @nikic,

Thanks for the swift comment, just opened up an RFC (https://discourse.llvm.org/t/rfc-ir-permit-load-store-alloca-for-struct-of-the-same-scalable-vector-type/69527).
Hope this time we can explore a way to have RVV tuple types.

Regards,

eop Chen

I think you also need to update LangRef.rst at least this sentence : https://github.com/llvm/llvm-project/blob/main/llvm/docs/LangRef.rst#L738

:ref:`Scalable vectors <t_vector>` cannot be global variables or members of
arrays because their size is unknown at compile time. They are allowed in
structs to facilitate intrinsics returning multiple values. Structs containing
scalable vectors cannot be used in loads, stores, allocas, or GEPs.
craig.topper added inline comments.Mar 30 2023, 9:22 AM
llvm/include/llvm/IR/DataLayout.h
636

This is a weird sentence. A "caller" is a function in the code base, it doesn't have a size.

"Should only be called for structs with scalable size"
llvm/include/llvm/IR/DerivedTypes.h
300

Missing period at end of sentence

301

isContainHomogeneousScalableVectorType -> containsHomogenousScalableVectorTypes.

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
6966 ↗(On Diff #508307)

Why is this needed?

llvm/lib/IR/DataLayout.cpp
51

You already checked containsScalableVectorType in the if above. You don't need to check it in the ?: too

63–69

Use "homogenous scalable vector types" here instead of "tuple types"

llvm/lib/IR/Type.cpp
634

As @nikic said. We need to return true for homogenous scalable vector structs. I put this check in when I allowed structs containing scalable vectors for intrinsic return values. See https://reviews.llvm.org/D94142

eopXD updated this revision to Diff 511066.Apr 5 2023, 5:38 AM
eopXD marked 7 inline comments as done.

Changes:

  • Address comments from Craig
    • Let isSize return true for homogeneous scalable vector structs.
    • Function rename isContainHomogeneousScalableVectorType -> containsHomogenousScalableVectorTypes
  • Fix return value for DataLayout::getTypeSizeInBits
  • Let Type::isScalableTy return true for StructType with scalable vectors
eopXD added a comment.Apr 5 2023, 5:39 AM

Also addressed comments from Kito:

  • Added description in Language Reference Manual for the relaxation added in this patch.
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
6966 ↗(On Diff #508307)

No this is not needed here. The original intent was to return base when a scalable structure was queried here.

nikic requested changes to this revision.Apr 5 2023, 5:46 AM

Generally, please re-review the whole patch and remove special cases that are no longer needed now that isSized() returns the correct value.

llvm/include/llvm/IR/DataLayout.h
630

Why not make these methods return TypeSize instead (and drop the separate getMinSize APIs)?

675

Assertion can be restored.

llvm/lib/AsmParser/LLParser.cpp
7627

These changes should no longer be necessary now that isSized() is fixed.

llvm/lib/IR/Type.cpp
628

Debugging leftover.

This revision now requires changes to proceed.Apr 5 2023, 5:46 AM
eopXD updated this revision to Diff 511072.Apr 5 2023, 5:50 AM

Change: Remove conditions of checking containsHomogeneousScalableVectorTypes since now isSized admits homogeneous scalable vector structs

eopXD marked an inline comment as done.Apr 5 2023, 5:50 AM
eopXD marked an inline comment as done.
eopXD updated this revision to Diff 511079.Apr 5 2023, 6:20 AM

Changes:

  • Address comments from Nikic
    • Restore assertion
    • Return TypeSize for getSizeInBytes and getSizeInBits.
  • Fix check under containsHomogeneousScalableVectorTypes
eopXD marked 2 inline comments as done.Apr 5 2023, 6:21 AM
eopXD edited the summary of this revision. (Show Details)Apr 5 2023, 6:38 AM
eopXD updated this revision to Diff 511086.Apr 5 2023, 6:53 AM
eopXD edited the summary of this revision. (Show Details)

Prune unnecessary changes.

eopXD updated this revision to Diff 511088.Apr 5 2023, 6:56 AM

Recover original code under Verifier.cpp.

craig.topper added inline comments.Apr 7 2023, 10:49 AM
llvm/docs/LangRef.rst
747–754

Need to work the exception into this sentence. Right now its an absolute statement and I wouldn't expect an exception if I didn't read the next paragraph.

May add "with one exception described below" to the end of the sentence?

756

Don't capitalize "Structs".

759

Don't capitalize "Structs"

10296

Maybe "Structs containing scalable vectors cannot be used in allocas unless all fields are the same scalable vector type"?

10297

Same

llvm/include/llvm/CodeGen/Analysis.h
75

Why the last field called Offset here but StartingOffset in the other two signatures?

llvm/lib/CodeGen/Analysis.cpp
96

TypeSize::get(0, StartingOffset.isScalable())

135

TypeSize::get(Offset, Ty->isScalableTy())

186

This assert contradicts the comment on the next line. Scalable vectors are fine if Offsets is null

llvm/lib/CodeGen/CodeGenPrepare.cpp
7684 ↗(On Diff #511088)

Is this information we can get from DL.getTypeSize? Not sure when StructLayout is created and whether that would be cheaper than walking every field of a struct looking for scalable vectors.

llvm/lib/IR/DataLayout.cpp
94

The Caller is not a structure.

eopXD updated this revision to Diff 511887.Apr 8 2023, 7:12 AM
eopXD marked 11 inline comments as done.

Address comments from Craig. Comment resolution are described under the comment threads.

llvm/docs/LangRef.rst
747–754

Merged the exception into the same paragraph so users can be aware of this.

756

The existing paragraph uses "Structs" instead of "structs".

Structs containing scalable vectors cannot be used in loads, stores, allocas, or GEPs.

Un-capitalized it.

llvm/lib/CodeGen/Analysis.cpp
186

Removed assertion.

llvm/lib/IR/DataLayout.cpp
94

Caller under SROA is ArrayType.
Caller under ConsantFolding, ModuleSummaryAnalysis, TypeMetadataUtils is ConstantStruct.
Caller under Target is StructType, but by grep-ing I don't find user of the function LLVMElementAtOffset.

I say it is safe to remove the assertion here.

craig.topper added inline comments.Apr 8 2023, 10:19 AM
llvm/lib/IR/DataLayout.cpp
94

I was referring to a word usage problem. The word caller refers to the function that contains the code calls this method on an object. The caller is not the same thing as the object this method acts on.

eopXD updated this revision to Diff 511907.Apr 8 2023, 10:36 AM
eopXD marked an inline comment as done.

Recover assertion and adjust wording.

llvm/lib/IR/DataLayout.cpp
94

Recovered the method and adjusted the wording.

eopXD retitled this revision from [1/N][IR] Permit load/store/alloca for struct of the same scalable vector type to [1/11][IR] Permit load/store/alloca for struct of the same scalable vector type.Apr 10 2023, 12:45 AM
eopXD edited the summary of this revision. (Show Details)
eopXD updated this revision to Diff 512139.Apr 10 2023, 7:04 AM

Resolve test failure - LLVM.tools/llvm-as::slow-ptrtoint.ll

  • Add memoize when containsHomogeneousScalableVectorTypes() == true under StructType::isSized.
  • Add memoize for StructType::containsScalableVectorType.
nikic added inline comments.Apr 13 2023, 1:12 AM
llvm/docs/LangRef.rst
752

The types should probably be wrapped in double backticks.

llvm/include/llvm/IR/DerivedTypes.h
223

NotContain

301

definition of

llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
160

Use isScalableTy()?

llvm/lib/IR/DataLayout.cpp
63–69

double "be the"

llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
527

Can this be an assertion? As this is related to GEP handling.

eopXD updated this revision to Diff 513214.Apr 13 2023, 6:44 AM
eopXD marked 5 inline comments as done.

Address comments from Nikic.

nikic added a comment.Apr 13 2023, 1:02 PM

It looks like there is a test failure in sme-aarch64-svcount.ll now.

We should also have a verifier test that checks that these are rejected in GEPs. We should also test the InstCombine bailouts (I think you can just take your "Other" tests and move them to InstCombine and run -passes=instcombine instead of -passes=verify. Those should cover the relevant code paths.)

craig.topper added inline comments.Apr 13 2023, 7:53 PM
llvm/docs/LangRef.rst
753

"can used" -> "can be used"

llvm/lib/IR/Type.cpp
472

If this structure is opaque, elements will be empty and we would set the SCDB_NotContainScalableVector flag. That seems wrong since it could gain scalable vector types if it becomes non-opaque.

eopXD updated this revision to Diff 513454.EditedApr 14 2023, 12:09 AM
eopXD marked 3 inline comments as done.

Address comments from Nikic and Craig.

  • Bail out when visiting extractvalue under InstructionCombining.cpp, added test case llvm/test/Transforms/InstCombine/scalable-vector-struct-extractvalue.ll
  • Check for GEP with scalable vector under LLParser.cpp, added test case llvm/test/Verifier/scalable-vector-struct-gep.ll
  • Removed assertion in GetElementPtrInst::GetElementPtrInst under Instructions.h, the verifier will take case of this check.
  • Fix grammatical error under LangRef.rst.
  • Adjust test case sme-aarch64-svcount.ll based on change under FunctionLoweringInfo.cpp
llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
160

Changing this to checking Ty->isScalableTy() results in changes the code generation of llvm/test/CodeGen/AArch64/sme-aarch64-svcount.ll. The change makes sense to me, but the code generation is worse here, @sdesmalen may you confirm if this is reasonable?

nikic added inline comments.Apr 14 2023, 12:57 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
2732

This looks too aggressive -- is it enough to only guard the LoadInst code below?

eopXD marked an inline comment as done.Apr 14 2023, 1:05 AM
eopXD updated this revision to Diff 513468.Apr 14 2023, 1:05 AM

Apply Nikic's suggestion to bail out when aggregate type contains scalable vector type and used in a load instruction.

eopXD updated this revision to Diff 513625.Apr 14 2023, 9:14 AM

Adjust checking sequence under LLParser.cpp to resolve test case failure for LLVM.Assembler::unsized-recursive-type.ll.

craig.topper added inline comments.Apr 17 2023, 12:31 PM
llvm/include/llvm/IR/DerivedTypes.h
222

Contain->Contains

llvm/lib/IR/Type.cpp
487

is -> are

eopXD updated this revision to Diff 514512.Apr 17 2023, 8:41 PM

Address comments from Craig.

eopXD updated this revision to Diff 514513.Apr 17 2023, 8:43 PM
eopXD marked 2 inline comments as done.

Minor change.

eopXD added a comment.Apr 24 2023, 3:20 AM

Ping, thank you.

eopXD added a comment.May 2 2023, 1:26 AM

Ping again, thank you.

craig.topper added inline comments.May 3 2023, 4:49 PM
llvm/lib/Analysis/ScalarEvolution.cpp
3768

Maybe just check in getOffsetOfExpr where we implicitly cast getElementOffset to uint64_t? We just need to make sure the TypeSize from getElementOffset isn't scalable.

llvm/lib/IR/DataLayout.cpp
49

Would StructSize be default constructed to TypeSize::Fixed(0) anyway?

50–51

Since we only support homogenous scalable structs. I wonder if we could changes StructSize to scalable on the first iteration of the below loop if the first type comes back scalable? That would save us needing to walk the entire struct hierachy separately here.

Though it gets memoized so maybe not a big deal.

94

"Cannot get element at offset for structure containing scalable vector types"

llvm/lib/IR/Type.cpp
640

If the element is a struct containing scalable vectors, should we no longer be considered sized? That would mean we have a nested struct.

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
768

Could we check the size from the SL below is not scalable?

1295

Same question

llvm/lib/Transforms/Scalar/SROA.cpp
3979

Can we check the size of the StructLayout?

llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp
526

"GEPs are not supported on structures containing scalable vectors"

eopXD updated this revision to Diff 519494.May 4 2023, 7:55 AM
eopXD marked 8 inline comments as done.

Address comments from Craig.

llvm/lib/Analysis/ScalarEvolution.cpp
3768

Moved the assertion to check assert(!SL->getSizeInBits().isScalable() under ScalarEvolution::getOffsetOfExpr.

llvm/lib/IR/DataLayout.cpp
49

The ctors of TypeSize are:

constexpr TypeSize(ScalarTy Quantity, bool Scalable)
    : FixedOrScalableQuantity(Quantity, Scalable) {}

static constexpr TypeSize getFixed(ScalarTy ExactSize) {
  return TypeSize(ExactSize, false);
}
static constexpr TypeSize getScalable(ScalarTy MinimumSize) {
  return TypeSize(MinimumSize, true);
}
static constexpr TypeSize get(ScalarTy Quantity, bool Scalable) {
  return TypeSize(Quantity, Scalable);
}
static constexpr TypeSize Fixed(ScalarTy ExactSize) {
  return TypeSize(ExactSize, false);
}
static constexpr TypeSize Scalable(ScalarTy MinimumSize) {
  return TypeSize(MinimumSize, true);
}

Am I using it incorrectly here?

50–51

Though calling Type::isScalableTy still results in calling containsScalableVectorType, adjusted the code as requested anyway.

llvm/lib/IR/Type.cpp
640

Good point. Nested structs containing scalable vector will not be consider sized. Added extra check to guard this.

craig.topper added inline comments.May 4 2023, 11:16 AM
llvm/lib/IR/DataLayout.cpp
49

You're right. There is no default constructor. Though it's base class has one.

llvm/lib/IR/Type.cpp
639

Could we call isScalableTy instead of isa<ScalableVectorType>(Ty) above?

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
767

Does SL->getSizeInBits().isScalable() below make this unnecessary?

eopXD marked 4 inline comments as done.May 5 2023, 1:12 AM
eopXD added inline comments.
llvm/lib/IR/DataLayout.cpp
49

Marking this thread as done.

llvm/lib/IR/Type.cpp
639

Yes you are correct. Corrected the code.

llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
767

Sorry, forgot to delete this one. Deleted.

eopXD updated this revision to Diff 519762.May 5 2023, 1:12 AM
eopXD marked 3 inline comments as done.

Address comments from Craig.

craig.topper added inline comments.May 5 2023, 10:03 AM
llvm/lib/IR/Type.cpp
641

Doesn't checking isScalableTy() make this unnecessary?

eopXD updated this revision to Diff 520267.May 7 2023, 11:57 PM
eopXD marked an inline comment as done.

Address comments from Craig, also removed infinite loop bug in containsScalableVectorType.

llvm/lib/IR/Type.cpp
641

Yes you are correct. Furthermore calling isScalableTy uncovered a bug that I forgot to record visited Type that would result in infinite loop case for recursively-defined types.

@nikic Is there any more concern on landing this?

eopXD updated this revision to Diff 522414.May 15 2023, 8:05 PM

Rebase to latest main.

nikic added inline comments.May 16 2023, 5:45 AM
llvm/docs/LangRef.rst
750

contains -> contain

752

kind -> kinds

llvm/include/llvm/IR/DerivedTypes.h
223

Contain -> Contains

294

a false -> false

llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
155–158

structure -> structs
vector -> vectors

llvm/lib/IR/Type.cpp
626

This has no relation to opaque types.

llvm/test/Other/load-scalable-vector-struct.ll
2 ↗(On Diff #522414)

Move these two tests into llvm/test/Assembler and combine them.

llvm/test/Transforms/InstCombine/scalable-vector-struct-extractvalue.ll
2 ↗(On Diff #522414)

You've added three bailouts in InstCombine -- does this test case cover all three of them?

Also, don't we need a test case for the check in SROA as well?

eopXD updated this revision to Diff 522897.May 16 2023, 10:39 PM
eopXD marked 8 inline comments as done.

Address comments from Nikic.

  • Fixed grammatical errors.
  • Merged llvm/test/Other/store-scalable-vector-struct.ll and llvm/test/Other/load-scalable-vector-struct.ll into llvm/test/Assembler/scalable-vector-struct.ll
  • Added store situation test case for llvm/test/Transforms/InstCombine/scalable-vector-struct.ll. The two test case should cover the three checks under load/store/extractvalue.
  • Added test case llvm/test/Transforms/SROA/scalable-vector-struct.ll
nikic accepted this revision.May 17 2023, 12:36 PM

LGTM

This revision is now accepted and ready to land.May 17 2023, 12:36 PM
eopXD edited the summary of this revision. (Show Details)May 17 2023, 10:25 PM
eopXD edited the summary of this revision. (Show Details)
eopXD added a comment.May 19 2023, 9:40 AM

Thank you Nikic and Craig for the patient reviewals :)

zixuan-wu added inline comments.
llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
160

Changing this to checking Ty->isScalableTy() results in changes the code generation of llvm/test/CodeGen/AArch64/sme-aarch64-svcount.ll. The change makes sense to me, but the code generation is worse here, @sdesmalen may you confirm if this is reasonable?

Hi. As Ty->isScalableTy() contains target scalable type, it extends the semantic. So it's not only for scalable vector type. I think we can pass the ty as argument to distinguish them in target TFI, then getStackIDForScalableVectors would be better to be named 'getStackIDForScalableTy'

eopXD added inline comments.May 22 2023, 10:51 PM
llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
160

Thank you for checking this out. I will come back to revisit this when I finished implementing the other RVV intrinsic features for v1.0.

paulwalker-arm added inline comments.
llvm/test/CodeGen/RISCV/rvv/alloca-load-store-scalable-struct.ll
18–19

Apologies for any naivety here but is the code generation here correct? I'm trying to figure out the patch's objective because it looks like it has enable support within the IR but ignored code generation. My interpretation of this output is that whilst it correctly allocates the vscale relative stack space:

csrrs a2, vlenb, zero
slli a2, a2, 1
sub sp, sp, a2

it then simply falls back to fixed length offsets when accessing it:

addi a2, a0, 8
vl1re64.v v8, (a2)

Is my interpretation here correct? or have I misunderstood something?

paulwalker-arm added inline comments.Aug 11 2023, 7:49 AM
llvm/test/CodeGen/RISCV/rvv/alloca-load-store-scalable-struct.ll
18–19

I've pushed a test for SVE under https://reviews.llvm.org/rGac2a7637fe74 which shows similar behaviour. So now I'm pretty sure this is just broken. I'm working on a fix that I'll post later, probably Monday.