[llvm][RISCV] Set ScalableVector stack id in proper place

[enoskova-sc]

Without this patch ScalableVector frame index property is used before assignment. More precisely, let's take a look at RISCVFrameLowering::assignCalleeSavedSpillSlots. In this function we divide callee saved registers on scalar and vector ones, based on ScalableVector property of their frame indexes:

  ...
  const auto &UnmanagedCSI = getUnmanagedCSI(*MF, CSI);
  const auto &RVVCSI = getRVVCalleeSavedInfo(*MF, CSI);
  ...

But we assign ScalableVector property several lines below:

  ...
  auto storeRegToStackSlot = [&](decltype(UnmanagedCSI) CSInfo) {
    for (auto &CS : CSInfo) {
      // Insert the spill to the stack frame.
      Register Reg = CS.getReg();
      const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(Reg);
      TII.storeRegToStackSlot(MBB, MI, Reg, !MBB.isLiveIn(Reg),
                              CS.getFrameIdx(), RC, TRI, Register());
    }
  };
  storeRegToStackSlot(UnmanagedCSI);
  ...

Due to it, list of RVV callee saved registers will always be empty. Currently this problem doesn't appear, but if you slightly change the code and, for example, put some instructions between scalar and vector spills, the resulting code will be ill formed.

Shrink-Wrap points split Part 1.
RFC: https://discourse.llvm.org/t/shrink-wrap-save-restore-points-splitting/83581

Part 2: #119355
Part 3: #119357
Part 4: #119358
Part 5: #119359

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

Author: Elizaveta Noskova (enoskova-sc)

Changes

Without this patch ScalableVector frame index property is used before assignment. More precisely, let's take a look at RISCVFrameLowering::assignCalleeSavedSpillSlots. In this function we divide callee saved registers on scalar and vector ones, based on ScalableVector property of their frame indexes:

  ...
  const auto &UnmanagedCSI = getUnmanagedCSI(*MF, CSI);
  const auto &RVVCSI = getRVVCalleeSavedInfo(*MF, CSI);
  ...

But we assign ScalableVector property several lines below:

  ...
  auto storeRegToStackSlot = [&](decltype(UnmanagedCSI) CSInfo) {
    for (auto &CS : CSInfo) {
      // Insert the spill to the stack frame.
      Register Reg = CS.getReg();
      const TargetRegisterClass *RC = TRI->getMinimalPhysRegClass(Reg);
      TII.storeRegToStackSlot(MBB, MI, Reg, !MBB.isLiveIn(Reg),
                              CS.getFrameIdx(), RC, TRI, Register());
    }
  };
  storeRegToStackSlot(UnmanagedCSI);
  ...

Due to it, list of RVV callee saved registers will always be empty. Currently this problem doesn't appear, but if you slightly change the code and, for example, put some instructions between scalar and vector spills, the resulting code will be ill formed.

---

Full diff: https://github.com/llvm/llvm-project/pull/117862.diff

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVFrameLowering.cpp (+4)

diff --git a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
index 2da32fece061bb..950916d98df285 100644
--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
@@ -1610,6 +1610,8 @@ bool RISCVFrameLowering::assignCalleeSavedSpillSlots(
         int FrameIdx = MFI.CreateFixedSpillStackObject(Size, Offset);
         assert(FrameIdx < 0);
         CS.setFrameIdx(FrameIdx);
+        if (RISCVRegisterInfo::isRVVRegClass(RC))
+          MFI.setStackID(FrameIdx, TargetStackID::ScalableVector);
         continue;
       }
     }
@@ -1626,6 +1628,8 @@ bool RISCVFrameLowering::assignCalleeSavedSpillSlots(
     if ((unsigned)FrameIdx > MaxCSFrameIndex)
       MaxCSFrameIndex = FrameIdx;
     CS.setFrameIdx(FrameIdx);
+    if (RISCVRegisterInfo::isRVVRegClass(RC))
+      MFI.setStackID(FrameIdx, TargetStackID::ScalableVector);
   }
 
   // Allocate a fixed object that covers the full push or libcall size.

[enoskova-sc]

@4vtomat, please, review.

[4vtomat]

Umm could you provide a test case, I don't quite understand what situation could break this lol

[enoskova-sc]

Umm could you provide a test case, I don't quite understand what situation could break this lol

@4vtomat, unfortunately, I can't write any test, which can be broken in current llvm code base.

But I have faced a problem while preparing this series of patches. More precisely, Part 4, test.

All in all, I think, error can reveal in any other context while code base is developing.

[topperc]

There aren't any vector registers in FixedCSRFIMap so I don't think this can happen.

[enoskova-sc]

addressed

[topperc]

LGTM

[arsenm]

The spill size is still an unsigned, and not a TypeSize. How do scalable types still have fixed spill sizes?

[enoskova-sc]

Good question. Do you want me to investigate this issue and may be create a fix in current PR?

[topperc]

Definitely should not be fixed in this PR. It requires tablegen changes, TargetRegisterInfo.h interface changes. Probably individual changes other targets.

[enoskova-sc]

@arsenm, @4vtomat, can I merge this PR?

[4vtomat]

LGTM, thank!