[CodeGen] Allow negative frame indicies in Register class. #164459

mgudim · 2025-10-21T17:12:42Z

The register values between 2 << 30 (inclusive) and 2 << 31 (exclusive) correspond to frame indices. To obtain the frame index from the given register value we interpret first 30 bits as an unsigned integer. Thus, currently only non-negative frame indices can be represented.

However, we should also be able to represent negative frame indices as register values as well. This is used by reaching definitions analysis for example.

In order to do that, we interpret the first 30 bits of the register value as a signed integer.

llvmbot · 2025-10-21T17:13:34Z

@llvm/pr-subscribers-llvm-regalloc

Author: Mikhail Gudim (mgudim)

Changes

This is used by reaching definitions analysis in order to track stores / loads from negative frame indices.

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

2 Files Affected:

(modified) llvm/include/llvm/CodeGen/Register.h (+7-4)
(modified) llvm/lib/CodeGen/ReachingDefAnalysis.cpp (-3)

diff --git a/llvm/include/llvm/CodeGen/Register.h b/llvm/include/llvm/CodeGen/Register.h
index e462a814562dc..961d6fe744b15 100644
--- a/llvm/include/llvm/CodeGen/Register.h
+++ b/llvm/include/llvm/CodeGen/Register.h
@@ -10,6 +10,7 @@
 #define LLVM_CODEGEN_REGISTER_H
 
 #include "llvm/MC/MCRegister.h"
