Interleave loop pre-header with SWP prologue #236

gbossu · 2024-11-14T14:31:16Z

Better review commit by commit, a lot of them are NFC and prepare for the final commit which enables interleaving.

QoR shows only improvements. Those will get bigger as we increase the scope of the post-pipeliner.

| Core_Compute_Cycle_Count       | ReduceProdAxis_1_aie2_bf16 | ReduceProdAxis_2_aie2_bf16 | ReduceProdAxis_4_aie2_bf16 | ReduceMeanAxis_2_aie2_bf16 | ReduceMeanAxis_4_aie2_bf16 | ReduceMeanAxis_1_aie2_bf16 | Sigmoid_bf16_0 | Requantize_0 | Softmax_bf16_0 | ReduceMeanAxis_3_aie2_bf16 | ReduceMeanAxis_6_aie2_bf16 | ReduceMeanAxis_5_aie2_bf16 | ReduceSumAxis_2_aie2_bf16 | Sigmoid_bf16_1 | Exp_bf16_0   | ReduceSumAxis_4_aie2_bf16 | ReduceSumAxis_1_aie2_bf16 | Log_bf16_0   | Neg_aie2_1   | EleMax_aie2_bfloat16 | EleMin_aie2_bfloat16 | ReduceMeanAxis_7_aie2_bf16 | Requantize_1 | MulAttributeBroadcasting_aie2_int8_0 | Tanh_1       | ReduceProdAxis_6_aie2_bf16 | ReduceSumAxis_5_aie2_bf16 | ReduceSumAxis_3_aie2_bf16 | ReduceSumAxis_6_aie2_bf16 | GELU_1       | HardSigmoid_bf16_0 | ReduceProdAxis_3_aie2_bf16 | ReduceProdAxis_5_aie2_bf16 | Exp_bf16_1   | ReduceProdAxis_7_aie2_bf16 | ReduceSumAxis_7_aie2_bf16 | Tanh_0       | GELU_0       | InstanceNormPart2_aie2_bf16_0 | MulBroadcasting_aie2_0 | HardSigmoid_bf16_1 | EleMax_aie2_int8 | EleMin_aie2_int8 | BatchNorm1d_aie2_bfloat16 | Mul_aie2_0   | HardSigmoid_int8_1 | HardSigmoid_int8_0 | BatchNorm1d_aie2_int8 | BitwiseNot_aie2_0 | BatchNorm2D_1 | GEMM_int8_0  | GEMM_int8_1  | Softmax_1    | BatchNorm2D_0 | Average diff |
| ------------------------------ | -------------------------- | -------------------------- | -------------------------- | -------------------------- | -------------------------- | -------------------------- | -------------- | ------------ | -------------- | -------------------------- | -------------------------- | -------------------------- | ------------------------- | -------------- | ------------ | ------------------------- | ------------------------- | ------------ | ------------ | -------------------- | -------------------- | -------------------------- | ------------ | ------------------------------------ | ------------ | -------------------------- | ------------------------- | ------------------------- | ------------------------- | ------------ | ------------------ | -------------------------- | -------------------------- | ------------ | -------------------------- | ------------------------- | ------------ | ------------ | ----------------------------- | ---------------------- | ------------------ | ---------------- | ---------------- | ------------------------- | ------------ | ------------------ | ------------------ | --------------------- | ----------------- | ------------- | ------------ | ------------ | ------------ | ------------- | ------------ |
| baseline                       | 35466                      | 17821                      | 35498                      | 13076                      | 13046                      | 13040                      | 2627           | 1421         | 8176           | 7233                       | 7219                       | 7212                       | 11900                     | 1727           | 7872         | 11920                     | 11884                     | 4149         | 455          | 227                  | 227                  | 6263                       | 781          | 517                                  | 2578         | 8709                       | 7055                      | 7052                      | 7038                      | 2811         | 937                | 8722                       | 8730                       | 1531         | 1794                       | 6212                      | 1970         | 2144         | 9536                          | 294                    | 649                | 164              | 164              | 390                       | 231          | 427                | 417                | 408                   | 135               | 416           | 2797         | 32931        | 570          | 308           | +0.00%       |
| this PR                        | 35405                      | 17789                      | 35433                      | 13051                      | 13016                      | 13010                      | 2620           | 1417         | 8144           | 7204                       | 7190                       | 7183                       | 11852                     | 1720           | 7840         | 11871                     | 11835                     | 4131         | 453          | 226                  | 226                  | 6231                       | 777          | 514                                  | 2560         | 8647                       | 7004                      | 7001                      | 6987                      | 2790         | 930                | 8656                       | 8662                       | 1519         | 1778                       | 6156                      | 1952         | 2123         | 9440                          | 291                    | 642                | 162              | 162              | 385                       | 228          | 421                | 411                | 402                   | 132               | 406           | 2701         | 31779        | 550          | 296           | -0.15%       |
| Total diff                     | IMPR(-0.17%)               | IMPR(-0.18%)               | IMPR(-0.18%)               | IMPR(-0.19%)               | IMPR(-0.23%)               | IMPR(-0.23%)               | IMPR(-0.27%)   | IMPR(-0.28%) | IMPR(-0.39%)   | IMPR(-0.40%)               | IMPR(-0.40%)               | IMPR(-0.40%)               | IMPR(-0.40%)              | IMPR(-0.41%)   | IMPR(-0.41%) | IMPR(-0.41%)              | IMPR(-0.41%)              | IMPR(-0.43%) | IMPR(-0.44%) | IMPR(-0.44%)         | IMPR(-0.44%)         | IMPR(-0.51%)               | IMPR(-0.51%) | IMPR(-0.58%)                         | IMPR(-0.70%) | IMPR(-0.71%)               | IMPR(-0.72%)              | IMPR(-0.72%)              | IMPR(-0.72%)              | IMPR(-0.75%) | IMPR(-0.75%)       | IMPR(-0.76%)               | IMPR(-0.78%)               | IMPR(-0.78%) | IMPR(-0.89%)               | IMPR(-0.90%)              | IMPR(-0.91%) | IMPR(-0.98%) | IMPR(-1.01%)                  | IMPR(-1.02%)           | IMPR(-1.08%)       | IMPR(-1.22%)     | IMPR(-1.22%)     | IMPR(-1.28%)              | IMPR(-1.30%) | IMPR(-1.41%)       | IMPR(-1.44%)       | IMPR(-1.47%)          | IMPR(-2.22%)      | IMPR(-2.40%)  | IMPR(-3.43%) | IMPR(-3.50%) | IMPR(-3.51%) | IMPR(-3.90%)  | -0.15%       |

This makes the option available for any user of getMinTripCount()

andcarminati · 2024-11-14T14:37:44Z

llvm/lib/Target/AIE/Utils/AIELoopUtils.cpp

@@ -29,7 +37,11 @@ const MDNode *getLoopID(const MachineBasicBlock &LoopBlock) {
 }

 std::optional<int64_t> getMinTripCount(const MachineBasicBlock &LoopBlock) {
-  return getMinTripCount(getLoopID(LoopBlock));
+  std::optional<int64_t> MinTripCount = getMinTripCount(getLoopID(LoopBlock));


This makes a lot of sense ;-)

andcarminati · 2024-11-14T14:45:41Z

llvm/test/CodeGen/AIE/aie2/schedule/postpipeliner/interleave-prologue.mir

@@ -0,0 +1,422 @@
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py


Clever! I like TDD. It also helps to see from where you start and how the work progresses with the sequence of commits.

llvm/lib/Target/AIE/AIEMaxLatencyFinder.cpp

andcarminati · 2024-11-15T11:03:40Z

llvm/lib/Target/AIE/AIEMaxLatencyFinder.cpp

+  for (MachineBasicBlock *SuccBB : MBB.successors()) {
+    auto &SBS = Sched.getInterBlock().getBlockState(SuccBB);
+    assert(SBS.isScheduled());
+    if (SBS.getRegions().empty()) {


I am curious about how rare this can be. If this can be frequent, we can take the successor's successor to reduce the pessimism.

andcarminati · 2024-11-15T11:06:56Z

Hi @gbossu, as we did before, we could integrate the 1st and 3rd commits before, as they are standalone changes.

gbossu · 2024-11-15T11:20:45Z

Hi @gbossu, as we did before, we could integrate the 1st and 3rd commits before, as they are standalone changes.

I'm actually tempted to drop the 3rd commit and do not have AIEMaxLatencyFinder/RegionEndEdges care about "timed"/"fixed" instructions. I think it makes sense if they keep focusing on successor MBBs instead. For dependencies between "fixed" and "free" instructions, I can add them in the EmitFixedSUnits mutator instead. Any opinion?

andcarminati · 2024-11-15T12:15:29Z

Hi @gbossu, as we did before, we could integrate the 1st and 3rd commits before, as they are standalone changes.

I'm actually tempted to drop the 3rd commit and do not have AIEMaxLatencyFinder/RegionEndEdges care about "timed"/"fixed" instructions. I think it makes sense if they keep focusing on successor MBBs instead. For dependencies between "fixed" and "free" instructions, I can add them in the EmitFixedSUnits mutator instead. Any opinion?

Is this case, I think it makes sense, as we are introducing this new mutator! In this way we can also restrict a bit more the changes.

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

andcarminati · 2024-11-15T12:32:42Z

llvm/lib/Target/AIE/AIEInterBlockScheduling.h

+  }
+  size_t getNumTopFixedInstructions() const { return NumTopFixedInstructions; }
+
+  /// Iterate over the instructions that are fixed at the top. Typically those


nit: at the bot

andcarminati · 2024-11-15T13:20:02Z

llvm/lib/Target/AIE/AIEMachineScheduler.cpp

+  if (Zone.isTop())
+    return nullptr;
+  const Region &Reg = InterBlock.getBlockState(CurMBB).getCurrentRegion();
+  unsigned NumEmitted = getNumEmittedInstrs(DAG, /*IsTop=*/false);


nit: const unsigned

Those iterators are left with garbage values when a function has multiple regions. This can confuse DAGMutators.

llvm/lib/CodeGen/ScheduleDAGInstrs.cpp

martien-de-jong · 2024-11-18T11:02:08Z

llvm/lib/Target/AIE/AIEBaseSubtarget.cpp

@@ -543,11 +543,10 @@ class WAWEdges : public ScheduleDAGMutation {
    LiveRegs.init(*TRI);
    bool AddReservedRegs = true;
    if (Scheduler) {
+      assert(!Scheduler->doMBBSchedRegionsTopDown());


CHECK: this assert is specific for the prologue handling, and the code needs to be extended for epilogue handling.

No, WAW handling could run on any region. Here I'm making sure that BS.getBottom() actually represents the bottom region.

andcarminati · 2024-11-18T11:07:39Z

llvm/lib/CodeGen/ScheduleDAGInstrs.cpp

@@ -596,6 +596,10 @@ std::optional<unsigned> ScheduleDAGInstrs::initSUnit(MachineInstr &MI) {
  if (MI.isDebugOrPseudoInstr())
    return {};

+  // Ensure all instructions can be given a SUnit without re-allocation.


I was thinking if we could do this in ScheduleDAGInstrs::enterRegion to remove this fixed cost from here. What do you think?

That would also be fine, the reason I kept it here is to avoid distributing the logic about creating SUnits in too many places. But I'm open to discussion :)

@martien-de-jong Would you find this better if I do the initialization in ScheduleDAGInstrs::enterRegion then?

I guess so. I was happy that initSUnit didn't need to care. At the time I hadn't thought about adding SUnits after edge creation. If we can pre-reserve the total number of sunits with some interface I'm fine.

martien-de-jong · 2024-11-18T11:17:38Z

llvm/lib/Target/AIE/AIEBaseSubtarget.cpp

-      auto Region = BS.getCurrentRegion();
-      auto BottomRegion = BS.getBottom();
-      if (*Region.begin() == *BottomRegion.begin()) {
+      if (&BS.getCurrentRegion() == &BS.getBottom()) {


CHECK: this a relic of the pre-GatheringRegions era

Not really, the DAGMutator can run for all types of regions. For non-loop blocks, we do not pre-gather regions.

Hmm. now you have me worried about reallocation of the region array. pre-gather would imply pre-allocation, making the test 'obviously correct'

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

martien-de-jong · 2024-11-18T11:32:47Z

llvm/lib/Target/AIE/AIEInterBlockScheduling.h

+
+    // Verify that all fixed instructions are at the right place in the MBB
+    assert(TopFixedBundles.empty() || Begin == BB->begin());
+    assert(TopFixedBundles.empty() ||


I like it. && at top level in an assert should be split in different asserts.

martien-de-jong · 2024-11-18T11:35:35Z

llvm/lib/Target/AIE/AIEInterBlockScheduling.h

+                    return getBundleStart(MI->getIterator()) == FreeEnd;
+                  }));
+
+    for (auto It = FreeBegin; It != FreeEnd; ++It) {
      SemanticOrder.push_back(&*It);


Maybe mention whether and where fixed instructions are added to the semantic order.

ok. They aren't. And I know why.

andcarminati · 2024-11-18T12:22:55Z

llvm/lib/Target/AIE/AIEHazardRecognizer.cpp

@@ -441,6 +461,12 @@ ScheduleHazardRecognizer::HazardType AIEHazardRecognizer::getHazardType(
    iterator_range<const MachineOperand *> MIOperands,
    const MachineRegisterInfo &MRI, int DeltaCycles) const {
  const unsigned SchedClass = TII->getSchedClass(Desc, MIOperands, MRI);
+  if (!IsPreRA && SchedClass == 0 &&


This verification could be refactored out to a helper function like check... or validate..., that can be used also below (same code).

martien-de-jong · 2024-11-18T12:35:27Z

llvm/lib/Target/AIE/AIEInterBlockScheduling.h

  }
-  size_t size() const { return SemanticOrder.size(); }
+  ArrayRef<MachineBundle> getBotFixedBundles() const { return BotFixedBundles; }
+  size_t getNumBotFixedInstructions() const { return BotFixedBundles.size(); }


nit: NumBotFixedBundles to be absolutely clear?

I'm actually thinking of deleting that function. One can just do Region.getBotFixedBundles().size(). Previously it was harder because I only had iterators, and std::distance isn't O(1)

llvm/lib/Target/AIE/AIEMachineScheduler.cpp

llvm/lib/Target/AIE/AIEHazardRecognizer.cpp

gbossu · 2024-11-18T12:50:45Z

llvm/lib/Target/AIE/AIEBaseSubtarget.cpp

@@ -318,6 +305,49 @@ class RegionEndEdges : public ScheduleDAGMutation {
  };
 };

+class EmitFixedSUnits : public ScheduleDAGMutation {


I could probably add a top-level comment for that mutator

andcarminati · 2024-11-18T12:52:26Z

llvm/lib/Target/AIE/AIEBaseSubtarget.cpp

@@ -318,6 +305,49 @@ class RegionEndEdges : public ScheduleDAGMutation {
  };
 };

+class EmitFixedSUnits : public ScheduleDAGMutation {


nit: for the future, we could have a high-level description of the mutator.

llvm/lib/Target/AIE/AIEBaseSubtarget.cpp

martien-de-jong · 2024-11-18T12:54:40Z

llvm/lib/Target/AIE/AIEHazardRecognizer.cpp

+    if (auto BundleStart = getBundleStart(Instr->getIterator());
+        BundleStart->isBundle()) {
+      assert(BundleStart != Instr->getIterator());
+      BundleStart->eraseFromBundle();


I'm wondering when this occurs, and whether we should not remove BUNDLEs as a precondition to calling this. Apart from that, I wonder whether there's anybody caring about the format order apart from the MClayer.

This happens when the prologue has VLIW instructions. They get materialized as BUNDLE. I could separate that "debundling" in a helper function if you want.

The ordering is mostly for stable ref updates at this point. Otherwise slight changes to the tryCandidate heuristics can be annoying, because they would change the order of instructions within bundles.

Ok. ref order is a good reason. Maybe my main confusion is that this function should live somewhere else.

It should. But I blame AIE1 ;)

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp

llvm/lib/Target/AIE/AIEBaseSubtarget.cpp

llvm/lib/Target/AIE/AIEMaxLatencyFinder.h

llvm/lib/Target/AIE/AIEMachineScheduler.cpp

martien-de-jong · 2024-11-18T13:51:41Z

llvm/lib/Target/AIE/AIEMachineScheduler.cpp

+  assert(!(IsTop && FirstBotFixedSU) && "Top-fixed SUnits must be added first");
+  assert(DAG->SUnits.size() < DAG->SUnits.capacity() &&
+         "SUnits need to be re-allocated.");
+  unsigned SUNum = DAG->initSUnit(MI).value();


nit: ignoring has_value() here.

That would be an error if the value is missing

and we don't like asserts? :-b

I mean that I would get to the same behaviour: the error would be caught and not silently ignored.

andcarminati · 2024-11-19T13:45:21Z

llvm/lib/Target/AIE/AIEMachineScheduler.cpp

+           SU.NodeNum < FirstBotFixedSU.value_or(DAG->SUnits.size());
+  }
+  return FirstBotFixedSU && SU.NodeNum >= *FirstBotFixedSU &&
+         SU.NodeNum <= LastBotFixedSU.value();


nit: SU.NodeNum <= *LastBotFixedSU (to be symmetric with the previous case) or FirstBotFixedSU.value().

This is on purpose to have dynamic checks on LastBotFixedSU (and have a runtime failure if there is no value). For FirstBotFixedSU, I previously checked has_value(), so there is no need for checking again.

Fixed instructions will be handled specifically by the scheduler. They cannot be reordered or inserted in different cycles. Note this commit is just an "interface change". The list of "fixed" instructions is still empty at the moment, and SWP prologues/epilogues are inserted with a specific logic in commitBlockSchedule().

This updates the scheduler code base so it can handle SUnits that are BUNDLE. This particularly focuses on/ - Properly emitting BUNDLEs in the scoreboard - Rebundling instructions that are in the same cycle. The commit also adds a bunch of asserts to verify "hidden" pre-conditions. I had to add some Itineraries in AIE1/AIE2 to make sure those conditions hold.

This means they will be placed by the scheduler, and once they are placed, other "free" instructions can be bundled with them.

andcarminati · 2024-11-20T08:36:25Z

llvm/lib/Target/AIE/AIEInterBlockScheduling.h

+  /// Those should not be re-ordered by the scheduler.
+  ArrayRef<MachineBundle> TopFixedBundles;
+
+  /// Instructions that are already scheduled at the bottom, e.g. an swp


nit: a swp prologue.

gbossu · 2024-11-21T15:11:50Z

Any more comments @andcarminati @martien-de-jong ?

andcarminati · 2024-11-22T09:55:41Z

llvm/lib/Target/AIE/AIEBaseSubtarget.cpp

-              PatchCycles >= InsertedCycles ? PatchCycles - InsertedCycles : 0;
-        }
-        EdgeLatency = std::max(EdgeLatency, PatchCycles);
+        const unsigned ZOLDistance = 8;


For the future: this magic 8 could be retrieved from TII as well.

andcarminati · 2024-11-22T10:22:06Z

llvm/lib/Target/AIE/AIEBaseSubtarget.cpp

+        CurRegion.getBotFixedBundles();
+    for (SUnit &FreeSU : make_filter_range(DAG->SUnits, IsFreeSU)) {
+      const MachineInstr &MI = *FreeSU.getInstr();
+      MachineInstr *FixedDepMI =


super nit:

To help readability, we could extend with overloaded cast operators:

struct InstrAndCycle { MachineInstr *MI = nullptr; int Cycle; + operator MachineInstr *() const { return MI; } + operator int() const { return Cycle; } };

Then we can just use:

MachineInstr *FixedDepMI = AIE::findEarliestRef(MI, BotFixedBundles, BotFixedBundles.size());

And also:

Earliest = findEarliestRef(MI, TopBundles, Earliest);

andcarminati · 2024-11-22T10:24:38Z

llvm/lib/Target/AIE/AIEBundle.h

+
+    // Keep track of BUNDLE instructions. They need to be cleaned up when
+    // de-bundling before re-bundling. See applyBundles()
+    if (Instr->getOpcode() == TargetOpcode::BUNDLE) {


super nit: Instr->isBundle()

andcarminati · 2024-11-22T13:36:28Z

llvm/lib/Target/AIE/AIEMachineScheduler.cpp

+  unsigned SUNum = DAG->initSUnit(MI).value();
+  SUnit &SU = DAG->SUnits[SUNum];
+
+  if (IsTop) {


It is really smart that you are preparing the way for epilogue merging (in some small details) ;-)

andcarminati · 2024-11-22T14:37:31Z

Hi @gbossu, I finished my review and left few suggestions. Nice piece of engineering that will make a lot more difference for the post-swp as we increase its use.

[AIEX] NFC: Move --aie-loop-min-tripcount option to AIELoopUtils

ed1b571

This makes the option available for any user of getMinTripCount()

gbossu requested review from abhinay-anubola, abnikant, andcarminati, khallouh, konstantinschwarz, martien-de-jong, SagarMaheshwari99 and stephenneuendorffer as code owners November 14, 2024 14:31

andcarminati reviewed Nov 14, 2024

View reviewed changes

gbossu commented Nov 14, 2024

View reviewed changes

llvm/lib/Target/AIE/AIEMaxLatencyFinder.cpp Outdated Show resolved Hide resolved

andcarminati reviewed Nov 14, 2024

View reviewed changes

llvm/lib/Target/AIE/AIEMaxLatencyFinder.cpp Outdated Show resolved Hide resolved

andcarminati reviewed Nov 14, 2024

View reviewed changes

llvm/lib/Target/AIE/AIEMaxLatencyFinder.cpp Outdated Show resolved Hide resolved

andcarminati reviewed Nov 15, 2024

View reviewed changes

andcarminati closed this Nov 15, 2024

gbossu reopened this Nov 15, 2024

andcarminati reviewed Nov 15, 2024

View reviewed changes

llvm/lib/Target/AIE/AIEInterBlockScheduling.cpp Outdated Show resolved Hide resolved

andcarminati reviewed Nov 15, 2024

View reviewed changes

[CodeGen][NFC] MachineScheduler: Properly initialize top() and bottom()

9dcbff6

Those iterators are left with garbage values when a function has multiple regions. This can confuse DAGMutators.

gbossu force-pushed the gaetan.merge.preheader.swp-prologue branch from dd15264 to d4c8472 Compare November 18, 2024 10:38