[PATCH] D70401: [RISCV] Implement ilp32e ABI

[Shiva Chen via Phabricator via llvm-commits]

shiva0217 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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70401