[PATCH] D46018: [GlobalISel][IRTranslator] Split aggregates during IR translation

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added a comment.

Hi Amara,

I think I'm about half-way through, didn't touch the `{pack,unpack}Regs` functions and the whole story with interfacing with legacy call-lowering yet.

This is great work, thank you for doing this!

Roman



================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:121
+      computeValueLLTs(DL, **EI, ValueTys, Offsets,
+                       StartingOffset + SL->getElementOffset(EI - EB));
+    return;
----------------
Maybe this way it's just a bit more straightforward:
```
  for (unsigned I = 0, E = STy->getNumElements(); I != E; ++I)
    computeValueLLTs(DL, *STy->getElementType(I), ValueTys, Offsets,
                     StartingOffset + SL->getElementOffset(I));
```
It's alright as it is, of course, just in case you would like the version above better yourself.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:128
+    uint64_t EltSize = DL.getTypeAllocSize(EltTy);
+    for (unsigned i = 0, e = ATy->getNumElements(); i != e; ++i)
+      computeValueLLTs(DL, *EltTy, ValueTys, Offsets,
----------------
I think we're trying to stick to capitalized local variables' names, including loop induction variables.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:157
+  SmallVector<LLT, 4> SplitTys;
+  computeValueLLTs(*DL, *Val.getType(), SplitTys, *Offsets);
+
----------------
Looks like technically for aggregate constants `getOrCreateVRegs` has worst case complexity of `O(N^2)`, for constants like `{ i8 0, { i8 0, { i8 0, ... } } }`, but this is probably not easy to avoid and such constants hopefully don't happen often.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:173
+    }
+  } else {
+    assert(SplitTys.size() == 1 && "unexpectedly split LLT");
----------------
This piece in general is not exactly trivial, perhaps it makes sense to add test cases having multiple deeply nested aggregate constants sharing some parts so `getOrCreateVRegs` would visit an actual non-tree DAG while traversing them.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:176
+    VRegs->push_back(MRI->createGenericVirtualRegister(SplitTys[0]));
+    bool Success = translate(cast<Constant>(Val), (*VRegs)[0]);
     if (!Success) {
----------------
`VRegs->front()` maybe


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:474
+  for (auto DstReg : getOrCreateVRegs(U))
+    MIRBuilder.buildCopy(DstReg, SrcRegs[Idx++]);
 
----------------
Same note as for `insertvalue`, do we really need to build these copies here?


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:508
+    else
+      MIRBuilder.buildCopy(DstRegs[i], SrcRegs[i]);
+  }
----------------
Let's say we have a large aggregate that eventually maps to N vregs, and also N `insertvalue`s each of which replaces just a single item (vreg). Will we end up generating N^2 `COPY`s for the IR of initial size `O(N)` here?

Granted, this probably happens very rarely in practice, but what if we don't `getOrCreateVRegs(U)`, but rather just `get(U)` (no implicit `MIR.createGenericRegister` calls), resize, and assign either `*InsertedIt++` or `SrcRegs[i]` to it directly w/o building any explicit `COPY`s at all? We're just remapping values back and forth here during translation, with always constant indices, so the whole thing could be copy-propagated on the fly avoiding quite a bit of MIR-churn.


Repository:
  rL LLVM

https://reviews.llvm.org/D46018