This is an archive of the discontinued LLVM Phabricator instance.

[MIPS GlobalISel] Lower aggregate structure return arguments
ClosedPublic

Authored by Petar.Avramovic on Sep 24 2019, 6:53 AM.

Details

Summary

Implement aggregate structure split to simpler types in splitToValueTypes.
splitToValueTypes is used for return values.
According to MipsABIInfo from clang/lib/CodeGen/TargetInfo.cpp,
aggregate structure arguments for O32 always get simplified and thus
will remain unsupported by the MIPS GlobalISel for the time being.
For O32, aggregate structures can be encountered only for complex number
returns e.g. 'complex float' or 'complex double' from <complex.h>.

Diff Detail

Repository
rL LLVM

Event Timeline

atanasyan accepted this revision.Sep 24 2019, 8:52 AM

LGTM
It looks like the tests check calling the splitToValueTypes from the MipsCallLowering::lowerReturn only. If it's true, could you add more tests to cover passing arguments?

This revision is now accepted and ready to land.Sep 24 2019, 8:52 AM
Petar.Avramovic marked 2 inline comments as done.Sep 24 2019, 9:21 AM

It looks like the tests check calling the splitToValueTypes from the MipsCallLowering::lowerReturn only.

Right. I will add tests for call of a function that returns aggregate structure.
Now that I think of it, I didn't encounter any aggregate structure as an argument in test-suite.
They get turned into inreg arguments like in this tests,
maybe we could mark aggregate structure as unsupported if they are arguments for now
and only allow them if they are return values?

lib/Target/Mips/MipsCallLowering.cpp
466 ↗(On Diff #221536)

If yes, then we could also inline old splitToValueTypes here.

586 ↗(On Diff #221536)

and here.

It looks like the tests check calling the splitToValueTypes from the MipsCallLowering::lowerReturn only.

Right. I will add tests for call of a function that returns aggregate structure.
Now that I think of it, I didn't encounter any aggregate structure as an argument in test-suite.
They get turned into inreg arguments like in this tests,

I think the following code is affected by the change in MipsCallLowering::lowerFormalArguments.

define { double, double } @bar({ double, double } %s) {
entry:
  %retval = alloca { double, double }, align 8
  %inval = alloca { double, double }, align 8
  store { double, double } %s, { double, double }* %inval, align 8
  %0 = bitcast { double, double }* %retval to i8*
  %1 = bitcast { double, double }* %inval to i8*
  call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 8 %0, i8* align 8 %1, i32 16, i1 false)
  %2 = load { double, double }, { double, double }* %retval, align 8
  ret { double, double } %2
}

declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture writeonly, i8* nocapture readonly, i32, i1)

I do not know how to get such LLVM-IR code from, for example, C code. In Clang the MipsABIInfo::classifyArgumentType coerces an aggregate argument into series of i32 arguments. But if your patch generates a correct code for aggregate arguments (I didn't check) let's add a test case and keep the patch as-is.

I also looked into MipsABIInfo today and it doesn't allow any aggregate structure like argument for O32, and only aggregate structure that can be encountered is return value that has ComplexType. Even structure with two floats or doubles returns get transformed into pointer argument despite the fact it is similar thing as complex float or complex double.
I am not so sure about correctness for aggregate struct arguments since I can't generate them with clang, and for that reason I would mark aggregate structures arguments as unsupported for now, and only handle aggregate structure returns. When test with aggregate structure arguments appears we can add handling for it.

Petar.Avramovic edited the summary of this revision. (Show Details)

Aggregate structure arguments remain unsupported like before this patch.
Support aggregate returns and update tests.

This revision was automatically updated to reflect the committed changes.