GT: Implement live-range covering (lengthened covered by shortened), plus relaxations #65

Quincunx271 · 2020-04-05T15:49:29Z

This PR implements live-range covering for the static superiority graph transformations, and it also relaxes the rules to apply graph transformations.

The relaxations:

Recursive successor requirement:
- Old: if B uses R which is not used by A, then R is used by C which is recursive successor of B.
- New: C is a recursive successor of A. (remember that RcrsvScsr(B) is a subset of RcrsvScsr(A)).
  This is valid because the point is to prove that R is used after the "critical" region, and A is at the end of said region.
Covering requirements:
- We can cover with defs, not just with uses. I implemented this by instead counting the net number of registers whose live ranges were lengthened, subtracting off those whose live ranges were shortened. This yields simpler code overall: 4 vectors became 1.

This has been tested on the fp part of CPU2006. Without history domination and with PERP, there are no mismatches.

lib/Scheduler/graph_trans.cpp

Quincunx271 · 2020-04-10T17:41:34Z

Perhaps this last commit should be its own PR

kerbowa · 2020-04-13T04:41:26Z

Perhaps this last commit should be its own PR

Yes that would make it easier to review. They would definitely ask for that on reviews.llvm.org

kerbowa · 2020-04-13T04:47:00Z

include/opt-sched/Scheduler/sched_basic_data.h

