[PATCH] D146872: [1/N][IR] Permit load/store/alloca for struct of the same scalable vector type
Craig Topper via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 7 10:50:08 PDT 2023
craig.topper added inline comments.
================
Comment at: llvm/docs/LangRef.rst:741
structs to facilitate intrinsics returning multiple values. Structs containing
scalable vectors cannot be used in loads, stores, allocas, or GEPs.
----------------
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?
================
Comment at: llvm/docs/LangRef.rst:743
+The only exception to the above restriction is for Structs that contains
+scalable vectors of the same type (e.g. {<vscale x 2 x i32>,
----------------
Don't capitalize "Structs".
================
Comment at: llvm/docs/LangRef.rst:746
+<vscale x 2 x i32>} contains the same type while {<vscale x 2 x i32>,
+<vscale x 2 x i64>} doesn't). These kind of Structs are considered sized.
+
----------------
Don't capitalize "Structs"
================
Comment at: llvm/docs/LangRef.rst:10255
+Structs containing scalable vectors cannot be used in allocas. The only
+exception to the above restriction is for Structs that contains scalable
----------------
Maybe "Structs containing scalable vectors cannot be used in allocas unless all fields are the same scalable vector type"?
================
Comment at: llvm/docs/LangRef.rst:10256
+Structs containing scalable vectors cannot be used in allocas. The only
+exception to the above restriction is for Structs that contains scalable
+vectors of the same type (e.g. {<vscale x 2 x i32>, <vscale x 2 x i32>}
----------------
Same
================
Comment at: llvm/include/llvm/CodeGen/Analysis.h:75
+ SmallVectorImpl<EVT> &ValueVTs,
+ SmallVectorImpl<uint64_t> *FixedOffsets, uint64_t Offset);
----------------
Why the last field called `Offset` here but `StartingOffset` in the other two signatures?
================
Comment at: llvm/lib/CodeGen/Analysis.cpp:96
+ TypeSize EltOffset = SL ? SL->getElementOffset(EI - EB)
+ : StartingOffset.isScalable() ? TypeSize::Scalable(0)
+ : TypeSize::Fixed(0);
----------------
`TypeSize::get(0, StartingOffset.isScalable())`
================
Comment at: llvm/lib/CodeGen/Analysis.cpp:136
+ TypeSize StartingOffset =
+ Ty->isScalableTy() ? TypeSize::Scalable(Offset) : TypeSize::Fixed(Offset);
+ return ComputeValueVTs(TLI, DL, Ty, ValueVTs, Offsets, StartingOffset);
----------------
TypeSize::get(Offset, Ty->isScalableTy())
================
Comment at: llvm/lib/CodeGen/Analysis.cpp:192
if (StructType *STy = dyn_cast<StructType>(&Ty)) {
+ assert(!STy->containsScalableVectorType() &&
+ "Unexpect scalable struct type");
----------------
This assert contradicts the comment on the next line. Scalable vectors are fine if Offsets is null
================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:7684
+ if (isa<StructType>(StoreType) &&
+ cast<StructType>(StoreType)->containsScalableVectorType())
----------------
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.
================
Comment at: llvm/lib/IR/DataLayout.cpp:94
+ assert(!StructSize.isScalable() &&
+ "Caller of this should not be a structure with scalable type");
+ TypeSize Offset = TypeSize::Fixed(FixedOffset);
----------------
The Caller is not a structure.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146872/new/
https://reviews.llvm.org/D146872
More information about the llvm-commits
mailing list