[PATCH] D146872: [1/N][IR] Permit load/store/alloca for struct of the same scalable vector type

[Yueh-Ting (eop) Chen via Phabricator via llvm-commits]

eopXD 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.
 
----------------
craig.topper wrote:
> 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?
Merged the exception into the same paragraph so users can be aware of this.


================
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>,
----------------
craig.topper wrote:
> Don't capitalize "Structs".
The existing paragraph uses "Structs" instead of "structs".

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

Un-capitalized it.


================
Comment at: llvm/lib/CodeGen/Analysis.cpp:192
   if (StructType *STy = dyn_cast<StructType>(&Ty)) {
+    assert(!STy->containsScalableVectorType() &&
+           "Unexpect scalable struct type");
----------------
craig.topper wrote:
> This assert contradicts the comment on the next line. Scalable vectors are fine if Offsets is null
Removed assertion.


================
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);
----------------
craig.topper wrote:
> The Caller is not a structure.
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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146872/new/

https://reviews.llvm.org/D146872