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

Thu Jun 17 08:50:43 PDT 2021

frasercrmck added a reviewer: kito-cheng.
frasercrmck added a comment.

In D102852#2824377 <https://reviews.llvm.org/D102852#2824377>, @luismarques wrote:

> The last time I looked at this I had some concerns but I didn't have time to fully investigate them.
> To help get this going again, could you please clarify:
>
> - Is this patch and/or the ABI changes supposed to be (roughly) permanent or just a temporary fix/workaround?
> - Can you provide more details about the "The register arguments seem to occupy unused stack slots, which I've observed in other vector tests"?

Thanks for taking a look. To quickly address the second part, I was a bit presumptuous. I was observing the above, e.g. the `<32 x float> being split into:

  ; CHECK-NEXT:    fmv.w.x fa0, a0
  ; CHECK-NEXT:    fmv.w.x fa1, a1
  ; CHECK-NEXT:    fmv.w.x fa2, a2
  ; CHECK-NEXT:    fmv.w.x fa3, a3
  ; CHECK-NEXT:    fmv.w.x fa4, a4
  ; CHECK-NEXT:    flw ft0, 64(sp)
  ; CHECK-NEXT:    flw ft1, 68(sp)
  ...

We (may) never access `0-63(sp)` so to a casual observer it seems wasteful to use the stack for those parts of the split value that go into registers `a0-a4`. But thinking about it further I now know (believe) that this is required in case the function takes the address of its argument. There needs to be a "full" copy of the argument somewhere in memory. I believe I would edit that out of the summary now.

As for your first question...

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.

I'll add @kito-cheng here since they'll presumably know enough about the psABI to help in case I've missed something.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102852