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

[Alexander Ivchenko via Phabricator via llvm-commits]

aivchenk 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() &&
----------------
rtereshin wrote:
> 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.
Hello Roman,

First of all, thank you for your review!

Yes, I've seen the similarity between computeValueLLTs  and ComputeValueVTs. I tried several things first like moving computeValueLLTs  into GlobalISel/Utils.h and have it return another pointer of SmallVectorImpl<Type*>, but it didn't look good. I haven't looked at where we can introduce that generic base functionality, but this is something that I can take a look for sure.

I think that another semi-major refactoring that needs to be done - and it goes to the point that you made below - is to change splitToValueTypes. AFAIU, we don't really need to do ComputeValueVTs there, because compound types are already split at this point (at least for lowerReturn), so we only need to have the "second part" : handling how we treat individual parts e.g. with TLI.functionArgumentNeedsConsecutiveRegisters or TLI.getNumRegisters. I can take a look at it as well





https://reviews.llvm.org/D49660