Page MenuHomePhabricator

[RISCV] Enable the LocalStackSlotAllocation pass support
Needs ReviewPublic

Authored by LiDongjin on Mar 5 2021, 9:35 PM.

Details

Summary

For RISC-V, load/store(exclude vector load/store) instructions only has a 12 bit immediate operand. If the offset is out-of-range, it must make use of a temp register to make up this offset. If between these offsets, they have a small(IsInt<12>) relative offset, LocalStackSlotAllocation pass can find a value as frame base register's value, and replace the origin offset with this register's value plus the relative offset.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
StephenFan updated this revision to Diff 336311.Apr 8 2021, 9:51 PM

Fix error.

In D98101#2663432, @asb wrote:

Could you please rebase this to account for D98716?

I _think_ the right fix is to change the call for needsStackRealignment to shouldRealignStack. But I'm seeing multiple runtime failures for the GCC torture suite with the patch applied. e.g. 20031012-1.c at O0 for rv32imafdc ilp32.

Hi @asb . I wrote a script to compile and run the gcc c torture tests on qemu. According to the test result and as you said, multiple runtime failures appeared. However, 20031012-1.c at O0 for rv32imafdc ilp32 didn't fail on qemu. And I have fixed the failures and updated the patch. Can you help me testing the gcc c torture test again? Thanks!

asb added a comment.Apr 15 2021, 8:06 AM

The GCC torture suite now gets a 100% pass rate for me - thanks for fixing. I've left various minor notes about comment phrasing and formatting etc.

I don't feel I've stepped through the logic of the patch carefully enough yet, but hopefully the suggested edits are actionable in the meantime.

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
364

No need to copy this doc comment TargetRegisterInfo.h.

371

returns => Returns

373

I think this sentence needs rewriting - I can't quite follow it.

378

clang-format prefers this with a space after the first ;

382

This thread is marked as done, but the comment still same to make the same claim that vector load/store don't have a frameindex?

401

I think: "the maximum possible offset relative to the frame pointer."?

407

bytes => byte and "maximum possible offset relative to the stack pointer." I think

423

Nit: end with full stop

442

Nit: re-wrap and end sentence in full stop.

452

Nit: end with full stop

460

Nit: end with full stop.

Address @asb 's comment

StephenFan marked an inline comment as done.

Delete "that" in comment

StephenFan marked 9 inline comments as done.Apr 16 2021, 11:50 PM
StephenFan added inline comments.
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
373

emm, This sentence is copied from TargetRegisterInfo.h. It means that, For RISCV, if a frame index operand has a Offset that out-of-range 12 bits, this function will return true to indicate this frame index operand needs a frame base register.

asb added inline comments.May 13 2021, 12:50 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
373

Yes that was my bad - the sentence does indeed make sense as written.

I've benchmarked the impact of this patch with CoreMark and Embench and it looked good. In summary, there were no or minimal performance differences but there were various small improvements to code size, which are probably similar to your test case.
I was going to add other checks like running the llvm test suite but the patch no longer applies. Can you refresh it?

jrtc27 added inline comments.May 13 2021, 7:43 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
383–387

Can we not just look and see if it both has an FI and is an I or S type instruction? This isn't really flexible, and I don't see why it's needed either (and I'd have to more than double the size of this switch statement downstream in CHERI-LLVM to add both capability base and capability value instructions).

398–400

