This is logical continuation of https://reviews.llvm.org/D46018 (r332449)
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/Target/AArch64/AArch64CallLowering.cpp | ||
---|---|---|
248 ↗ | (On Diff #156734) | Hi Alexander, Thanks for doing this! 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). 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. 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:
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. |
lib/Target/X86/X86CallLowering.cpp | ||
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. |
lib/Target/AArch64/AArch64CallLowering.cpp | ||
---|---|---|
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.
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?
Thanks,
Roman
Hi Roman, I would prefer to do it as separate patches: the first one for Value splitting routine and the second for one splitToValueTypes