This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Frame handling for RISC-V V extension.
AbandonedPublic

Authored by HsiangKai on Dec 23 2020, 1:46 AM.

Details

Summary

This patch proposes how to deal with RISC-V vector frame objects. The layout of RISC-V vector frame will look like

|---------------------------------|
| RVV incoming arguments          |
|---------------------------------| previous frame
| scalar incoming arguments       |
|---------------------------------| <- start of frame (fp)
| scalar callee-saved registers   |
|---------------------------------|
| scalar local variables          |
|---------------------------------|
| addresses of RVV objects (*)    | current frame
|---------------------------------|
| RVV local variables             |
|---------------------------------|
| RVV outgoing arguments          |
|---------------------------------|
| scalar outgoing arguments       |
|---------------------------------| <- end of frame (sp)

We will reserve stack space for RISC-V vector frame objects and store their base addresses in “addresses of RVV objects” area. The benefits of this way is we have no need to calculate the addresses of RVV objects repeatedly. There is no easy way to get the addresses of the RISC-V vector objects in V instructions. We need to read out VLENB and multiple VLENB with LMUL to know the size of vector objects. Then we need to subtract or add the size from the stack pointer. If there are multiple vector objects in the frame, we need to subtract or add the accumulated size from the stack pointer. If we store the base addresses in the frame first, we only need to load the base address from the stack frame and access the vector object directly.

Spilling for RISC-V V extension will be another patch.

Authored-by: Roger Ferrer Ibanez <rofirrim@gmail.com>
Co-Authored-by: Hsiangkai Wang <kai.wang@sifive.com>

Diff Detail

Unit TestsFailed

Event Timeline

HsiangKai created this revision.Dec 23 2020, 1:46 AM
HsiangKai requested review of this revision.Dec 23 2020, 1:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2020, 1:46 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
HsiangKai updated this revision to Diff 313541.Dec 23 2020, 6:03 AM
HsiangKai added reviewers: lenary, asb.
jrtc27 added inline comments.Dec 23 2020, 6:23 AM
llvm/include/llvm/CodeGen/MachineFrameInfo.h
226

How come SVE doesn't need this already?

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
280

Hm, looking at the stack-realignment.ll diff, were we double-counting before? If so it'd be worth splitting out that bit of this revision into its own smaller one.

496

