[PATCH] D99339: [MCA] Support carry-over instructions for in-order processors

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb accepted this revision.
andreadb added a comment.
This revision is now accepted and ready to land.

LGTM modulo a few nits (see comments below).



================
Comment at: llvm/lib/MCA/Stages/InOrderIssueStage.cpp:266-268
+  if (Desc.EndGroup)
+    Bandwidth = 0;
+
----------------
This check is not needed if `ShouldCarryOver` is true. It can be safely moved inside the the above else block (at like 261).


================
Comment at: llvm/lib/MCA/Stages/InOrderIssueStage.cpp:333-356
+  // Continue to issue the instruction carried over from the previous cycle
+  if (CarriedOver) {
+    assert(Bandwidth > 0);
+    assert(CarryOver > 0);
+    assert(!StalledInst);
+    if (CarryOver <= Bandwidth) {
+      LLVM_DEBUG(dbgs() << "[E] Carry over (complete) #" << CarriedOver
----------------
Please move this to a separate function. Something like `updateCarriedOverInstruction()` (or a similar name..).


================
Comment at: llvm/lib/MCA/Stages/InOrderIssueStage.cpp:335-337
+    assert(Bandwidth > 0);
+    assert(CarryOver > 0);
+    assert(!StalledInst);
----------------
The first assert (i.e. `assert(Bandwidth > 0)`) can be safely removed. IssueWidth is supposed to always be bigger than 0. You would see other problems otherwise.

The second assert (i.e. `assert(CarryOver > 0)`) can also be removed because, by construction, it is an impossible situation if we take that if-stmt.

You can live the third assert, but please add a string comment to it.



================
Comment at: llvm/lib/MCA/Stages/InOrderIssueStage.cpp:339-340
+    if (CarryOver <= Bandwidth) {
+      LLVM_DEBUG(dbgs() << "[E] Carry over (complete) #" << CarriedOver
+                        << " \n");
+
----------------
Nit:  The `[E]` notation should only be used for real events.

I noticed this in previous patches too, but I didn't say anything because it was not so important. Also, those debug comments are very useful in general.

However, it would be nicer if we could use a different prefix for these new remarks; messages like this one are not strictly related to hardware events. It would be nicer if we consistently use a different char for it (maybe N - as in "Note", or R for "Remark"?) instead of `E`, which is supposed to be used as prefix for event-related debug messages.


================
Comment at: llvm/lib/MCA/Stages/InOrderIssueStage.cpp:352-353
+      Bandwidth = 0;
+      LLVM_DEBUG(dbgs() << "[E] Carry over (" << CarryOver << "cy left) #"
+                        << CarriedOver << " \n");
+    }
----------------
Same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99339