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

[Fraser Cormack via Phabricator via llvm-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