[PATCH] D94928: [llvm-mca] Add support for in-order CPUs

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

In D94928#2596996 <https://reviews.llvm.org/D94928#2596996>, @asavonic wrote:

> In D94928#2568386 <https://reviews.llvm.org/D94928#2568386>, @andreadb wrote:
>
>> Now that https://reviews.llvm.org/D95954 has been fixed, there are only a few things left to do:
>>
>> 1. Disable the bottleneck analysis for in-order processors.
>> 2. Add a couple of lines to the release notes describing this new feature.
>> 3. Update the llvm-mca docs.
>
> Thank you! Can you please check if wording in the docs is ok?
>
> Regarding the bottleneck analysis: it seems to work for some cases
> (A55-all-views.s), but not for others (A55-out-of-order-retire.s).
> I will check what is wrong here. The feature is disabled for now as you
> suggested.

Thanks Andrew for the updated patch!

The doc changes look good to me.

The code changes also look good, except for one thing (I left a comment below).

About the bottleneck analysis:
The current analysis assumes an out-of-order pipeline. So I wouldn't be surprised if there are cases where it doesn't work well.
As you might already know, the bottleneck analysis effectively observes pressure events which are dynamically generated by a Scheduler component. Based on the observed trace, and it the predicts if throughput was somehow limited by the lack of processor resources, and/or data dependencies.
The good thing about your patch is that your code already generates pressure events (from your new `canExecute()` method). A bottleneck analysis for in-order processors would be much simpler, since we don't need any complex analysis based on the knowledge of which instructions are in a PENDING/READY state in a scheduler buffer.

The best thing to do for now is to raise an llvm-mca bug about implementing a bottleneck analysis for in-order processors. You can raise it now, or wait until this patch is finally committed (either way is fine).



================
Comment at: llvm/lib/MCA/Stages/InOrderIssueStage.cpp:83
+        // Try again in the next cycle
+        return 1;
+      }
----------------
As I wrote before, you should simply ignore the case where the number of cycles left is equal to UNKNOWN_CYCLES.
You shouldn't early exit with an arbitrary number of cycles.

Basically what I am saying is that you should replace this return statement with a `continue;`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94928