[PATCH] D99590: [Clang] Do not use memcpy for scalable struct copy.

[Hsiangkai Wang via Phabricator via cfe-commits]

HsiangKai updated this revision to Diff 334579.
HsiangKai added a comment.

Address comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99590

Files:
  clang/include/clang/AST/Type.h
  clang/lib/AST/Type.cpp
  clang/lib/CodeGen/CGExprAgg.cpp


Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -2077,6 +2077,23 @@
 }
   }
 
+  // Aggregate assignment turns into element-by-element copy.
+  if (const RecordType *RecordTy = Ty->getAs()) {
+if (RecordTy->hasSizelessFields()) {
+  RecordDecl *Record = RecordTy->getDecl();
+  llvm::Value *SrcAgg = Builder.CreateLoad(SrcPtr);
+  llvm::Value *DestAgg = llvm::UndefValue::get(ConvertType(Ty));
+  unsigned FieldCount =
+  getContext().getASTRecordLayout(Record).getFieldCount();
+  for (unsigned I = 0; I < FieldCount; ++I) {
+llvm::Value *Vec = Builder.CreateExtractValue(SrcAgg, I);
+DestAgg = Builder.CreateInsertValue(DestAgg, Vec, I);
+  }
+  Builder.CreateStore(DestAgg, DestPtr);
+  return;
+}
+  }
+
   // Aggregate assignment turns into llvm.memcpy.  This is almost valid per
   // C99 6.5.16.1p3, which states "If the value being stored in an object is
   // read from another object that overlaps in anyway the storage of the first
Index: clang/lib/AST/Type.cpp
===
--- clang/lib/AST/Type.cpp
+++ clang/lib/AST/Type.cpp
@@ -3510,6 +3510,28 @@
   return false;
 }
 
+bool RecordType::hasSizelessFields() const {
+  std::vector RecordTypeList;
+  RecordTypeList.push_back(this);
+  unsigned NextToCheckIndex = 0;
+
+  while (RecordTypeList.size() > NextToCheckIndex) {
+for (FieldDecl *FD :
+ RecordTypeList[NextToCheckIndex]->getDecl()->fields()) {
+  QualType FieldTy = FD->getType();
+  if (FieldTy.getTypePtr()->isSizelessType())
+return true;
+  FieldTy = FieldTy.getCanonicalType();
+  if (const auto *FieldRecTy = FieldTy->getAs()) {
+if (llvm::find(RecordTypeList, FieldRecTy) == RecordTypeList.end())
+  RecordTypeList.push_back(FieldRecTy);
+  }
+}
+++NextToCheckIndex;
+  }
+  return false;
+}
+
 bool AttributedType::isQualifier() const {
   // FIXME: Generate this with TableGen.
   switch (getAttrKind()) {
Index: clang/include/clang/AST/Type.h
===
--- clang/include/clang/AST/Type.h
+++ clang/include/clang/AST/Type.h
@@ -4623,6 +4623,10 @@
   /// is declared const, return true. Otherwise, return false.
   bool hasConstFields() const;
 
+  /// Recursively check all fields in the record for sizeless. If any field
+  /// is a sizeless type, return true. Otherwise, return false.
+  bool hasSizelessFields() const;
+
   bool isSugared() const { return false; }
   QualType desugar() const { return QualType(this, 0); }
 


Index: clang/lib/CodeGen/CGExprAgg.cpp
===
--- clang/lib/CodeGen/CGExprAgg.cpp
+++ clang/lib/CodeGen/CGExprAgg.cpp
@@ -2077,6 +2077,23 @@
 }
   }
 
+  // Aggregate assignment turns into element-by-element copy.
+  if (const RecordType *RecordTy = Ty->getAs()) {
+if (RecordTy->hasSizelessFields()) {
+  RecordDecl *Record = RecordTy->getDecl();
+  llvm::Value *SrcAgg = Builder.CreateLoad(SrcPtr);
+  llvm::Value *DestAgg = llvm::UndefValue::get(ConvertType(Ty));
+  unsigned FieldCount =
+  getContext().getASTRecordLayout(Record).getFieldCount();
+  for (unsigned I = 0; I < FieldCount; ++I) {
+llvm::Value *Vec = Builder.CreateExtractValue(SrcAgg, I);
+DestAgg = Builder.CreateInsertValue(DestAgg, Vec, I);
+  }
+  Builder.CreateStore(DestAgg, DestPtr);
+  return;
+}
+  }
+
   // Aggregate assignment turns into llvm.memcpy.  This is almost valid per
   // C99 6.5.16.1p3, which states "If the value being stored in an object is
   // read from another object that overlaps in anyway the storage of the first
Index: clang/lib/AST/Type.cpp
===
--- clang/lib/AST/Type.cpp
+++ clang/lib/AST/Type.cpp
@@ -3510,6 +3510,28 @@
   return false;
 }
 
+bool RecordType::hasSizelessFields() const {
+  std::vector RecordTypeList;
+  RecordTypeList.push_back(this);
+  unsigned NextToCheckIndex = 0;
+
+  while (RecordTypeList.size() > NextToCheckIndex) {
+for (FieldDecl *FD :
+ RecordTypeList[NextToCheckIndex]->getDecl()->fields()) {
+  QualType FieldTy = FD->getType();
+  if (FieldTy.getTypePtr()->isSizelessType())
+return true;
+  FieldTy = FieldTy.getCanonicalType();
+  if (const auto *FieldRecTy = FieldTy->getAs()) {
+if (llvm::find(RecordTypeList, FieldRecTy) == RecordTypeList.

[PATCH] D99590: [Clang] Do not use memcpy for scalable struct copy.

[Fraser Cormack via Phabricator via cfe-commits]

frasercrmck added inline comments.



Comment at: clang/include/clang/AST/Type.h:4627
+  /// Recursively check all fields in the record for sizeless. If any field
+  /// is sizeless type, return true. Otherwise, return false.
+  bool hasSizelessFields() const;

nit: missing "a": `is a sizeless type`



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:2085
+  llvm::Value *SrcVec = Builder.CreateLoad(SrcPtr);
+  llvm::Value *DestVec = llvm::UndefValue::get(ConvertType(Ty));
+  llvm::Value *Vec;

Are `SrcVec` and `DestVec` the right names here? It took me a while to realise 
they're not vector types.



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:2086
+  llvm::Value *DestVec = llvm::UndefValue::get(ConvertType(Ty));
+  llvm::Value *Vec;
+  for (unsigned I = 0;

We could define `Vec` inside the loop?



Comment at: clang/lib/CodeGen/CGExprAgg.cpp:2088
+  for (unsigned I = 0;
+   I < getContext().getASTRecordLayout(Record).getFieldCount(); ++I) {
+Vec = Builder.CreateExtractValue(SrcVec, I);

I don't know if `getContext().getASTRecordLayout(Record).getFieldCount()` is 
expensive to compute, but this may be recomputing it on every iteration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99590

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits