[PATCH] D150312: [MISched] Introduce and use ResourceSegments.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Francesco,

Apologies for the very late reply. I have been quite busy these days, and I am still trying to figure out mentally how well this new framework works in practice.

I am thinking about edge cases where writes of a same instruction somehow conflict in terms of resource cycles and/or `StartAtCycle` quantities.

If I understand it correctly, `StartAtCycle` forces the scheduling algorithm to find valid segments after that relative cycle. Scheduling won't fail if no segment can be allocated exactly at that cycle.
Basically, resource consumption is requested to start not before `StartAtCycle`. However, it is OK to start after `StartAtCycle` if slot allocation is unsuccessful.
Is that correct?

If so, then what happens if I declare the following write:

Write W = [ A (cycles=1, start_at=0), B (cycles=1, start_at=0), AB (cycles=3, start_at=1) ].

Where:
A and B are simple resource units (one unit each).
AB is a resource group containing A and B.

I want AB to be consumed after the first A and after the first B.

Ideally, I expect either this:

  ///       C      1      2      3      4      5  ...
  /// ------|------|------|------|------|------|----->
  /// 
  /// A    [C,                       C+4)  -- includes the extra cycle from AB.
  /// B    [C, C+1)

or

  ///       C      1      2      3      4      5  ...
  /// ------|------|------|------|------|------|----->
  /// 
  /// A    [C, C+1)
  /// B    [C,                       C+4)  -- includes the extra cycle from AB.

However, if resouce A is unavailable until cycle 2. Then what happens to the schedule?

  ///       C      1      2      3      4      5  ...
  /// ------|------|------|------|------|------|----->
  /// 
  /// A     X      X      X        [C+3, C+4)
  /// B    [C,  C+1)

At this point, when would AB be allocated? Could it be allocated to B from relative cycle 2?

I am asking this question because `StartAtCycle` could be used to describe unmodeled dependencies between multiple micro opcodes of a same instruction.
In that example, the user might have wanted to suggest that the consumption of group AB must start after A and B have been consumed for one cycle.

For example, an instruction may decode into 3 micro opcodes:

- opcode #1 consumes one cycle of resource A
- opcode #2 consumes one cycle of resource B
- opcode #3 waits for opcode #1 and #2 to complete, and then consumes three cycles of A or B.

If we use `StartAtCycle`, I am not sure if we can guarantee that consumption of "A or B" would always start at the right time.

NOTE: On x86, a similar pattern could be used to implement horizontal operations which are often microcoded and expanded into a pair of independent shuffle operations, followed by an ADD/SUB.

I wonder if there is a way to delay the start cycle until both A and B have been consumed.
Again, apologies if I misunderstood all of this behaviour.

I also wonder about what happens to those cases where a write triggers the consumption of extra cycles of so-called "super" resources. I suspect that "super" resources must be treated specially, and they should inherit the same `StartAtCycle`?

I have only suggested some minor changes (see my comments below).
I am curious to see how much scheduling improves if we start using `StartAtCycle` in our scheduling models. Do you have some perf numbers to share?

On x86 (SimonP can correct me on this) I believe that x86 still uses the old latency-based post-ra scheduling algorithm, which doesn't do any bookkeeping on hw resources.
I think that using `StartAtCycle` can significantly improve the quality of most x86 models (horizontal operations would benefit from it a lot). However, it would be nicer if it was possible to express a `StartAfter resource consumption`.



================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:691
+  /// reservation history of the instance of and individual resource.
+  struct ResourceSegments {
+  public:
----------------
I'd be tempted to just move your new ResourceSegments struct outside of SchedBoundary. Not sure what other reviewers think about it.
I feel like it would be slightly more readable all this part.
It was also suggested by Simon before. You might have missed his comment before.
Basically, this struct is big enough to be promoted to top level (mainly for readability reasons).


================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:692
+  struct ResourceSegments {
+  public:
+    /// Represents an interval of discrete integer values closed on
----------------
Was this meant to be public? If so, then it is redundant.



================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:864-866
+    static bool sort_predicate(const IntervalTy &A, const IntervalTy &B) {
+      return A.first < B.first;
+    }
----------------
Can this be an inline lambda used by the std::is_sorted at line 868?


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:2264
+          .getFirstAvailableAtFromBottom(CurrCycle, StartAtCycle, Cycles);
+    else
+      return ReservedResourceSegments[InstanceIdx].getFirstAvailableAtFromTop(
----------------
You don't need else after return.


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:2652-2661
+            if (isTop())
+              ReservedResourceSegments[InstanceIdx].add(
+                  SchedBoundary::ResourceSegments::getResourceIntervalTop(
+                      NextCycle, PI->StartAtCycle, PI->Cycles),
+                  MIResourceCutOff);
+            else
+              ReservedResourceSegments[InstanceIdx].add(
----------------
Not entirely sure about the coding style. However, I suggest to use braces for those blocks because statements are particularly long and use four lines...


================
Comment at: llvm/lib/CodeGen/MachineScheduler.cpp:4215
+  assert(CutOff > 0 && "0-size interval history has no use.");
+#ifndef NDEBUG
+  assert(all_of(_Intervals,
----------------
Don't need to check NDEBUG


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150312/new/

https://reviews.llvm.org/D150312