:( is there progress being made to specify it?

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
249

Can you not just do isa<> to check it's the right class and then look at mayLoad/mayStore?

llvm/test/CodeGen/RISCV/rvv/allocate-lmul-2-4-8.ll
5

Only nounwind is needed for these I believe?

HsiangKai added inline comments.Dec 23 2020, 7:54 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
280

I think so. I will create another patch for it.

StephenFan added inline comments.Dec 23 2020, 8:17 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
547

why if the machine function has the scalable vector objects, then the prolog doesn't need to realign the stack address ?

HsiangKai added inline comments.Dec 24 2020, 6:15 AM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
496

After rethinking it, there is no callee-saved vector registers in the current specification. I think we could remove the if statement.

Reference: https://github.com/riscv/riscv-v-spec/blob/master/calling-convention.adoc

HsiangKai added inline comments.Dec 24 2020, 6:34 AM
llvm/include/llvm/CodeGen/MachineFrameInfo.h
226

I didn't dig into how SVE implements it. I will do some study.

llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
547

I need to think about how to deal with it when there are objects needed realignment and RVV objects on the stack at the same time.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
249

Good idea! Thanks for your tips.

HsiangKai updated this revision to Diff 313692.Dec 24 2020, 6:47 AM
StephenFan added inline comments.Dec 24 2020, 6:57 AM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
547

we have an internal implementation that if needs realignment and RVV objects on the stack at the same time. The BP register needs to be used as base address of stack object. So modify the hasBP function in RISCVFrameLowering.cpp may be a good choice. However, maybe there is a better solution.

khchen added a subscriber: khchen.Dec 24 2020, 6:40 PM
StephenFan added inline comments.Dec 24 2020, 7:21 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
829

Is This for loop unuseful? because in the FunctionLoweringinfo.cpp has a set function that will setstackid

if (isa<ScalableVectorType>(Ty))
  MF->getFrameInfo().setStackID(FrameIndex,
                                TFI->getStackIDForScalableVectors());
HsiangKai updated this revision to Diff 313728.Dec 25 2020, 7:15 AM
HsiangKai updated this revision to Diff 313729.Dec 25 2020, 7:18 AM
HsiangKai marked 5 inline comments as done.
HsiangKai updated this revision to Diff 313768.Dec 26 2020, 6:10 PM

Deal with the case when there are realignment objects and RVV objects on the stack at the same time.

HsiangKai marked an inline comment as done.Dec 26 2020, 6:11 PM
HsiangKai added inline comments.Dec 27 2020, 6:58 PM
llvm/include/llvm/CodeGen/MachineFrameInfo.h
226

The SVE spill/fill instructions have VL-scaled addressing modes such as:
ldr z8, [fp, #-7 mul vl]

In SVE, there is no need to do address calculation to get the location of SVE frame objects. The number of SVE objects is encoded in the instruction.

In RVV, we have no VL-scaled addressing mode. We need to read out VLENB, do multiplication or shift to know the size of RVV objects, and add/subtract the accumulated size from FP/SP. To avoid the repeated address calculation, we have two areas for RVV objects in the frame. One is for RVV object addresses, and another is for RVV objects themselves. SP will be updated by the size of RVV object region if there are any RVV objects. We still could use SP to access frame objects, but it will need to add multiple of the run-time VLENB value. To ease the complexity, we use FP to access frame objects if there is any RVV object on the stack. That’s why I define hasScalableVectorObjects() to know about it.

jrtc27 added inline comments.Jan 3 2021, 5:59 PM
llvm/include/llvm/CodeGen/MachineFrameInfo.h
226

Isn't that basically just a special case of DYNAMIC_STACKALLOC? Can we not reuse some of that logic with BP?

craig.topper added inline comments.Jan 4 2021, 5:17 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
632

unsigned -> Register.

Use RISCV::NoRegister instead of 0

633

BinSizes.data() might be a little better than "&BinSizes[0]"

llvm/lib/Target/RISCV/RISCVMCInstLower.cpp
204

Can we use RISCVSysReg::lookupSysRegByName("VLENB")->Encoding here instead of adding a new enum that repeats the addresses?

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

Are we just using RVV as a boolean and not a pointer? Should we do something like?

bool IsRVV = RISCVVPseudosTable::getPseudoInfo(MI.getOpcode()) != nullptr;
192

Add 'const' as the pre-merge check suggests.

jrtc27 added inline comments.Jan 4 2021, 5:32 PM
llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.h
438–443

Only VLENB is used, and as Craig says we use lookupSysRegByName for CYCLE[H] already. If we wanted to avoid that we should modify TableGen, not duplicate things here.

jrtc27 added a comment.Jan 4 2021, 5:35 PM

One thing I don't understand is why RVV incoming/outgoing arguments need their own stack slots. I assume they're just passed indirectly, so why do we need to distinguish between arguments and locals rather than just treat them like anything else that ends up being passed indirectly?

HsiangKai updated this revision to Diff 314851.Jan 6 2021, 4:02 AM
HsiangKai added a reviewer: jrtc27.

Address @craig.topper and @jrtc27's comments.

HsiangKai marked 5 inline comments as done.Jan 6 2021, 4:04 AM
HsiangKai added inline comments.Jan 6 2021, 4:30 AM
llvm/include/llvm/CodeGen/MachineFrameInfo.h
226

It is not a special case of DYNAMIC_STACKALLOC. The RVV objects are statically allocated. The path is different in SelectionDAGBuilder::visitAlloca().

One thing I don't understand is why RVV incoming/outgoing arguments need their own stack slots. I assume they're just passed indirectly, so why do we need to distinguish between arguments and locals rather than just treat them like anything else that ends up being passed indirectly?

It seems the existing logic in https://github.com/llvm/llvm-project/blob/4e0e79dd349a208384449fd8dcdc9bf1644ee0f3/llvm/lib/Target/RISCV/RISCVISelLowering.cpp#L3154.

In current RISC-V implementation, if the argument is passed indirectly, it will create a temporary frame object and pass the address of the temporary frame object.

HsiangKai abandoned this revision.Jan 13 2021, 7:25 AM

Let's focus on D94465. If there is anyone oppose it, I could reactivate this commit.