+#include "llvm/Support/MathExtras.h"
 #include <cassert>
 
 namespace llvm {
@@ -35,7 +36,9 @@ class Register {
   // DenseMapInfo<unsigned> uses -1u and -2u.
   static_assert(std::numeric_limits<decltype(Reg)>::max() >= 0xFFFFFFFF,
                 "Reg isn't large enough to hold full range.");
-  static constexpr unsigned FirstStackSlot = 1u << 30;
+  static constexpr unsigned MaxFrameIndexBitwidth = 30;
+  static constexpr unsigned FirstStackSlot = 1u << MaxFrameIndexBitwidth;
+  static const unsigned StackSlotMask = (unsigned)(-1) >> (CHAR_BIT * sizeof(unsigned) - MaxFrameIndexBitwidth);
   static_assert(FirstStackSlot >= MCRegister::LastPhysicalReg);
   static constexpr unsigned VirtualRegFlag = 1u << 31;
 
@@ -46,8 +49,8 @@ class Register {
 
   /// Convert a non-negative frame index to a stack slot register value.
   static Register index2StackSlot(int FI) {
-    assert(FI >= 0 && "Cannot hold a negative frame index.");
-    return Register(FI + Register::FirstStackSlot);
+    assert(isInt<30>(FI) && "Frame index must be at most 30 bit integer");
+    return Register((FI & Register::StackSlotMask) | Register::FirstStackSlot);
   }
 
   /// Return true if the specified register number is in
@@ -87,7 +90,7 @@ class Register {
   /// Compute the frame index from a register value representing a stack slot.
   int stackSlotIndex() const {
     assert(isStack() && "Not a stack slot");
-    return static_cast<int>(Reg - Register::FirstStackSlot);
+    return static_cast<int>(SignExtend64(Reg & Register::StackSlotMask, 30));
   }
 
   constexpr operator unsigned() const { return Reg; }
diff --git a/llvm/lib/CodeGen/ReachingDefAnalysis.cpp b/llvm/lib/CodeGen/ReachingDefAnalysis.cpp
index 40a89078bcf59..61706e13b8e91 100644
--- a/llvm/lib/CodeGen/ReachingDefAnalysis.cpp
+++ b/llvm/lib/CodeGen/ReachingDefAnalysis.cpp
@@ -193,7 +193,6 @@ void ReachingDefInfo::processDefs(MachineInstr *MI) {
   for (auto &MO : MI->operands()) {
     if (MO.isFI()) {
       int FrameIndex = MO.getIndex();
-      assert(FrameIndex >= 0 && "Can't handle negative frame indicies yet!");
       if (!isFIDef(*MI, FrameIndex, TII))
         continue;
       MBBFrameObjsReachingDefs[{MBBNumber, FrameIndex}].push_back(CurInstr);
@@ -302,8 +301,6 @@ void ReachingDefInfo::print(raw_ostream &OS) {
         Register Reg;
         if (MO.isFI()) {
           int FrameIndex = MO.getIndex();
-          assert(FrameIndex >= 0 &&
-                 "Can't handle negative frame indicies yet!");
           Reg = Register::index2StackSlot(FrameIndex);
         } else if (MO.isReg()) {
           if (MO.isDef())

github-actions · 2025-10-21T17:14:33Z

✅ With the latest revision this PR passed the C/C++ code formatter.

mgudim · 2025-10-21T17:17:58Z

this is part of work for: #90819
but hopefully it is useful on its own.

This is used by reaching definitions analysis in order to track stores / loads from negative frame indices.

llvm/include/llvm/CodeGen/Register.h

arsenm

I don't understand the title. Can you rephrase an elaborate in the description

llvm/include/llvm/CodeGen/Register.h

Simplified the definition of "StackSlotMask" Fixed wrong assertion. Added unit test.

topperc · 2025-10-22T18:37:11Z

I tried to make the title better.

llvm/include/llvm/CodeGen/Register.h

topperc · 2025-10-22T18:39:05Z

llvm/include/llvm/CodeGen/Register.h

    assert(isStack() && "Not a stack slot");
-    return static_cast<int>(Reg - Register::FirstStackSlot);
+    return static_cast<int>(
+        SignExtend64<MaxFrameIndexBitwidth>(Reg & Register::StackSlotMask));


Use SignExtend32 so you don't need the static_cast?

I thought int could be smaller then int32_t in theory.

Actually, to me it looks like stackSlotIndex should return int32_t. If int is not 32 bits, the result may not be able to fit, no?

@topperc @arsenm

We use unsigned to represent a register, but we implicitly assume that it is 32 bits at least. Isn't it wrong? should we use uint32_t for register values?

EDIT:
Oh, I see there is this static assert:

static_assert(std::numeric_limits<decltype(Reg)>::max() >= 0xFFFFFFFF

is this OK to merge?

llvm/include/llvm/CodeGen/Register.h

llvm/unittests/CodeGen/RegisterTest.cpp

MatzeB · 2025-10-22T22:15:30Z

I don't understand what a "negative frame index" is supposed to be. The Register class can encode references to stack slots which are indices into an array of stack objects in MachineFrameInfo and negative indexes make no sense there, no?

Edit: Apparently it does make sense.

topperc · 2025-10-22T22:17:34Z

I don't understand what a "negative frame index" is supposed to be. The Register class can encode references to stack slots which are indices into an array of stack objects in MachineFrameInfo and negative indexes make no sense there, no?

From MachineFrameInfo.h

/// To support this, the class assigns unique integer identifiers to stack       
/// objects requested clients.  These identifiers are negative integers for      
/// fixed stack objects (such as arguments passed on the stack) or nonnegative   
/// for objects that may be reordered.  Instructions which refer to stack        
/// objects use a special MO_FrameIndex operand to represent these frame         
/// indexes.

llvm/unittests/CodeGen/RegisterTest.cpp

mgudim · 2025-10-23T14:27:17Z

I don't understand what a "negative frame index" is supposed to be. The Register class can encode references to stack slots which are indices into an array of stack objects in MachineFrameInfo and negative indexes make no sense there, no?

From MachineFrameInfo.h
/// To support this, the class assigns unique integer identifiers to stack       
/// objects requested clients.  These identifiers are negative integers for      
/// fixed stack objects (such as arguments passed on the stack) or nonnegative   
/// for objects that may be reordered.  Instructions which refer to stack        
/// objects use a special MO_FrameIndex operand to represent these frame         
/// indexes.  

@MatzeB @topperc @arsenm
We expect vast majority of the indices to be positive and a few negative ones. In theory we could "shift" the values so that we use the same 30 bits but only represent say 100 negative values and the rest are positive. I can do this too if people think its necessary.

topperc · 2025-10-23T17:26:46Z

I don't understand what a "negative frame index" is supposed to be. The Register class can encode references to stack slots which are indices into an array of stack objects in MachineFrameInfo and negative indexes make no sense there, no?

From MachineFrameInfo.h
/// To support this, the class assigns unique integer identifiers to stack       
/// objects requested clients.  These identifiers are negative integers for      
/// fixed stack objects (such as arguments passed on the stack) or nonnegative   
/// for objects that may be reordered.  Instructions which refer to stack        
/// objects use a special MO_FrameIndex operand to represent these frame         
/// indexes.  
@MatzeB @topperc @arsenm We expect vast majority of the indices to be positive and a few negative ones. In theory we could "shift" the values so that we use the same 30 bits but only represent say 100 negative values and the rest are positive. I can do this too if people think its necessary.

I don't think its necessary. Having more than 2^29 frame indices in either direction seems unlikely. The amount of memory consumed by MachineFrameInfo just to track that would be excessive.

llvm/include/llvm/CodeGen/Register.h

llvm/unittests/CodeGen/RegisterTest.cpp

mgudim · 2025-10-27T19:15:05Z

does this look OK to merge?

topperc · 2025-10-27T21:09:48Z

llvm/unittests/CodeGen/RegisterTest.cpp

+  EXPECT_EQ(Register::index2StackSlot(-1),
+            Register::StackSlotZero | Register::StackSlotMask);
+  // check that we do not crash on the highest possible value of frame index.
+  EXPECT_NO_FATAL_FAILURE(Register::index2StackSlot((1 << 29) - 1));


Use named constants for 29 here and the line below?

ppenzin · 2025-10-31T18:31:40Z

llvm/unittests/CodeGen/RegisterTest.cpp

+  EXPECT_EQ(Register::index2StackSlot(0), Register::StackSlotZero);
+  EXPECT_EQ(Register::index2StackSlot(1), Register::StackSlotZero | 1);
+  EXPECT_EQ(Register::index2StackSlot(-1),
+            Register::StackSlotZero | Register::StackSlotMask);
+  // check that we do not crash on the highest possible value of frame index.
+  EXPECT_NO_FATAL_FAILURE(Register::index2StackSlot((1 << 29) - 1));
+  // check that we do not crash on the lowest possible value of frame index.
+  EXPECT_NO_FATAL_FAILURE(Register::index2StackSlot(-(1 << 29)));


Suggested change

EXPECT_EQ(Register::index2StackSlot(0), Register::StackSlotZero);

EXPECT_EQ(Register::index2StackSlot(1), Register::StackSlotZero | 1);

EXPECT_EQ(Register::index2StackSlot(-1),

Register::StackSlotZero | Register::StackSlotMask);

// check that we do not crash on the highest possible value of frame index.

EXPECT_NO_FATAL_FAILURE(Register::index2StackSlot((1 << 29) - 1));

// check that we do not crash on the lowest possible value of frame index.

EXPECT_NO_FATAL_FAILURE(Register::index2StackSlot(-(1 << 29)));

int MaxPowOf2 = 1 << 29;

EXPECT_EQ(Register::index2StackSlot(0), Register::StackSlotZero);

EXPECT_EQ(Register::index2StackSlot(1), Register::StackSlotZero | 1);

EXPECT_EQ(Register::index2StackSlot(-1),

Register::StackSlotZero | Register::StackSlotMask);

// check that we do not crash on the highest possible value of frame index.

EXPECT_NO_FATAL_FAILURE(Register::index2StackSlot(MaxPowOf2 - 1));

// check that we do not crash on the lowest possible value of frame index.

EXPECT_NO_FATAL_FAILURE(Register::index2StackSlot(-MaxPowOf2));

Or just MaxBits = 29 or something like that.

It should use Register::MaxFrameIndexBitwidth - 1 instead of putting 29 anywhere in the test.

mgudim requested a review from topperc October 21, 2025 17:12

llvmbot added llvm:codegen llvm:regalloc labels Oct 21, 2025

mgudim requested a review from ppenzin October 21, 2025 17:14

mgudim marked this pull request as draft October 21, 2025 17:18

Negative frame indicies as register.

dc95302

This is used by reaching definitions analysis in order to track stores / loads from negative frame indices.

mgudim force-pushed the neg_indices branch from 59ee5ba to dc95302 Compare October 21, 2025 19:08

topperc reviewed Oct 21, 2025

View reviewed changes

llvm/include/llvm/CodeGen/Register.h Outdated Show resolved Hide resolved

llvm/include/llvm/CodeGen/Register.h Outdated Show resolved Hide resolved

addressed review comments.

7b2fcda

mgudim marked this pull request as ready for review October 21, 2025 21:03

mgudim changed the title ~~Negative frame indicies as register.~~ [CodeGen] Negative frame indicies as register. Oct 21, 2025

arsenm reviewed Oct 22, 2025

View reviewed changes

llvm/include/llvm/CodeGen/Register.h Outdated Show resolved Hide resolved

llvm/include/llvm/CodeGen/Register.h Outdated Show resolved Hide resolved

Renamed "FirstStackSlot" to "StackSlotZero".

478c945

Simplified the definition of "StackSlotMask" Fixed wrong assertion. Added unit test.

topperc changed the title ~~[CodeGen] Negative frame indicies as register.~~ [CodeGen] Allow negative frame indicies in Register class. Oct 22, 2025

topperc reviewed Oct 22, 2025

View reviewed changes

llvm/include/llvm/CodeGen/Register.h Outdated Show resolved Hide resolved

topperc reviewed Oct 22, 2025

View reviewed changes

Mikhail Gudim added 2 commits October 22, 2025 11:51

addressed review comments.

695baef

address review comments.

29bf5cd

topperc reviewed Oct 22, 2025

View reviewed changes

llvm/include/llvm/CodeGen/Register.h Outdated Show resolved Hide resolved

topperc reviewed Oct 22, 2025

View reviewed changes

llvm/unittests/CodeGen/RegisterTest.cpp Outdated Show resolved Hide resolved

arsenm reviewed Oct 23, 2025

View reviewed changes

addressed review comments.

a871d1f

mgudim force-pushed the neg_indices branch from 30ed5de to a871d1f Compare October 23, 2025 14:28

topperc reviewed Oct 23, 2025

View reviewed changes

llvm/include/llvm/CodeGen/Register.h Outdated Show resolved Hide resolved

topperc reviewed Oct 23, 2025

View reviewed changes

llvm/unittests/CodeGen/RegisterTest.cpp Outdated Show resolved Hide resolved

address review comments.

e6a016a

topperc reviewed Oct 27, 2025

View reviewed changes

ppenzin reviewed Oct 31, 2025

View reviewed changes

[CodeGen] Allow negative frame indicies in Register class. #164459

Are you sure you want to change the base?

[CodeGen] Allow negative frame indicies in Register class. #164459

Uh oh!

Conversation

mgudim commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Oct 21, 2025

Uh oh!

github-actions bot commented Oct 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgudim commented Oct 21, 2025

Uh oh!

Uh oh!

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

topperc commented Oct 22, 2025

Uh oh!

Uh oh!

topperc Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

mgudim Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

mgudim Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgudim Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MatzeB commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topperc commented Oct 22, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgudim commented Oct 23, 2025

Uh oh!

topperc commented Oct 23, 2025

Uh oh!

Uh oh!

Uh oh!

mgudim commented Oct 27, 2025

Uh oh!

topperc Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

ppenzin Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

topperc Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mgudim commented Oct 21, 2025 •

edited

Loading

github-actions bot commented Oct 21, 2025 •

edited

Loading

mgudim Oct 22, 2025 •

edited

Loading

MatzeB commented Oct 22, 2025 •

edited

Loading