Page MenuHomePhabricator

[WIP][RISCV] Implement ilp32e ABI
Needs ReviewPublic

Authored by lenary on Nov 18 2019, 8:34 AM.

Details

Summary

ILP32E is a calling convention for 32-bit RISC-V which only uses the 16
registers from the RV32E base architecture. The calling convention has a
different stack alignment, and has fewer argument registers.

Importantly, the ABI chosen does not influence which registers should be
saved when an interrupt happens (this choice is made based on the
available architectural extensions).

This patch paves the way for adding full RV32E support.

This is based upon D59592 by Daliang Xu (xudaliang.pku). I have fixed
the issue with interrupt CSRs, and excessive duplication of the GPR
list.

Event Timeline

lenary created this revision.Nov 18 2019, 8:34 AM
lenary marked 2 inline comments as done.Nov 18 2019, 8:39 AM

Some notes and queries about this patch.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
2363

In the epilog, return values have to be in a0 and a1, which we might as well be explicit about.

llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll
14

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

luismarques added inline comments.Nov 18 2019, 10:26 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1518

Consider extracting this logic into its own function and calling that function in the various places in this patch where that logic is used.

1598

There are many renames like these. Consider making ArgGPRs the ArrayRef (instead of AllocatableArgGPRs) and renaming the original array to something less generic (ArgGPRs_Standard? _Full? _NonE?)

shiva0217 added inline comments.Nov 19 2019, 12:01 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1526

The variadic argument for ilp32e doesn't need to align to even register. We could also add a test line in vararg.ll.

llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll
14

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?

Jim marked an inline comment as done.Nov 19 2019, 10:22 PM
Jim added inline comments.
llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll
14

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.

shiva0217 added inline comments.Nov 20 2019, 12:15 AM
llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll
14

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

shiva0217 added inline comments.Nov 20 2019, 6:21 PM
llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll
14

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?

lenary marked an inline comment as done.Nov 21 2019, 11:20 AM
lenary added inline comments.
llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll
14

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

lenary retitled this revision from [RISCV] Implement ilp32e ABI to [WIP][RISCV] Implement ilp32e ABI.Nov 21 2019, 11:20 AM
lenary updated this revision to Diff 233089.Dec 10 2019, 6:53 AM
  • Address ILP32E / rv32d frame pointer issue. I didn't manage to find a reasonable way to query for defined fpr64s, so I'm just enabling the FP if the D extension is enabled.
  • Update callee-saved-fpr64s.ll to clearly document expectations about the test
lenary updated this revision to Diff 233092.Dec 10 2019, 7:09 AM
  • Test Updates
lenary updated this revision to Diff 233103.Dec 10 2019, 7:42 AM
  • Update CSR definition comments
lenary updated this revision to Diff 233106.Dec 10 2019, 7:56 AM
  • Clang support for ilp32e
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2019, 7:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
lenary updated this revision to Diff 233353.Dec 11 2019, 6:39 AM
lenary marked 7 inline comments as done.
  • Update vararg.ll test to add ILP32E tests
  • Address code duplication review comments.
lenary added inline comments.Dec 11 2019, 6:40 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1526

I'm not sure I agree with this interpretation of the psABI. The 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.

llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll
14

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.

lenary updated this revision to Diff 233356.Dec 11 2019, 6:44 AM
  • Correct typo
lenary updated this revision to Diff 233369.Dec 11 2019, 7:26 AM
  • Ran clang-format
luismarques added inline comments.Dec 11 2019, 12:03 PM
llvm/lib/Target/RISCV/RISCVCallingConv.td
35

Nitpick: "the interrupt happens" -> "an interrupt happens" (or, even better, "is serviced").

llvm/test/CodeGen/RISCV/calling-conv-ilp32e-double-bug.ll
2

It would be good to describe in a comment what the bug is.

shiva0217 added inline comments.Dec 12 2019, 5:14 AM
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1526

It seems to be the current GCC behavior and the following case could observe that double will not align to even pair.

#include <stdarg.h>
void va_double (int n, ...) {
  va_list args;
  va_start (args, n);
  if (va_arg (args, double) != 2.0)
    abort ();
  va_end (args);
 }
 int main (int a) {
  va_double (1, 2.0);
  return a;
 }

In a second thought, it seems that non-fixed double arguments may generate incorrect code, even with align even pair.
For ilp32 or lp64 ABI with feature D, stack alignment will be 16, so even pair can make sure when pushing/popping the non-fixed double to/from the stack, it will be 8-byte alignment. For ilp32e with 4-byte alignment, even pair can not guarantee the double will be pushed to stack with 8-byte alignment.

llvm/test/CodeGen/RISCV/callee-saved-fpr64s.ll
14

I think option 1 could be a reasonable way to fix the issue.

lenary updated this revision to Diff 234070.Dec 16 2019, 8:09 AM
  • Update Comments
lenary marked 3 inline comments as done.Dec 16 2019, 8:32 AM
lenary added inline comments.
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
1526

Ah, I see the issue.

It's not clear that choosing to spill to a register pair where the first register is a multiple of 4 would solve the problem either, right? The problem is that we actually need to realign the spill slots for these register pairs.

I'm not sure how we achieve this. I will investigate further.

Jim added inline comments.Dec 24 2019, 7:33 PM
llvm/test/CodeGen/RISCV/calling-conv-ilp32e-double-bug.ll
2

No FileCheck line here.