Page MenuHomePhabricator

[RISCV] Remove RA from reserved register to use as callee saved register
ClosedPublic

Authored by shiva0217 on Sep 18 2019, 2:00 AM.

Details

Summary

Remove RA from reserved register list, so we could use it as callee saved register

Diff Detail

Event Timeline

shiva0217 created this revision.Sep 18 2019, 2:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 18 2019, 2:00 AM

We discussed this in the RISC-V meeting on 19 Sep 2019.

  • Pros: GCC for RISC-V and LLVM for ARM and AArch64 seem to do the same. It can help in situations with particularly bad register pressure requirements.
  • Cons: It can make debugging a lot harder, though this seems not to be an issue in GDB for RISC-V.

I think we don't want to have yet another configuration flag to control this.

It would be good to see some performance comparison, but I realise you may not be able to release internal benchmarks, and we have no public benchmarking system for RISC-V.

We discussed this in the RISC-V meeting on 19 Sep 2019.

  • Pros: GCC for RISC-V and LLVM for ARM and AArch64 seem to do the same. It can help in situations with particularly bad register pressure requirements.
  • Cons: It can make debugging a lot harder, though this seems not to be an issue in GDB for RISC-V.

I think we don't want to have yet another configuration flag to control this.

It would be good to see some performance comparison, but I realise you may not be able to release internal benchmarks, and we have no public benchmarking system for RISC-V.

I can't reveal the detail of the benchmark, but the most significant case improve about 1%. It might be reasonable to free RA for register allocation since most of the backend already do so.

In CoreMark-Pro, when the execution model is 1 instr == 1 cycle, with this patch the results I get is that sha-test improves by +1.43% in RV64 (GC, LP64D). For all other sub-benchmarks the performance differences round to 0.00%. There's no change to sha-test in RV32 before and after the patch.

We had a discussion internally at lowRISC about this. I no longer think that debuggability is a concern, as FP/CFI-info is far more important than the return address. Alex has raised that preserving RA is useful for some lightweight tracing applications, but I feel that can be solved with the implementation of -ffixed-x1 (D67185).

With that in mind, we're happy to approve this patch, pending the following:

Please can you rebase it? It applies, but tests fail. Looks like a simple update caused by a change to the materialisation of the address of var.

shiva0217 updated this revision to Diff 224951.Oct 14 2019, 7:59 PM

Rebase the test case.

Good to know -ffixed-x1 seems to be a great solution.

lenary accepted this revision.Oct 28 2019, 7:06 AM

LGTM, thanks @shiva0217

This revision is now accepted and ready to land.Oct 28 2019, 7:06 AM
This revision was automatically updated to reflect the committed changes.