[PATCH] D102852: [RISCV] Fix a crash when lowering split float arguments

[Luís Marques via Phabricator via llvm-commits]

luismarques added a comment.

In D102852#2824822 <https://reviews.llvm.org/D102852#2824822>, @frasercrmck wrote:

> My understanding was that this would be a permanent change. Take all of the following with a grain of salt as you no doubt understand the psABI better than I do. I still have misgivings about whether this is the correct thing to do.
>
> I suppose the ultimate problem is that vectors aren't really specified by the psABI as far as I can tell, so we don't have a concrete model to follow. So as long we're correctly handling other types I think we're okay.
>
> The problem is that as far as LLVM's concerned, illegal vector types are "split". "Split" arguments in our backend are definitely encoding some assumptions which, when I authored this patch, I assumed were only for scalar integers. At the least it does seem to assume types which have been assigned integer locations. The psABI doesn't mention anything in the "Hardware Floating-Point Calling Convention" section about passing floating-point types indirectly. However, if the floating-point argument is passed soft, or variadic, or once other float arguments are exhausted, we do fall into the integer handling, at which point these sections cover us on "split" arguments:
>
>   Scalars that are 2×XLEN bits wide are passed in a pair of argument registers, with the low-order XLEN bits in the lower-numbered register and the high-order XLEN bits in the higher-numbered register. If no argument registers are available, the scalar is passed on the stack by value. If exactly one register is available, the low-order XLEN bits are passed in the register and the high-order XLEN bits are passed on the stack.
>   
>   Scalars wider than 2×XLEN are passed by reference and are replaced in the argument list with the address.
>
> Thing is, I can't work out how we would get to either of the code paths above (where we have a split type and push it into `PendingLocs`) when dealing with anything //other// than a scalar integer type. Floating-point type are all handled earlier by a combination of hard/float ABIs and assumptions. We certainly don't have any tests that cover it.
>
> If you pass `double` on `riscv32` (which is the first thing I thought of) you get `2*XLEN` //scalar integer// locations so that's unchanged. `double` on `riscv32 +d` is specially handled and early-returned (`// Handle passing f64 on RV32D with a soft float ABI or when floating point registers are exhausted.`). And `double` on `risv32` with a float ABI is passed in a register, or else through the previous case.
>
> I agree the code looks odd in that it doesn't align to what the psABI prescribes, but after having explained this it does look okay.

You're right that, when it comes to vectors, we're generally out of what the psABI specifies. But, if anything, I would say that this falls within the "Aggregates larger than 2×XLEN bits are passed by reference and are replaced in the argument list with the address". So I'm not particularly convinced that this is the correct long-term solution, as before we were doing exactly that and now we're not.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102852/new/

https://reviews.llvm.org/D102852