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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

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

> Sorry if I'm misunderstanding your point, but we *are* accessing `0-63(sp)` (later below).

Yeah sorry I misread the assembly somehow. I thought that the lowering of `%A` in the non-fastcc `@caller` was passing the first 8 elements in `a0-a7` but were shadowed by stack locations 0-63, which felt wasteful. I thought that because the first load was from `64(sp)` onwards. Looking again I now see that this isn't the case because I forgot to notice that the sp is being decremented by 64 first :(. So `64(sp)` is indeed the 9th element. I'll remove that bit from the commit summary now. However, this may be irrelevant given the discussion below:

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

> 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.

I agree that if vectors are aggregates then this isn't doing the right thing. However, I couldn't find a concrete definition of "aggregates" as per the psABI. The System V ABI is a bit clearer in saying "Aggregates (structures and arrays)"  which I //don't// think covers vectors.

I suppose it may be wise to take a step back in case we're on the wrong track. These vector types we're dealing with are those that are passed by value in the LLVM IR. The frontend never actually generates this code. It would generate `define float @foo(<32 x float>* nocapture readonly %0)` for the equivalent C code:

  typedef float float32 __attribute__((ext_vector_type(32)));
  
  extern float ext(float32);
  
  float foo(float32 v) {
    return ext(v);
  }

So I realise I brought it up but is the psABI applicable for by-value vector types in IR? I believe @kito-cheng might have had the opinion that it was effectively a compiler-internal ABI in one of the recent LLVM calls.

If we do want vectors (without the V extension) to behave like the equivalently-sized aggregates in the psABI then we'll need to fix the CC lowering so that it correctly handles float ABIs in conjunction with non-scalar types. I think we've got at least 2 separate symptoms which I would like to fix here. Both of these bugs are bad interactions between split type lowering and floating-point type lowering. Both are fixed by this patch:

  ; RUN: llc -mtriple=riscv64 -mattr=+f < %s
  ; RUN: llc -mtriple=riscv64 -mattr=+f --target-abi=lp64f < %s
  ; RUN: llc -mtriple=riscv64 -mattr=+d --target-abi=lp64d < %s
  
  ; CC_RISCVAssign2XLen picks up on this and crashes, returning incorrect part type sizes
  
  define <2 x float> @callee_v2f32(<2 x float> %x) {
    ret <2 x float> %x
  }

  ; RUN: llc -mtriple=riscv32 -mattr=+f --target-abi=ilp32f  < %s
  ; RUN: llc -mtriple=riscv32 -mattr=+d --target-abi=ilp32d < %s
  ; RUN: llc -mtriple=riscv64 -mattr=+f --target-abi=lp64f < %s
  ; RUN: llc -mtriple=riscv64 -mattr=+d --target-abi=lp64d < %s
  
  ; The pending locs/split handling takes over, losing the initial "BCvt" LocInfo for the ABI, assigns an XLEN loc, and emits a COPY from float to int registers. This is an impossible reg-to-reg copy.
  
  define <4 x float> @callee_v4f32(<4 x float> %x) {
    ret <4 x float> %x
  }

Since the `Hardware Floating-point Calling Convention` doesn't refer to "aggregates" as much as it refers to "structs" I suppose it follows from your opinion about aggregates that, if the psABI is relevant, we should be looking entirely at the `Integer Calling Convention`? So there shouldn't be any use of floating-point registers in lowering these illegal vectors at all, no matter which ABI is used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102852