[llvm-mca] Add command line option -call-latency #92958

chinmaydd · 2024-05-21T19:52:33Z

Currently we assume a constant latency of 100 cycles for call instructions. This commit allows the user to specify a custom value for the same as a command line argument. Default latency is set to 100.

github-actions · 2024-05-21T19:52:53Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-05-21T19:53:23Z

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-tools-llvm-mca

Author: Chinmay Deshpande (chinmaydd)

Changes

Currently we assume a constant latency of 100 cycles for call instructions. This commit allows the user to specify a custom value for the same as a command line argument. Default latency is set to 100.

Full diff: https://github.com/llvm/llvm-project/pull/92958.diff

3 Files Affected:

(modified) llvm/include/llvm/MCA/InstrBuilder.h (+3)
(modified) llvm/lib/MCA/InstrBuilder.cpp (+9-8)
(modified) llvm/tools/llvm-mca/llvm-mca.cpp (+6)

diff --git a/llvm/include/llvm/MCA/InstrBuilder.h b/llvm/include/llvm/MCA/InstrBuilder.h
index 3594372489148..49ba8d93ec76f 100644
--- a/llvm/include/llvm/MCA/InstrBuilder.h
+++ b/llvm/include/llvm/MCA/InstrBuilder.h
@@ -78,6 +78,7 @@ class InstrBuilder {
 
   bool FirstCallInst;
   bool FirstReturnInst;
+  unsigned CallLatency;
 
   using InstRecycleCallback = std::function<Instruction *(const InstrDesc &)>;
   InstRecycleCallback InstRecycleCB;
@@ -111,6 +112,8 @@ class InstrBuilder {
   /// or null if there isn't any.
   void setInstRecycleCallback(InstRecycleCallback CB) { InstRecycleCB = CB; }
 
+  void setCallLatency(unsigned CL) { CallLatency = CL; }
+
   Expected<std::unique_ptr<Instruction>>
   createInstruction(const MCInst &MCI, const SmallVector<Instrument *> &IVec);
 };
diff --git a/llvm/lib/MCA/InstrBuilder.cpp b/llvm/lib/MCA/InstrBuilder.cpp
index bcf065c566918..e608fa520fc19 100644
--- a/llvm/lib/MCA/InstrBuilder.cpp
+++ b/llvm/lib/MCA/InstrBuilder.cpp
@@ -33,7 +33,7 @@ InstrBuilder::InstrBuilder(const llvm::MCSubtargetInfo &sti,
                            const llvm::MCInstrAnalysis *mcia,
                            const mca::InstrumentManager &im)
     : STI(sti), MCII(mcii), MRI(mri), MCIA(mcia), IM(im), FirstCallInst(true),
-      FirstReturnInst(true) {
+      FirstReturnInst(true), CallLatency(100U) {
   const MCSchedModel &SM = STI.getSchedModel();
   ProcResourceMasks.resize(SM.getNumProcResourceKinds());
   computeProcResourceMasks(STI.getSchedModel(), ProcResourceMasks);
@@ -220,17 +220,18 @@ static void initializeUsedResources(InstrDesc &ID,
 
 static void computeMaxLatency(InstrDesc &ID, const MCInstrDesc &MCDesc,
                               const MCSchedClassDesc &SCDesc,
-                              const MCSubtargetInfo &STI) {
+                              const MCSubtargetInfo &STI,
+                              unsigned CallLatency) {
   if (MCDesc.isCall()) {
     // We cannot estimate how long this call will take.
-    // Artificially set an arbitrarily high latency (100cy).
-    ID.MaxLatency = 100U;
+    // Artificially set an arbitrarily high latency (default: 100cy).
+    ID.MaxLatency = CallLatency;
     return;
   }
 
   int Latency = MCSchedModel::computeInstrLatency(STI, SCDesc);
-  // If latency is unknown, then conservatively assume a MaxLatency of 100cy.
-  ID.MaxLatency = Latency < 0 ? 100U : static_cast<unsigned>(Latency);
+  // If latency is unknown, then conservatively assume a MaxLatency set for calls (default: 100cy).
+  ID.MaxLatency = Latency < 0 ? CallLatency : static_cast<unsigned>(Latency);
 }
 
 static Error verifyOperands(const MCInstrDesc &MCDesc, const MCInst &MCI) {
@@ -568,7 +569,7 @@ InstrBuilder::createInstrDescImpl(const MCInst &MCI,
     // We don't correctly model calls.
     WithColor::warning() << "found a call in the input assembly sequence.\n";
     WithColor::note() << "call instructions are not correctly modeled. "
-                      << "Assume a latency of 100cy.\n";
+                      << "Assume a latency of " << CallLatency << "cy.\n";
     FirstCallInst = false;
   }
 
@@ -580,7 +581,7 @@ InstrBuilder::createInstrDescImpl(const MCInst &MCI,
   }
 
   initializeUsedResources(*ID, SCDesc, STI, ProcResourceMasks);
-  computeMaxLatency(*ID, MCDesc, SCDesc, STI);
+  computeMaxLatency(*ID, MCDesc, SCDesc, STI, CallLatency);
 
   if (Error Err = verifyOperands(MCDesc, MCI))
     return std::move(Err);
diff --git a/llvm/tools/llvm-mca/llvm-mca.cpp b/llvm/tools/llvm-mca/llvm-mca.cpp
index 03d7d7944b9cd..73b704283b8a8 100644
--- a/llvm/tools/llvm-mca/llvm-mca.cpp
+++ b/llvm/tools/llvm-mca/llvm-mca.cpp
@@ -135,6 +135,11 @@ static cl::opt<unsigned>
                                "(instructions per cycle)"),
                       cl::cat(ToolOptions), cl::init(0));
 
+static cl::opt<unsigned>
+    CallLatency("call-latency", cl::Hidden,
+                cl::desc("Number of cycles to assume for a call instruction"),
+                cl::cat(ToolOptions), cl::init(100U));
+
 enum class SkipType { NONE, LACK_SCHED, PARSE_FAILURE, ANY_FAILURE };
 
 static cl::opt<enum SkipType> SkipUnsupportedInstructions(
@@ -569,6 +574,7 @@ int main(int argc, char **argv) {
 
   // Create an instruction builder.
   mca::InstrBuilder IB(*STI, *MCII, *MRI, MCIA.get(), *IM);
+  IB.setCallLatency(CallLatency);
 
   // Create a context to control ownership of the pipeline hardware.
   mca::Context MCA(*MRI, *STI);

mshockwave

Thanks for sending the patch. Could you add a simple LIT test?

michaelmaitland · 2024-05-21T20:29:35Z

llvm/lib/MCA/InstrBuilder.cpp

@@ -33,7 +33,7 @@ InstrBuilder::InstrBuilder(const llvm::MCSubtargetInfo &sti,
                           const llvm::MCInstrAnalysis *mcia,
                           const mca::InstrumentManager &im)
    : STI(sti), MCII(mcii), MRI(mri), MCIA(mcia), IM(im), FirstCallInst(true),
-      FirstReturnInst(true) {
+      FirstReturnInst(true), CallLatency(100U) {


Should we pass CallLatency when we construct the InstrBuilder and remove the call to setCallLatency? This will simplify the number of places where we have to specify the default value to 100.

Thanks for pointing that out. I tried to address it.

michaelmaitland · 2024-05-21T22:57:34Z

llvm/lib/MCA/InstrBuilder.cpp

    return;
  }

  int Latency = MCSchedModel::computeInstrLatency(STI, SCDesc);
-  // If latency is unknown, then conservatively assume a MaxLatency of 100cy.
-  ID.MaxLatency = Latency < 0 ? 100U : static_cast<unsigned>(Latency);
+  // If latency is unknown, then conservatively assume a MaxLatency set for calls (default: 100cy).


nit: I think this line may be too long according to LLVM Coding Standards. Can you either format it accordingly or drop the default part since it no longer defaults to 100 inside InstrBuilder. I am content with either approach. If you end up going with the latter, please also remove the (default: 100cy) above on line 228.

Thanks for that. I should have checked the formatting guidelines before pushing.

michaelmaitland

LGTM. Thanks for the patch!

michaelmaitland · 2024-05-21T23:13:42Z

Please let me know if you would like my help in committing.

chinmaydd · 2024-05-21T23:14:48Z

Sure, that would be much appreciated. This is my first contribution to LLVM.

mshockwave

The CI reported build failures. Please update the InstrBuilder usage in https://github.com/llvm/llvm-project/blob/main/llvm/unittests/tools/llvm-mca/MCATestBase.cpp as well.

mshockwave · 2024-05-21T23:56:24Z

The CI reported build failures. Please update the InstrBuilder usage in https://github.com/llvm/llvm-project/blob/main/llvm/unittests/tools/llvm-mca/MCATestBase.cpp as well.

Or provide a default value to the new InstrBuilder ctor argument you added.

mshockwave · 2024-05-22T01:17:24Z

I think the CI is failing on the new call-latency.s test you added. It seems like there is a mismatch on the number of uOps so you might want to rerun update_mca_check.py again and/or rebase in case there is any change on uOps in the scheduling model.

Note that passing (pre-commit) CI is not a requirement for merging PR in LLVM but it'll be easier to address any issue early, otherwise 10+ (post-commit) buildbots might flood you with build failure emails after merge.

chinmaydd · 2024-05-22T01:19:57Z

Ah, thanks for pointing it out Min. I'll check again.

github-actions · 2024-05-22T10:33:50Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff dfdf1c5fe45a82b9c578306f3d7627fd251d63f8 3d13902620ea866688b6381983df835d9a12d4de -- llvm/include/llvm/MCA/InstrBuilder.h llvm/lib/MCA/InstrBuilder.cpp llvm/tools/llvm-mca/llvm-mca.cpp llvm/unittests/tools/llvm-mca/MCATestBase.cpp llvm/unittests/tools/llvm-mca/X86/TestIncrementalMCA.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/MCA/InstrBuilder.cpp b/llvm/lib/MCA/InstrBuilder.cpp
index 4d3b9d952d..d5cbdc5de0 100644
--- a/llvm/lib/MCA/InstrBuilder.cpp
+++ b/llvm/lib/MCA/InstrBuilder.cpp
@@ -31,8 +31,7 @@ InstrBuilder::InstrBuilder(const llvm::MCSubtargetInfo &sti,
                            const llvm::MCInstrInfo &mcii,
                            const llvm::MCRegisterInfo &mri,
                            const llvm::MCInstrAnalysis *mcia,
-                           const mca::InstrumentManager &im,
-                           unsigned cl)
+                           const mca::InstrumentManager &im, unsigned cl)
     : STI(sti), MCII(mcii), MRI(mri), MCIA(mcia), IM(im), FirstCallInst(true),
       FirstReturnInst(true), CallLatency(cl) {
   const MCSchedModel &SM = STI.getSchedModel();

RKSimon · 2024-05-22T10:47:40Z

llvm/lib/MCA/InstrBuilder.cpp

@@ -31,9 +31,10 @@ InstrBuilder::InstrBuilder(const llvm::MCSubtargetInfo &sti,
                           const llvm::MCInstrInfo &mcii,
                           const llvm::MCRegisterInfo &mri,
                           const llvm::MCInstrAnalysis *mcia,
-                           const mca::InstrumentManager &im)
+                           const mca::InstrumentManager &im,
+                           unsigned cl)


clang-format this

Currently we assume a constant latency of 100 cycles for a call instruction. This commit allows the user to specify a custom value for the same as a command line argument. We assume a value of 100 if none is provided.

mshockwave

LGTM. Thank you and congrats on your first PR!

github-actions · 2024-05-22T20:52:12Z

@chinmaydd Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvmbot added the tools:llvm-mca label May 21, 2024

mshockwave requested review from mshockwave and michaelmaitland May 21, 2024 20:04

mshockwave reviewed May 21, 2024

View reviewed changes

michaelmaitland reviewed May 21, 2024

View reviewed changes

llvmbot added the backend:X86 label May 21, 2024

michaelmaitland reviewed May 21, 2024

View reviewed changes

michaelmaitland approved these changes May 21, 2024

View reviewed changes

mshockwave requested changes May 21, 2024

View reviewed changes

chinmaydd force-pushed the mca-call branch from ea3f7d6 to 3d13902 Compare May 22, 2024 01:33

RKSimon requested a review from adibiagio May 22, 2024 10:30

RKSimon reviewed May 22, 2024

View reviewed changes

[llvm-mca] Add command line option -call-latency

346399c

Currently we assume a constant latency of 100 cycles for a call instruction. This commit allows the user to specify a custom value for the same as a command line argument. We assume a value of 100 if none is provided.

chinmaydd force-pushed the mca-call branch from 3d13902 to 346399c Compare May 22, 2024 15:57

mshockwave approved these changes May 22, 2024

View reviewed changes

mshockwave merged commit 848bef5 into llvm:main May 22, 2024
2 of 3 checks passed

chinmaydd deleted the mca-call branch June 6, 2024 04:00

[llvm-mca] Add command line option -call-latency #92958

[llvm-mca] Add command line option -call-latency #92958

Uh oh!

Conversation

chinmaydd commented May 21, 2024

Uh oh!

github-actions bot commented May 21, 2024

Uh oh!

llvmbot commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mshockwave left a comment

Choose a reason for hiding this comment

Uh oh!

michaelmaitland May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaydd May 21, 2024

Choose a reason for hiding this comment

Uh oh!

michaelmaitland May 21, 2024

Choose a reason for hiding this comment

Uh oh!

chinmaydd May 21, 2024

Choose a reason for hiding this comment

Uh oh!

michaelmaitland left a comment

Choose a reason for hiding this comment

Uh oh!

michaelmaitland commented May 21, 2024

Uh oh!

chinmaydd commented May 21, 2024

Uh oh!

mshockwave left a comment

Choose a reason for hiding this comment

Uh oh!

mshockwave commented May 21, 2024

Uh oh!

mshockwave commented May 22, 2024

Uh oh!

chinmaydd commented May 22, 2024

Uh oh!

github-actions bot commented May 22, 2024

Uh oh!

RKSimon May 22, 2024

Choose a reason for hiding this comment

Uh oh!

mshockwave left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented May 22, 2024

Uh oh!

Uh oh!

llvmbot commented May 21, 2024 •

edited

Loading

michaelmaitland May 21, 2024 •

edited

Loading