What about floating point registers? Can we also generalise this to iterate over the list of saved registers rather than copying the ABI to yet another place (and thus yet another place we'd have to change downstream in CHERI-LLVM)?

405

Why 128? A hard-coded constant common across all ISAs seems fishy.

445

We use the less-confusing name FIOperandNum elsewhere

447

The copies of this in RegisterScavenging and other backends have assert(i < MI.getNumOperands() && "Instr doesn't have FrameIndex operand!");

449–451

I don't see why the first part matters. Only include the relevant information, that "FrameIndex operands are always represented as a register followed by an immediate".

452

This variable seems pointless to me, just inline the +1

llvm/test/CodeGen/RISCV/local-stack-allocation.ll
30 ↗(On Diff #338495)

Please fix this test, either before you pre-commit it or now as another pre-commit if you've already done so.

craig.topper added inline comments.Sep 29 2022, 1:09 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
382

I think vector load/store no longer have frame indices.

383–387

This is missing LBU, LHU, and LWU.

I agree with @jrtc27 we should check the I or S type instead.

396

No need to say 'int' llvm almost always uses unsigned by itself.

398–400

Are X3 and X4 really callee saved to the stack? They're reserved so I don't think we ever spill them.

Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 1:09 PM

Address Comments.

StephenFan marked 9 inline comments as done.Sep 30 2022, 8:24 AM

Improve comments.

StephenFan added inline comments.Sep 30 2022, 8:48 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
405

This value should be an experimental target-dependent value. But I don't know yet which value is appropriate for riscv.

craig.topper added inline comments.Sep 30 2022, 10:26 AM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
398

Is ReservedByUser not already cover by getReservedRegs?

405

Can we copy the comments from ARM and AArch64 here including the FIXME for 128?

StephenFan updated this revision to Diff 464553.Oct 2 2022, 7:38 AM
  1. Remove check user reserved regsiter list.
  2. Improve comments.
craig.topper added inline comments.Oct 4 2022, 9:52 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
465

Should we assert that it is I or S format?

StephenFan updated this revision to Diff 465382.Oct 5 2022, 6:58 AM

Add assertion.

I think we need this change from D134851 to match SVE from D83859

 bool isStackIdSafeForLocalArea(unsigned StackId) const override {
  // We don't support putting RVV objects into the pre-allocated local
  // frame block at the moment.
  return StackId != TargetStackID::ScalableVector;
}

Implement target hook.

craig.topper added inline comments.Oct 12 2022, 10:48 AM
llvm/test/CodeGen/RISCV/local-stack-slot-allocation.ll
7–8

Remove TODO

Remove TODO.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 19 2022, 1:16 AM
This revision was automatically updated to reflect the committed changes.

I'm seeing failures in the llvm-testsuite on riscv64-unknown-linux-gnu with -O2 (no vectors) which git bisect attributes to this change.

Failed Tests (3):
  test-suite :: MultiSource/Benchmarks/DOE-ProxyApps-C++/CLAMR/CLAMR.test
  test-suite :: MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4.test
  test-suite :: SingleSource/UnitTests/matrix-types-spec.test

Has anyone seen something similar?

I'm seeing failures in the llvm-testsuite on riscv64-unknown-linux-gnu with -O2 (no vectors) which git bisect attributes to this change.

Failed Tests (3):
  test-suite :: MultiSource/Benchmarks/DOE-ProxyApps-C++/CLAMR/CLAMR.test
  test-suite :: MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4.test
  test-suite :: SingleSource/UnitTests/matrix-types-spec.test

Has anyone seen something similar?

I think we're seeing those failures too.

craig.topper reopened this revision.Nov 1 2022, 8:21 PM
This comment was removed by dnpetrov-sc.
LiDongjin removed a subscriber: LiDongjin.
LiDongjin commandeered this revision.Sun, Nov 27, 7:12 PM
LiDongjin added a reviewer: StephenFan.

Fix the failures in the llvm-testsuite on riscv64-unknown-linux-gnu with -O2 (no vectors).

LiDongjin updated this revision to Diff 478127.Sun, Nov 27, 7:24 PM

For the failures in the llvm-testsuite, it seems some problems for Add instruction which generates wrong frame index.
Also in Arm and AArch64, No need to support for Add instruction.
Therefore load/store(exclude vector load/store) instruction is enough for LocalStackSlotAllocation pass.

If it would be better to add a test case that tests we don't do local stack slot allocation on ADDI instruction?

LiDongjin updated this revision to Diff 478853.Wed, Nov 30, 1:35 AM