+  int16_t NumUses() { return useCnt_; }
+
+  llvm::ArrayRef<Register *> GetDefs() {
+    return {defs_, static_cast<size_t>(defCnt_)};


This has the same issue as before right? It just extends it to other parts of the code. I thought we could make the underlying datastrcuture (defs_,uses_) a vector. Is this a lot of work to implement? I know it's in list from #47.

Otherwise I can just review the graphtrans portion of the change without this latest commit.

I am planning to pull this commit out into its own PR; I just haven't gotten around to that yet.

What do you mean by "same issue as before"? This doesn't copy the underlying datastructure at all. It's just a pointer + size. This also allows the underlying datastructure to be converted into a vector<Register *> without any change for users. I did not want to implement conversion to a vector yet because it's tricky to ensure the change doesn't break anything.

Okay this makes sense to me.

Quincunx271 · 2020-04-13T15:44:13Z

I pulled that commit into its own branch (https://github.com/Quincunx271/OptSched/tree/refactor-getusedef-arrayref). I'll PR that once this PR gets merged

kerbowa · 2020-04-23T08:15:30Z

Sorry, I'll review this as I get a chance.

kerbowa · 2020-04-27T15:54:59Z

lib/Scheduler/graph_trans.cpp

+  Register **uses;
+  const int useCount = node->GetUses(uses);
+  assert(useCount >= 0);
+  return {uses, static_cast<size_t>(useCount)};


Just wondering, does makeArrayRef work here?

Indeed it would. It has an overload which calls the same constructor that I'm calling here.

kerbowa

LGTM

I wonder about live out values. I believe the exit node will be counted as a user of live-out vregs. If B is the last user in the region of a live-out value should it matter from reg-alloc POV?

Quincunx271 · 2020-05-03T16:04:02Z

@kerbowa I want to make sure I understand what you're saying.

If we have a live-out vreg R, meaning that Exit uses R and thus should be its last user, if we also have an instruction B which uses R and is the last such instruction before Exit?

R in (Uses(B) intersect Uses(Exit)) and B is the last use of R except for Exit.

I don't see how that can affect things for this graph transformation. We'll always find Exit as a node C which also uses R.

kerbowa · 2020-05-03T22:09:46Z

I don't see how that can affect things for this graph transformation. We'll always find Exit as a node C which also uses R.

That's what I mean. I was wondering if live-out values might need special consideration, and should "affect things". For example what if the live range is split and we are spilling the second half of the interval. Could it be better to schedule B first in that situation? This most likely falls out of the realm of what the scheduler should be expected to know when calculating register pressure. Whether live-out values should have special consideration is something that has wider implications. I have no issues with this PR.

Allow a live range to be lengthened, if a corresponding range is shortened. Some refactoring: - Pull out logic to find possible last uses of a node into its own function. - Wrap GetUses and GetDefs to return an llvm::SmallVector - Use algorithms where possible

For the hypothetical schedule considering nodes A and B, where B appears before A, given a register R used by B but not by A, there must be a node C such that: The old condition was: C in RcrsvScsr(B) (and hence C in RcrsvScsr(A) because of an earlier RcrsvScsr(B) is a subset of RcrsvScsr(A)). We can relax this requirement: C in RcrsvScsr(A). The goal is to show that such a C must exist after A, given the hypothetical: [..., B, ..., A, ...]. If C in RcsrvScsr(A) and C is not A, then the hypothetical must actually be [..., B, ..., A, ..., C, ...], meaning that we're good and can proceed.

We can cover lengthened live ranges based on defines, not just uses. The easiest way to account for this is to change the way of thinking about it: (# lengthened) - (# shortened) <= 0 meets our optimality condition.

We don't need to copy the uses/defs. Instead, we use an llvm::ArrayRef.

Quincunx271 requested a review from kerbowa April 5, 2020 15:49

kerbowa reviewed Apr 10, 2020

View reviewed changes

lib/Scheduler/graph_trans.cpp Outdated Show resolved Hide resolved

lib/Scheduler/graph_trans.cpp Outdated Show resolved Hide resolved

lib/Scheduler/graph_trans.cpp Outdated Show resolved Hide resolved

lib/Scheduler/graph_trans.cpp Outdated Show resolved Hide resolved

Quincunx271 force-pushed the gt-relax-recursive-successor-requirements branch from 9fafd82 to 9be050c Compare April 10, 2020 16:34

kerbowa reviewed Apr 13, 2020

View reviewed changes

Quincunx271 force-pushed the gt-relax-recursive-successor-requirements branch 2 times, most recently from 442b86d to 9c1f567 Compare April 13, 2020 15:40

Quincunx271 requested a review from kerbowa April 13, 2020 15:41

kerbowa reviewed Apr 27, 2020

View reviewed changes

kerbowa approved these changes May 3, 2020

View reviewed changes

Quincunx271 added 5 commits May 4, 2020 08:40

Use algorithms rather than raw loops

02affd8

[GT]: Relax covering requirements

fdbcda0

We can cover lengthened live ranges based on defines, not just uses. The easiest way to account for this is to change the way of thinking about it: (# lengthened) - (# shortened) <= 0 meets our optimality condition.

[GT]: Use ArrayRef rather than SmallVector

fb7ed87

We don't need to copy the uses/defs. Instead, we use an llvm::ArrayRef.

Quincunx271 force-pushed the gt-relax-recursive-successor-requirements branch from 9c1f567 to fb7ed87 Compare May 4, 2020 15:41

Quincunx271 merged commit 6c7c0a8 into CSUS-LLVM:master May 18, 2020

Quincunx271 deleted the gt-relax-recursive-successor-requirements branch May 18, 2020 16:50

Quincunx271 mentioned this pull request May 18, 2020

Improve GetUses() / GetDefs() to return ArrayRef #82

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GT: Implement live-range covering (lengthened covered by shortened), plus relaxations #65

GT: Implement live-range covering (lengthened covered by shortened), plus relaxations #65

Quincunx271 commented Apr 5, 2020

Quincunx271 commented Apr 10, 2020

kerbowa commented Apr 13, 2020

kerbowa Apr 13, 2020

Quincunx271 Apr 13, 2020

kerbowa Apr 13, 2020

Quincunx271 commented Apr 13, 2020

kerbowa commented Apr 23, 2020

kerbowa Apr 27, 2020

Quincunx271 Apr 27, 2020 •

edited

Loading

kerbowa left a comment

Quincunx271 commented May 3, 2020

kerbowa commented May 3, 2020 •

edited

Loading

GT: Implement live-range covering (lengthened covered by shortened), plus relaxations #65

GT: Implement live-range covering (lengthened covered by shortened), plus relaxations #65

Conversation

Quincunx271 commented Apr 5, 2020

Quincunx271 commented Apr 10, 2020

kerbowa commented Apr 13, 2020

kerbowa Apr 13, 2020

Choose a reason for hiding this comment

Quincunx271 Apr 13, 2020

Choose a reason for hiding this comment

kerbowa Apr 13, 2020

Choose a reason for hiding this comment

Quincunx271 commented Apr 13, 2020

kerbowa commented Apr 23, 2020

kerbowa Apr 27, 2020

Choose a reason for hiding this comment

Quincunx271 Apr 27, 2020 • edited Loading

Choose a reason for hiding this comment

kerbowa left a comment

Choose a reason for hiding this comment

Quincunx271 commented May 3, 2020

kerbowa commented May 3, 2020 • edited Loading

Quincunx271 Apr 27, 2020 •

edited

Loading

kerbowa commented May 3, 2020 •

edited

Loading