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

[Sam Elliott via Phabricator via llvm-commits]

lenary marked an inline comment as done.
lenary added inline comments.


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




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70401