[CodeGen] Preserved additional analyses in StackSlotColoring pass. #93779

vg0204 · 2024-05-30T07:47:58Z

The pass pipeline of some architecture splits register allocation phase based on different register classes. As some analyses need to be computed at the beginning of the register allocation and kept alive till all values are assigned to some physical registers.

This poses challenge with objective of introducing StackSlotColoring after partial virtual registers are assigned to physical registers, in order to optimize stack slots usage.As this pass doesn't preserve few analysis yet to be needed by the register allocation of the remaining virtual registers, necessiating them to be kept preserved.

github-actions · 2024-05-30T07:48:13Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

github-actions · 2024-05-30T07:51:03Z

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

The pass pipeline of some architecture splits register allocation phase based on different register classes. As some analyses need to be computed at the beginning of the register allocation and kept alive till all values are assigned to some physical registers. This poses challenge with objective of introducing StackSlotColoring after partial virtual registers are assigned to physical registers, in order to optimize stack slots usage.As this pass doesn't preserve few analysis yet to be needed by the register allocation of the remaining virtual registers, necessiating them to be kept preserved.

llvm/lib/CodeGen/StackSlotColoring.cpp

cdevadas · 2024-06-03T08:47:41Z

Don't merge this patch now. Wait for the other patch to get approved.

vg0204 · 2024-06-03T09:47:02Z

e this patch now. Wait for the other patch to get ap

But, the other patch depends upon this. Unless this changes are not present there, many of the existing test cases would fail there, including the one I added. So it won't get approved.

vg0204 · 2024-06-05T08:11:41Z

Did anymore change or any wait is needed (maybe because of dependency on other patch) to merge this PR.

arsenm · 2024-06-06T09:59:39Z

Don't merge this patch now. Wait for the other patch to get approved.

Don't see a reason to wait on that, this is fine standalone

arsenm · 2024-06-06T10:00:41Z

llvm/lib/CodeGen/StackSlotColoring.cpp

@@ -13,6 +13,7 @@
 #include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
+#include "llvm/CodeGen/LiveDebugVariables.h"


Could probably avoid the include by using the PassID

For that, "char &llvm::passID = pass::ID", need to be added in the respective pass (not presently there in LiveDebugVariables.cpp), followed by exposing it via extern in Passes.h. So, I think this is better option.

But those should be there

As I was going through the Codegen passes, while most of them have it, others don't. What should be the right practice?

