Page MenuHomePhabricator

[RISCV] Add a scavenge spill slot when use ADDI to compute scalable stack offset
ClosedPublic

Authored by StephenFan on Jun 20 2022, 5:34 AM.

Details

Summary

Computing scalable offset needs up to two scrach registers. We add
scavenge spill slots according to the result of RISCV::isRVVSpill
and RVVStackSize. Since ADDI is not included in RISCV::isRVVSpill,
PEI doesn't add scavenge spill slots for scrach registers when using
ADDI to get scalable stack offsets.

The ADDI instruction has a destination register which can be used as
a scrach register. So one scavenge spil slot is sufficient for
computing scalable stack offsets.

Diff Detail

Event Timeline

StephenFan created this revision.Jun 20 2022, 5:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 20 2022, 5:34 AM
StephenFan requested review of this revision.Jun 20 2022, 5:34 AM

Rename some variables.

Is it possible to add a test where we use the 2 slots for spills?

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

Why not use std::max here? Why separate local/global variable?

986

Should we stop searching when GlobalMostScavSlotsNum becomes 2?

Add a mir test

  1. Early return if GlobalMostScavSlotsNum reaches the GlobalMostScavSlotsNumKnown
  2. Remove LocalMostScavSlotsNum

Is it possible to add a test where we use the 2 slots for spills?

I added a mir test since it seems that .ll test can't reflect the number of scavenging slots very well.

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

You are right, the local variable is redundant and I should use std::max here

986

Yes.

craig.topper added inline comments.Jun 27 2022, 9:32 AM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
968

Variable names should be capitalized

972

Variable names should be capitalized

Capitalize variable names.

StephenFan marked 2 inline comments as done.Jun 27 2022, 10:34 PM
craig.topper added inline comments.Jun 27 2022, 11:22 PM
llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
975

Drop "Global". Without Local it doesn't mean anything.

Replace "Most" with "Max"

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

Is IsRVVSpill used anywhere other than the following if? If not can we call RICV::isRVVSpill as part of the if and remove the variable.

  1. Drop global.
  2. Remove local variable IsRVVSpill
  3. Replace 'Most' with 'Max'
StephenFan marked an inline comment as done.Jun 27 2022, 11:36 PM
StephenFan added inline comments.
llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
177

It is used on lines 262 and 272.

Undo changes.

This revision is now accepted and ready to land.Jun 30 2022, 2:56 PM
frasercrmck added inline comments.Jul 1 2022, 1:14 AM
llvm/test/CodeGen/RISCV/rvv/rvv-stack-align.mir
125–128

What's causing changes in these tests?

craig.topper added inline comments.Jul 1 2022, 9:07 AM
llvm/test/CodeGen/RISCV/rvv/rvv-stack-align.mir
125–128

We're allocating an emergency spill slot because there is an ADDI frame index reference to a scalable slot. We weren't considering ADDI before.

This revision was landed with ongoing or failed builds.Jul 3 2022, 6:04 AM
This revision was automatically updated to reflect the committed changes.