Page MenuHomePhabricator

[RISCV] Enable the LocalStackSlotAllocation pass support
Needs ReviewPublic

Authored by StephenFan 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

StephenFan created this revision.Mar 5 2021, 9:35 PM
StephenFan requested review of this revision.Mar 5 2021, 9:35 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2021, 9:35 PM
StephenFan edited the summary of this revision. (Show Details)Mar 5 2021, 9:36 PM
craig.topper added inline comments.Mar 5 2021, 9:58 PM
llvm/test/CodeGen/RISCV/local-stack-allocation.ll
1

Is it possible to pre-commit this test case and only show the changes here in this patch?

craig.topper added inline comments.Mar 5 2021, 10:03 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
309

I think vector load/store can have a frameindex operand. There's no immediate field so it will get split out into an ADDI in eliminateFrameIndex, but I think that happens later.

StephenFan added inline comments.Mar 5 2021, 10:10 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
309

In the RISCVIselDAGToDAG.cpp:

case ISD::FrameIndex: {
    SDValue Imm = CurDAG->getTargetConstant(0, DL, XLenVT);
    int FI = cast<FrameIndexSDNode>(Node)->getIndex();
    SDValue TFI = CurDAG->getTargetFrameIndex(FI, VT);
    ReplaceNode(Node, CurDAG->getMachineNode(RISCV::ADDI, DL, VT, TFI, Imm));
    return;
  }

So the llvm ir like this :

%addr = alloca i8
call void callee(i8* %addr)

the %addr will be selected to ADDI

craig.topper added inline comments.Mar 5 2021, 10:20 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
309

Right. I was just asking why the comment says vector load/store are different?

StephenFan added inline comments.Mar 5 2021, 10:25 PM
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
309

Oh, sorry. You're right, the vector load/store can have a FrameIndex Operand. I made a mistake.

StephenFan marked 2 inline comments as done.Mar 5 2021, 10:25 PM

Pre-commit local-stack-allocation.ll test.

StephenFan marked an inline comment as done.Mar 5 2021, 10:32 PM
StephenFan added inline comments.Mar 5 2021, 10:39 PM
llvm/test/CodeGen/RISCV/local-stack-allocation.ll
17

This instruction will be eliminated in D92479 which I will land it soon after.

Fix comment error.

asb requested changes to this revision.Apr 1 2021, 3:37 AM

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.

This revision now requires changes to proceed.Apr 1 2021, 3:37 AM
StephenFan updated this revision to Diff 336311.Thu, Apr 8, 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.Thu, Apr 15, 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
291

No need to copy this doc comment TargetRegisterInfo.h.

298

returns => Returns

300

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

305

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

309

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?

328

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

334

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

350

Nit: end with full stop

369

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

379

Nit: end with full stop

387

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.Fri, Apr 16, 11:50 PM
StephenFan added inline comments.
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
300

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.