[PATCH] D96854: [CodeExtractor] Enable partial aggregate arguments

[Giorgis Georgakoudis via Phabricator via llvm-commits]

ggeorgakoudis marked 2 inline comments as done.
ggeorgakoudis added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:833
   std::vector<Type *> paramTy;
+  std::vector<Type *> ScalarParamTy;
+  std::vector<Type *> AggParamTy;
----------------
vsk wrote:
> I notice there's a `paramTy = ScalarParamTy`: can we delete `paramTy` entirely? Having two copies of the same thing creates a risk of the copies diverging, adding complexity.
In `paramTy` we concatenate the scalar parameter types and the aggregate type. The comment on diverging copies makes sense. I'll change `paramTy` to `ParamTy`, remove `ScalarParamTy`, and directly push non-aggregate arguments and concat the aggregate type.


================
Comment at: llvm/lib/Transforms/Utils/CodeExtractor.cpp:861
+      "Number of scalar and aggregate params does not match inputs, outputs");
+
+  paramTy = ScalarParamTy;
----------------
vsk wrote:
> Suggest - `assert(StructValues.empty() || AggregateArgs && "StructValues updated for arg structs only")`
Makes sense, will add the check


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96854