They all should (but the PassID will go away in new PM so it's probably not worth fixing all of the missing instances)

So, what should be supposed to be my final take on it!

llvm/lib/CodeGen/StackSlotColoring.cpp

arsenm · 2024-06-06T10:02:54Z

llvm/lib/CodeGen/StackSlotColoring.cpp

+      // split into multiple phases based on register class. So, this pass
+      // may be invoked multiple times requiring it to save these analyses to be
+      // used by RA later.
+      AU.addPreserved<LiveIntervals>();


Do you need to addPreserved<SlotIndexes>? I'm also surprised you don't need to refer to LiveIntervals to maintain them?

"addPreserved < SlotIndexes > " was already there before. As for LiveIntervals, it would be needed to be preserved for maybe the upcoming regAlloc pass for remaining register classes, else it is being re-computed, which somehow creating problem.

llvmbot · 2024-06-06T10:03:35Z

@llvm/pr-subscribers-llvm-regalloc

Author: Vikash Gupta (vg0204)

Changes

The pass pipeline of some architecture splits register allocation phase based on different register classes. As some analyses need to be computed at the beginning of the register allocation and kept alive till all values are assigned to some physical registers.

This poses challenge with objective of introducing StackSlotColoring after partial virtual registers are assigned to physical registers, in order to optimize stack slots usage.As this pass doesn't preserve few analysis yet to be needed by the register allocation of the remaining virtual registers, necessiating them to be kept preserved.

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

1 Files Affected:

(modified) llvm/lib/CodeGen/StackSlotColoring.cpp (+14-1)

diff --git a/llvm/lib/CodeGen/StackSlotColoring.cpp b/llvm/lib/CodeGen/StackSlotColoring.cpp
index 9fdc8a338b52a..eb7d730e236bf 100644
--- a/llvm/lib/CodeGen/StackSlotColoring.cpp
+++ b/llvm/lib/CodeGen/StackSlotColoring.cpp
@@ -13,6 +13,7 @@
 #include "llvm/ADT/BitVector.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
+#include "llvm/CodeGen/LiveDebugVariables.h"
 #include "llvm/CodeGen/LiveInterval.h"
 #include "llvm/CodeGen/LiveIntervalUnion.h"
 #include "llvm/CodeGen/LiveIntervals.h"
@@ -64,6 +65,7 @@ namespace {
     MachineFrameInfo *MFI = nullptr;
     const TargetInstrInfo *TII = nullptr;
     const MachineBlockFrequencyInfo *MBFI = nullptr;
+    SlotIndexes *Indexes = nullptr;
 
     // SSIntervals - Spill slot intervals.
     std::vector<LiveInterval*> SSIntervals;
@@ -152,6 +154,14 @@ namespace {
       AU.addRequired<MachineBlockFrequencyInfo>();
       AU.addPreserved<MachineBlockFrequencyInfo>();
       AU.addPreservedID(MachineDominatorsID);
+
+      // In some Target's pipeline, register allocation (RA) might be
+      // split into multiple phases based on register class. So, this pass
+      // may be invoked multiple times requiring it to save these analyses to be
+      // used by RA later.
+      AU.addPreserved<LiveIntervals>();
+      AU.addPreserved<LiveDebugVariables>();
+
       MachineFunctionPass::getAnalysisUsage(AU);
     }
 
@@ -496,8 +506,10 @@ bool StackSlotColoring::RemoveDeadStores(MachineBasicBlock* MBB) {
     ++I;
   }
 
-  for (MachineInstr *MI : toErase)
+  for (MachineInstr *MI : toErase) {
     MI->eraseFromParent();
+    Indexes->removeMachineInstrFromMaps(*MI);
+  }
 
   return changed;
 }
@@ -515,6 +527,7 @@ bool StackSlotColoring::runOnMachineFunction(MachineFunction &MF) {
   TII = MF.getSubtarget().getInstrInfo();
   LS = &getAnalysis<LiveStacks>();
   MBFI = &getAnalysis<MachineBlockFrequencyInfo>();
+  Indexes = &getAnalysis<SlotIndexes>();
 
   bool Changed = false;

llvm/lib/CodeGen/StackSlotColoring.cpp

arsenm · 2024-06-10T07:40:49Z

llvm/lib/CodeGen/StackSlotColoring.cpp

@@ -496,8 +506,12 @@ bool StackSlotColoring::RemoveDeadStores(MachineBasicBlock* MBB) {
    ++I;
  }

-  for (MachineInstr *MI : toErase)
+  for (MachineInstr *MI : toErase) {
+    if (Indexes) {


Don't need braces

github-actions · 2024-06-11T06:40:58Z

@vg0204 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

…lvm#93779) The pass pipeline of some architecture splits register allocation phase based on different register classes. As some analyses need to be computed at the beginning of the register allocation and kept alive till all values are assigned to some physical registers. This poses challenge with objective of introducing StackSlotColoring after partial virtual registers are assigned to physical registers, in order to optimize stack slots usage.As this pass doesn't preserve few analysis yet to be needed by the register allocation of the remaining virtual registers, necessiating them to be kept preserved.

This PR is dependent on [#93779](#93779). As currently, each SGPR Spills are lowered to go into distinct stack slots in stack frame after SGPR allocation phase. Therefore, this patch utilizes the capability of StackSlotColoring to ensure the stack slot sharing if possible for stack frame index, where the SGPR spills are occuring in the non-interfering region. StackSlotColoring is introduced immediately after SGPR register allocation, just to ensure that any further lowering happens on the optimally allocated stack slots, with certain flags to indicate the preservation of certain analysis result later to be used by RA of other register classes.

This PR is dependent on [llvm#93779](llvm#93779). As currently, each SGPR Spills are lowered to go into distinct stack slots in stack frame after SGPR allocation phase. Therefore, this patch utilizes the capability of StackSlotColoring to ensure the stack slot sharing if possible for stack frame index, where the SGPR spills are occuring in the non-interfering region. StackSlotColoring is introduced immediately after SGPR register allocation, just to ensure that any further lowering happens on the optimally allocated stack slots, with certain flags to indicate the preservation of certain analysis result later to be used by RA of other register classes.

vg0204 self-assigned this May 30, 2024

vg0204 force-pushed the vg0204/preserve-analysis-in-StackSlotColoring branch from 6ea80f6 to dc21436 Compare May 30, 2024 07:55

vg0204 mentioned this pull request May 30, 2024

[AMDGPU]Optimize SGPR spills #93668

Merged

vg0204 requested review from arsenm and cdevadas May 30, 2024 09:17

arsenm reviewed May 30, 2024

View reviewed changes

llvm/lib/CodeGen/StackSlotColoring.cpp Outdated Show resolved Hide resolved

Add recommended changes addressing the grammatical issue with comments.

c8079d7

cdevadas approved these changes Jun 3, 2024

View reviewed changes

vg0204 requested a review from arsenm June 3, 2024 11:31

arsenm reviewed Jun 6, 2024

View reviewed changes

arsenm added the llvm:regalloc label Jun 6, 2024

arsenm requested changes Jun 6, 2024

View reviewed changes

llvm/lib/CodeGen/StackSlotColoring.cpp Outdated Show resolved Hide resolved

arsenm approved these changes Jun 10, 2024

View reviewed changes

vg0204 force-pushed the vg0204/preserve-analysis-in-StackSlotColoring branch from 0c526ed to c56007d Compare June 10, 2024 16:13

vg0204 added 2 commits June 10, 2024 21:45

Addressed requested changes after review in StackSlotColoring.cpp.

c56007d

Removed unnecessary braces from stackSlotColoring.cpp

eb410b8

arsenm approved these changes Jun 10, 2024

View reviewed changes

vg0204 merged commit e9a3623 into llvm:main Jun 11, 2024
7 checks passed

HerrCai0907 mentioned this pull request Jun 13, 2024

tidy #95384

Closed

vg0204 deleted the vg0204/preserve-analysis-in-StackSlotColoring branch August 6, 2024 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CodeGen] Preserved additional analyses in StackSlotColoring pass. #93779

[CodeGen] Preserved additional analyses in StackSlotColoring pass. #93779

vg0204 commented May 30, 2024

github-actions bot commented May 30, 2024

github-actions bot commented May 30, 2024 •

edited

Loading

cdevadas commented Jun 3, 2024

vg0204 commented Jun 3, 2024 •

edited

Loading

vg0204 commented Jun 5, 2024

arsenm commented Jun 6, 2024

arsenm Jun 6, 2024

vg0204 Jun 6, 2024 •

edited

Loading

arsenm Jun 6, 2024

vg0204 Jun 6, 2024

arsenm Jun 6, 2024

vg0204 Jun 6, 2024

arsenm Jun 6, 2024

vg0204 Jun 6, 2024 •

edited

Loading

llvmbot commented Jun 6, 2024

arsenm Jun 10, 2024

github-actions bot commented Jun 11, 2024

[CodeGen] Preserved additional analyses in StackSlotColoring pass. #93779

[CodeGen] Preserved additional analyses in StackSlotColoring pass. #93779

Conversation

vg0204 commented May 30, 2024

github-actions bot commented May 30, 2024

github-actions bot commented May 30, 2024 • edited Loading

cdevadas commented Jun 3, 2024

vg0204 commented Jun 3, 2024 • edited Loading

vg0204 commented Jun 5, 2024

arsenm commented Jun 6, 2024

arsenm Jun 6, 2024

Choose a reason for hiding this comment

vg0204 Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

arsenm Jun 6, 2024

Choose a reason for hiding this comment

vg0204 Jun 6, 2024

Choose a reason for hiding this comment

arsenm Jun 6, 2024

Choose a reason for hiding this comment

vg0204 Jun 6, 2024

Choose a reason for hiding this comment

arsenm Jun 6, 2024

Choose a reason for hiding this comment

vg0204 Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

llvmbot commented Jun 6, 2024

arsenm Jun 10, 2024

Choose a reason for hiding this comment

github-actions bot commented Jun 11, 2024

github-actions bot commented May 30, 2024 •

edited

Loading

vg0204 commented Jun 3, 2024 •

edited

Loading

vg0204 Jun 6, 2024 •

edited

Loading

vg0204 Jun 6, 2024 •

edited

Loading