Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CodeGen] Preserve LiveStack analysis in StackSlotColoring pass #94967

Closed

Conversation

vg0204
Copy link
Contributor

@vg0204 vg0204 commented Jun 10, 2024

This PR originated as a follow-up requirement from #93668.

For some target architectures, stack slot coloring pass may be invoked multiple times. It can occur due to the split of RA pass, opening up the opportunity to optimize stack slots usage after each RA invocation.

In order to achieve that if we could keep livestack analysis alive uptil the invocation of the RA pass for the last time in pipeline, it would save up the overhead of recomputing live stack analysis after each RA. Thus, livestack result requires it to be preserved at stack slot coloring pass which basically optimizes stack slots, but not makes the needed updates in live stack results. This has been achieved here in this patch.

Copy link

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.

@vg0204 vg0204 self-assigned this Jun 10, 2024
@vg0204 vg0204 force-pushed the vg0204/preserve-livestack-in-stackslotcoloring branch from 6a69b4d to e13e0f8 Compare June 10, 2024 14:30
For some target architectures, stack slot coloring pass may be invoked
multiple times. It can occur due to the split of RA pass, opening up
the opportunity to optimize stack slots usage at each RA invocation.

In order to achieve that if we could keep stack analysis alive
uptil the invocation of the RA pass for the last time, it will save
up the overhead of recomputing live stack analysis after each RA.
This requires it to be preserved at stack slot coloring pass which
basically optimizes stack slots, but not makes the needed updates
in live stack results. This has been achieved here.
@llvmbot
Copy link
Collaborator

llvmbot commented Jun 10, 2024

@llvm/pr-subscribers-llvm-regalloc

Author: Vikash Gupta (vg0204)

Changes

This PR originated as a follow-up requirement from #93668.

For some target architectures, stack slot coloring pass may be invoked multiple times. It can occur due to the split of RA pass, opening up the opportunity to optimize stack slots usage after each RA invocation.

In order to achieve that if we could keep livestack analysis alive uptil the invocation of the RA pass for the last time in pipeline, it would save up the overhead of recomputing live stack analysis after each RA. Thus, livestack result requires it to be preserved at stack slot coloring pass which basically optimizes stack slots, but not makes the needed updates in live stack results. This has been achieved here in this patch.


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

1 Files Affected:

  • (modified) llvm/lib/CodeGen/StackSlotColoring.cpp (+18)
diff --git a/llvm/lib/CodeGen/StackSlotColoring.cpp b/llvm/lib/CodeGen/StackSlotColoring.cpp
index 9fdc8a338b52a..7bf966b21ebc1 100644
--- a/llvm/lib/CodeGen/StackSlotColoring.cpp
+++ b/llvm/lib/CodeGen/StackSlotColoring.cpp
@@ -149,6 +149,7 @@ namespace {
       AU.addRequired<SlotIndexes>();
       AU.addPreserved<SlotIndexes>();
       AU.addRequired<LiveStacks>();
+      AU.addPreserved<LiveStacks>();
       AU.addRequired<MachineBlockFrequencyInfo>();
       AU.addPreserved<MachineBlockFrequencyInfo>();
       AU.addPreservedID(MachineDominatorsID);
@@ -411,6 +412,23 @@ bool StackSlotColoring::ColorSlots(MachineFunction &MF) {
     }
   }
 
+  // In order to preserve LiveStack analysis, the live ranges for dead spill
+  // stack slots would be merged with the live range of those stack slots that
+  // now share the spill object of the mentioned dead stack slot.
+  for (unsigned SS = 0, SE = SlotMapping.size(); SS != SE; ++SS) {
+    int NewFI = SlotMapping[SS];
+    if (SlotMapping[SS] == -1 || (NewFI == (int)SS)) {
+      continue;
+    }
+
+    LiveRange &lrToUpdateInto =
+        static_cast<LiveRange &>(LS->getInterval(NewFI));
+    const LiveRange &lrToUpdateFrom =
+        static_cast<LiveRange &>(LS->getInterval((int)SS));
+    lrToUpdateInto.MergeSegmentsInAsValue(lrToUpdateFrom,
+                                          lrToUpdateInto.getValNumInfo(0));
+  }
+
   return true;
 }
 

}

LiveRange &lrToUpdateInto =
static_cast<LiveRange &>(LS->getInterval(NewFI));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't need this cast?

Comment on lines 428 to 429
lrToUpdateInto.MergeSegmentsInAsValue(lrToUpdateFrom,
lrToUpdateInto.getValNumInfo(0));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use join.

@vg0204
Copy link
Contributor Author

vg0204 commented Jun 11, 2024

This implementation is not as straightforward as it seems, as my current solution does not cover lot of corner cases. Also, I found that eventhough not stack slot re-usage may occur but stack slot coloring effectively re-assigns stack slots, which is currently like any change not updated in live stack. As a result, for example if a piece of code has 100 Stack slots objects all of which are just being re-assigned to new SS index supposedly (with no SS index sharing possible), this has to be propagated in LS results, which I feel is equivalent to creating entire new LS. Besides if there occur stack slot share, those all need to collected (could be any number), join their live ranges as well map them to newly assigned stack slot index, as it is very possible that all stack slots being shared themselves might get SS index re-assigned.

@vg0204
Copy link
Contributor Author

vg0204 commented Jun 11, 2024

So, considering all this I am just refelcting whether its worthwhile to preserve liveStacks, as once used by SCC in middle, it may be of no use to later upcoming RA passes(as they themselves enter their own stack spills livenes to be later maybe used by SCC). So one probable solution in my suggestion could be reset the LS results.

@vg0204 vg0204 requested review from arsenm and kparzysz June 12, 2024 05:14
// efforts to rectify which is equivalent to do it all over again. Also,
// this current results would not be used later, so better to clear it &
// preserve analysis.
LS->releaseMemory();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're just going to throw away the result there's no point in claiming this is preserved, since it's a lie

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But, currently LiveStack analysis really is doing nothing just creating a empty shell result, & real work is happening at RA to compute and update livestack result. Also, once generated at RA pass, spill entries in LS will later be used at stackSlotColoring pass, followed by not being used ever again (LS later be used during SCC pass coming after later RA passes, but at that point RA is concerned with newly introduced spill records in LS by itself). So, my point is that preserving LS won't help out in any way. And LS analyis itself is doing no computation, so its re-occurence should not have impact on performance.

@arsenm
Copy link
Contributor

arsenm commented Jun 13, 2024

Should close this then?

@vg0204
Copy link
Contributor Author

vg0204 commented Jun 14, 2024

Should close this then?

Yes, It should be as It truly does not makes thing effective anyway.

@arsenm arsenm closed this Jun 14, 2024
@vg0204 vg0204 deleted the vg0204/preserve-livestack-in-stackslotcoloring branch July 1, 2024 09:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants