[PATCH] D49660: [GlobalISel] Rewrite CallLowering::lowerReturn to accept multiple VRegs per Value

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added inline comments.


================
Comment at: lib/Target/AArch64/AArch64CallLowering.cpp:248
+    SmallVector<EVT, 4> SplitEVTs;
+    ComputeValueVTs(TLI, DL, Val->getType(), SplitEVTs);
+    assert(VRegs.size() == SplitEVTs.size() &&
----------------
Hi Alexander,

Thanks for doing this!

+ @aemerson

It feels a little unnatural to get all the EVTs from an IR Type and then map them back  to IR Types (see `SplitEVTs[i].getTypeForEVT(Ctx)` below) just to map them back to EVTs again (see `splitToValueTypes`s implementation. It would later on map them back to Type again (and finally to LLT) if I'm not mistaken).
It looks like the only functionality of `ComputeValueVTs` we are actually interested in here specifically is flattening the aggregate IR Types.

In fact, there is a very similar function `computeValueLLTs` in IRTranslator that computes LLTs from a Value Type by flattening the aggregates. It also dictates the size of the `VRegs` that is asserted below.
`computeValueLLTs`: https://github.com/llvm-mirror/llvm/blob/a145774c76ef75206356f92dd4c1b2c0d8dea896/lib/CodeGen/GlobalISel/IRTranslator.cpp#L112-L140
Compare with `ComputeValueVTs`:  https://github.com/llvm-mirror/llvm/blob/a145774c76ef75206356f92dd4c1b2c0d8dea896/lib/CodeGen/Analysis.cpp#L77-L115

What I would suggest is to see if we can extract a similar flattening function (if it doesn't already exist somewhere else) that outputs IR Types (and offsets) instead of EVTs or LLTs, implement `computeValueLLTs ` and `ComputeValueVTs` via that function (by post-processing the results with `getLLTForType` and `TLI.getValueType(DL, Ty)` respectively), but use it directly here. I think that way we could have:
1. No Type -> EVT -> Type -> EVT -> Type -> LLT conversion, just Type -> EVT -> Type -> LLT (not sure if it could be even further reduced to Type -> LLT as `TargetLoweringBase::getValueType` seems to be doing stuff like replacing pointer types with integers, and it's unclear to me if it's required in GlobalISel's context given we have pointer LLT types)
2. Code deduplication
3. Simpler `*CallLowering::lowerReturn`s

Otherwise I think this chain of type conversions gets a little too long to follow.

What do you think? That could be a separate patch of course.


================
Comment at: lib/Target/X86/X86CallLowering.cpp:210
+      setArgFlags(CurArgInfo, AttributeList::ReturnIndex, DL, F);
+      if (!splitToValueTypes(CurArgInfo, SplitArgs, DL, MRI,
+                             [&](ArrayRef<unsigned> Regs) {
----------------
It would also be nice if we could reduce the amount of repetition from target to target if it's possible w/o introducing weird APIs. This could be a separate follow-up patch of course.


https://reviews.llvm.org/D49660