Page MenuHomePhabricator

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

Authored by aivchenk on Jul 23 2018, 3:30 AM.

Diff Detail


Event Timeline

aivchenk created this revision.Jul 23 2018, 3:30 AM
rtereshin added inline comments.Jul 26 2018, 12:35 PM
248 ↗(On Diff #156734)

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 splitToValueTypess 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.
Compare with ComputeValueVTs:

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::lowerReturns

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.

210 ↗(On Diff #156734)

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.

aivchenk added inline comments.Jul 27 2018, 7:32 AM
248 ↗(On Diff #156734)

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

Thanks for taking this on. I think some x86 tests would also be good here as there evidently isn't much coverage at the moment.

aivchenk updated this revision to Diff 158518.Aug 1 2018, 6:13 AM

Thanks for your comments. Indeed the struct handling is not very well covered with tests in x86. I added the tests for cases, which are supported now

Hi Alexander,

Do you want to do the refactoring within this patch or as a separate one?


Hi Roman, I would prefer to do it as separate patches: the first one for Value splitting routine and the second for one splitToValueTypes

rtereshin accepted this revision.Aug 1 2018, 5:12 PM

Hi Roman, I would prefer to do it as separate patches: the first one for Value splitting routine and the second for one splitToValueTypes

Great, thanks!

This revision is now accepted and ready to land.Aug 1 2018, 5:12 PM
This revision was automatically updated to reflect the committed changes.