When considering how to classify homogeneous aggregates as return/argument types, the ABI of the function (specified by attribute pcs) wasn't being taken into account. This resulted in some weird and unexpected hybrid assembly when compiling with softfp.
It looks like every call to getABIKind() is doing the wrong thing; I'm worried that's going to lead to some sort of subtle miscompile without some sort of centralized fix to compute the correct calling convention for every place that checks it. Not sure how to write a testcase off the top of my head... have you tried homogeneous aggregates with more than two elements?
|4 ↗||(On Diff #189714)|
This test needs "// REQUIRES: arm-registered-target" so it's automatically disabled if the ARM target isn't available.
Good catch! Seems like if we surpass "isHomogeneousAggregateSmallEnough" (4), it's no longer classified as an aggregate and so doesn't benefit from this fix. I'll see if I can work out a way to catch that as well.
It looks like every call to getABIKind() is doing the wrong thing; I'm worried that's going to lead to some sort of subtle miscompile without some sort of centralized fix to compute the correct calling convention for every place that checks it.
Agreed. It would be much better if this determined the ABI on context. That said, it doesn't look like that's going to be trivial to achieve...
I got bit a confused earlier. That does actually seem like correct behaviour (once we're no longer able to treat the struct as a homogeneous aggregate).
I've tightened up the conditional a bit as it wouldn't have previously accounted for the case where you had mfloat-abi=hard with an AAPCS (non VFP) attribute. Although in this instance, the backend does seem to be correcting/ignoring that, it's still best we don't blindly rely on such behaviour.
I've also re-structured the conditional and added some comments so it's (hopefully) a little bit clearer what's going on.
I believe this fixes the bug at hand. There's some other calls to getABIKind referencing AAPCS16_VFP, but that doesn't have a direct translation to an LLVM/FI CallingConvention (that I can see). Seems there is a lot of different ways to check various ABI/CC's and none of them seem able to give you the full picture at any point.
Looking more carefully, I guess none of the other places actually distinguish between ARMABIInfo::AAPCS_VFP and ARMABIInfo::AAPCS, so I guess the current change is sufficient given the calling-convention attributes that are likely to be used in practice.
Please pull the IsEffectivelyAAPCS_VFP out into a separate helper function on ARMABIInfo.
Done. I've added an extra parameter over what you might expect as classifyArgumentTypes doesn't seem to consider AAPCS16, whereas classifyReturnTypes does.
I've also renamed the variable to make the distinction between it and the function clearer.