This is an archive of the discontinued LLVM Phabricator instance.

Handle floating-point type homogeneous aggregate return values in ABISysV_arm
ClosedPublic

Authored by omjavaid on Feb 7 2016, 12:57 PM.

Details

Summary

Arm hard float ABI can use floating point registers for returning structures containing all 4 or 8 byte floating point elements.

Arm ABI documentation call such structs a Homogeneous Aggregate, which is a Composite Type where all of the Fundamental Data Types that compose the type
are the same.

With Arm Hard float ABI a Homogeneous Aggregate with a Base Type of a single- or double-precision floating-point type with one to four Elements can be returned using register s0-s3 and d0-d3.

This patch updates ABISysV_arm::GetReturnValueObjectImpl to handle these cases.

ReturnValueTestCase passes on armhf based linux target after applying this patch.

Diff Detail

Event Timeline

omjavaid updated this revision to Diff 47152.Feb 7 2016, 12:57 PM
omjavaid retitled this revision from to Handle floating-point type homogeneous aggregate return values in ABISysV_arm.
omjavaid updated this object.
omjavaid added reviewers: tberghammer, clayborg.
omjavaid added a subscriber: lldb-commits.
labath added a subscriber: labath.Feb 8 2016, 2:00 AM

Do we have a test for this?

If not, it sounds like it would be an easy thing to add one.

labath added a comment.Feb 8 2016, 2:01 AM

Nevermind, I now see you mention TestReturnValue. :/

tberghammer added inline comments.Feb 8 2016, 5:23 AM
source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
581–666

I think if we are returning an aggregate containing 1 32 bit float (byte_size == 4) then it will be returned in s0 while your current implementation expect it to be returned in r0. I am not sure about it but please take a look.

If this is the case I would suggest to order the conditions the following way to decrees the nesting level of the ifs:

if (IsArmHardFloat(thread))
{
    ...
}
else if (byte_size <= 4)
{
    ....
}
673

What is float_count means here? Do we have to check its value to decide if we can print the return value (e.g. what happens when float_count == 2)?

688–695

I would suggest to use the dwarf register numbers instead of the register names for the lookup:

uint32_t regnum = 0;
if (byte_size == 4)
    regnum = dwarf_s0 + reg_index;
else if (base_byte_size == 8)
    regnum = dwarf_d0 + reg_index;
else
    break;
const RegisterInfo *reg_info = reg_ctx->GetRegisterInfoBy(eRegisterKindDWARF, regnum);
omjavaid added inline comments.Feb 8 2016, 6:38 AM
source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
581–666

Yes, you are right. I ll rearrange the code accordingly.

673

float_count is just used to fullfil argument requirements. It was already declared in the function so didnt have to define it here.

It returns 1 if type is a standard builtin type (float or double), returns 2 for complex and number of elements for vector types.

We are using homogeneous_count to tell the number of elements in our aggregate type.

688–695

Alright I ll make the appropriate change.

tberghammer added inline comments.Feb 8 2016, 6:41 AM
source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
673

In that case I think we should check that its value after the call is 1 because we don't want to use the current implementation for vector types (we already check for complex types)

omjavaid updated this revision to Diff 47191.Feb 8 2016, 6:54 AM

updated diff after incorporating suggested corrections.

tberghammer added inline comments.Feb 8 2016, 7:34 AM
source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
592

Please check that "float_count == 1" as well (we don't want to use this code path for vector types)

592

What the ABI says about homogeneous complex types? The current implementation treats them as in-memory return types what I think is not the right thing to do (I am happy with saying we don't support it but then we should return an empty object)

636

If we failed to read out all data (this condition is false) then we will fall through to the non hard float case and will try to read out the return value as an in-memory return value. I think this isn't what we want to do based on the ABI.

omjavaid updated this revision to Diff 47198.Feb 8 2016, 7:36 AM

Updated adding float_count check.

clayborg resigned from this revision.Feb 8 2016, 11:34 AM
clayborg removed a reviewer: clayborg.

I am fine as long as tberghammer is happy.

Is this good to go ?

tberghammer edited edge metadata.Feb 11 2016, 2:36 AM

Can you take a look for the 2 marked comment? I think you handling those error scenarios incorrectly

source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
592

?

636

?

658

I think this should be an "else if" to fix the issue I mentioned in line 636

omjavaid updated this revision to Diff 47625.Feb 11 2016, 3:36 AM
omjavaid edited edge metadata.

I have added an else case for unhandled or error cases.
I have added to Todo for handling complex and vector types. ABI document doesnt say much about how complex will be returned so I am doing a bit investigation with how they are being managed by compiler. I will handle them both in next patch.

omjavaid added inline comments.Feb 11 2016, 3:37 AM
source/Plugins/ABI/SysV-arm/ABISysV_arm.cpp
658

Homogenous types with elements more than 4 will fall through and will be handled by this even in case of hardfloat ABI so we cannot make it an elseif here.

tberghammer accepted this revision.Feb 11 2016, 3:38 AM
tberghammer edited edge metadata.

Looks good, thank you for fixing it

This revision is now accepted and ready to land.Feb 11 2016, 3:38 AM
This revision was automatically updated to reflect the committed changes.