[PATCH] D150310: [TableGen][SubtargetEmitter] Add the StartAtCycles field in the WriteRes class.

[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.

Hi Francesco,

Thanks for contributing this patch!

In the past, I always wanted to add the ability to model "delayed resource consumption". However, I never got the time to actually do it.
On x86, we could use `StartAtCycle` to fix scheduling information for horizontal operations. We could also use it for improving instructions with folded loads (for which, we use read-advance entries).

In the past, the lack of a proper mechanism for describing delayed resource consumption was identified as the main reason why sometimes llvm-mca produced poor quality perf reports.
I remember I discussed this issue with Andrew Trick a couple of times.
Eventually, he suggested that we implemented something very similar to your `StartAtCycle` (see https://lists.llvm.org/pipermail/llvm-dev/2020-May/141487.html).
I believe that this (and the rest of your patch set) will fix issue: https://github.com/llvm/llvm-project/issues/45218

In future, more work will be needed to teach llvm-mca and exegesis how to correctly handle `StartAtCycle` information (if available).
However, that would be a separate task :).

Long story short: your approach looks good to me and it similar to what was suggested in the past by others.

Patch looks good to me (modulo a few minor nits - see below).

Cheers,
-Andrea



================
Comment at: llvm/include/llvm/MC/MCSchedule.h:62
 /// Identify one of the processor resource kinds consumed by a particular
 /// scheduling class for the specified number of cycles.
 struct MCWriteProcResEntry {
----------------
This code comment should describe (in a sentence or two) what is `StartAtCycle` used for.


================
Comment at: llvm/tools/llvm-exegesis/lib/SchedClassResolution.cpp:86
         SM.getProcResource(WPR->ProcResourceIdx);
+    assert(WPR->StartAtCycle == 0 && "`llvm-exegesis` does not handle StartAtCycle > 0");
     if (ProcResDesc->SubUnitsIdxBegin == nullptr) {
----------------
Is there a ticket for this?


================
Comment at: llvm/utils/TableGen/SubtargetEmitter.cpp:1196-1197
           WPREntry.Cycles = Cycles[PRIdx];
+          WPREntry.StartAtCycle = StartAtCycles[PRIdx];
+          if (StartAtCycles[PRIdx] > Cycles[PRIdx]) {
+            PrintFatalError(WriteRes->getLoc(),
----------------
Should we also check for negative values? Not that I expect people to use negative delay cycles.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150310