[PATCH] D70401: [WIP][RISCV] Implement ilp32e ABI

[Sam Elliott via Phabricator via llvm-commits]

lenary added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1525
   unsigned TwoXLenInBytes = (2 * XLen) / 8;
   if (!IsFixed && ArgFlags.getOrigAlign() == TwoXLenInBytes &&
       DL.getTypeAllocSize(OrigTy) == TwoXLenInBytes) {
----------------
shiva0217 wrote:
> The variadic argument for ilp32e doesn't need to align to even register. We could also add a test line in vararg.ll.
I'm not sure I agree with this interpretation of the psABI. The [[ https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#ilp32e-calling-convention | ILP32E Section ]] makes no exception for variadic arguments, and the base calling convention is only defined in relation to `XLEN`, not in terms of stack alignment.

I will add a test to `vararg.ll` so the behaviour is at least tested. 


================
Comment at: llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll:11
+; This test currently fails because the machine code does not validate:
+; RUN: not llc -mtriple=riscv32 -mattr=+d -target-abi ilp32e -verify-machineinstrs < %s
+; It will need FileCheck %s -check-prefix=ILP32-LP64-NO-D
----------------
lenary wrote:
> shiva0217 wrote:
> > shiva0217 wrote:
> > > Jim wrote:
> > > > shiva0217 wrote:
> > > > > lenary wrote:
> > > > > > @shiva0217 I think this test is failing because of the base pointer patch, but I'm not sure. Can you look at the issue? It thinks that x8 gets killed by a store (which I don't think should be using x8), and therefore x8 is not live when we come to the epilog. It's a super confusing issue.
> > > > > Hi @lenary, it seems that hasBP() return false in this case, the issue trigger by register allocation allocating x8 which should be preserved. I'm not sure why it will happen, I try to write a simple C code to reproduce the case but fail to do that. Could you obtain the C code for the test case?
> > > > It seems that RISCVRegisterInfo::getReservedRegs doesn't add x8(fp) into reserved registers (TFI->hasFP(MF) return false), then x8 is a candidate register for register allocation. After register allocation, some of fpr64 splitted into stack that makes stack need to be realign (MaxAlignment(8) > StackAlignment(4)), therefore x8 should be used as frame pointer (TFI->hasFP(MF) return true). In emitting epilogue, instructions for fp adjustment is inserted.
> > > With the investigation from @Jim, here is the simple C could reproduce the case.
> > >   extern double var;
> > >   extern void callee();
> > >   void test(){
> > >     double val = var;
> > >     callee();
> > >     var = val;
> > >   }
> > > Thanks, @Jim 
> > There're might be few ways to fix the issue:
> > 1. hasFP() return true for ilp32e ABI with feature D
> > 2. hasFP() return true for ilp32e ABI with feature D and there's is a virtual register with f64 type.
> > 3. Not allow  ilp32e ABI with feature D.
> > Given that most of the targets supported double float instructions have stack alignment at least eight bytes to avoid frequently realignment. Would it more reasonable to have a new embedded ABI with stack alignment at least eight bytes to support feature D?
> @Jim, @shiva0217, thank you very much for tracking down this bug, and providing a small testcase, that's very helpful.
> 
> We talked about this on the call this week, and I indicated I was going to go with a solution as close to 2 as I could.
> 
> I have since started an investigation (which I hoped would be quicker than it is) of what happens if we implement `canRealignStackFrame` to check if FP is unused, and this also seems to solve the problem. I'm doing some deeper checks (which require implementing parts of the backend around MIR that I haven't looked at before), but I think this might be a better solution? I'll keep this patch updated on when I upload the fix for stack realignment to cover this case. In the case that this fix isn't enough, I'll look to implement solution 2.
> 
> In any case, it's evident that allocating a spill slot for a register that has higher spill alignment than the stack slot, is the kernel of the problem, and this may arise again depending on how we choose to implement other extensions.
> 
> 
I couldn't find a reasonable way to check for a virtual (or physical) register of type fp64, without iterating over all the instructions in a function, which I'd prefer not to do.

So Instead I have implemented option 1 in `hasFP`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70401