[PATCH] D127084: [MCA] Allow mca::Instruction-s to be recycled and reused

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Min,

I left a few comments below.

Thanks for the patch!

-Andrea



================
Comment at: llvm/include/llvm/MCA/IncrementalSourceMgr.h:87-114
+  void updateNext() override {
+    ++TotalCounter;
+    Instruction *I = Staging.front();
+    Staging.pop_front();
+    I->reset();
+
+    if (InstFreedCallback)
----------------
Could you please move these definitions to a IncrementalSourceMgr.cpp ?


================
Comment at: llvm/include/llvm/MCA/InstrBuilder.h:25
 #include "llvm/Support/Error.h"
+#include <system_error>
 
----------------
I really don't think that we should use system_error.



================
Comment at: llvm/include/llvm/MCA/InstrBuilder.h:74
 
-  Expected<const InstrDesc &> createInstrDescImpl(const MCInst &MCI);
-  Expected<const InstrDesc &> getOrCreateInstrDesc(const MCInst &MCI);
+  llvm::function_ref<Instruction *(const InstrDesc &)> InstRecycleCallback;
+
----------------
Would it be more readable if we defined that type with a `using InstRecycleCallback = ...`?

That way, you may not need to use decltype at line 101.




================
Comment at: llvm/include/llvm/MCA/Instruction.h:571
   bool isOptimizableMove() const { return IsOptimizableMove; }
-  void setOptimizableMove() { IsOptimizableMove = true; }
+  void setOptimizableMove(bool Val = true) { IsOptimizableMove = Val; }
   bool isMemOp() const { return MayLoad || MayStore; }
----------------
Personally, I prefer that we don't touch that method. What about adding a 
`void clearOptimizableMove()` ?



================
Comment at: llvm/include/llvm/MCA/Instruction.h:647-657
+  void reset() {
+    // Note that this won't clear read/write descriptors
+    // or other non-trivial fields
+    Stage = IS_INVALID;
+    CyclesLeft = UNKNOWN_CYCLES;
+    setOptimizableMove(false);
+    RCUTokenID = 0;
----------------
Please move this definition to Instruction.cpp


================
Comment at: llvm/lib/MCA/InstrBuilder.cpp:542-544
+InstrBuilder::createInstrDescImpl(const MCInst &MCI, bool &StaticDesc) {
   assert(STI.getSchedModel().hasInstrSchedModel() &&
          "Itineraries are not yet supported!");
----------------
I wonder if instead we should add a bitfield `bool IsRecyclable : 1` to `struct InstrDesc`.

By default, it would be set to false.

That way, you don't need to change these signatures, and you can still retrieve that information automatically. It might make the rest of the logic a bit more readable/self documenting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127084