[PATCH] D59928: [MCA] Add an experimental MicroOpQueue stage.

[Matt Davis via Phabricator via llvm-commits]

mattd added a comment.

LGTM!



================
Comment at: include/llvm/MCA/Stages/MicroOpQueueStage.h:1
+//===---------------------- MicroOpQueue.h ----------------------*- C++ -*-===//
+//
----------------
nit: s/MicroOpQueue.h/MicroOpQueueStage.h/


================
Comment at: include/llvm/MCA/Stages/MicroOpQueueStage.h:16
+
+#ifndef LLVM_MCA_BUFFER_STAGE_H
+#define LLVM_MCA_BUFFER_STAGE_H
----------------
nit: Perhaps rename the guard to "LLVM_MCA_MICRO_OP_QUEUE_STAGE" . I know that looks a little nasty, but it follows the header-guard convention.


================
Comment at: include/llvm/MCA/Stages/MicroOpQueueStage.h:51
+  // number of entries consumed by an instruction is normalized to the
+  // std::min(NumMicroOpcodes, Buffer.size()). This is to avoid problems with
+  // (microcoded) instructions that generate a number of micro opcodes than
----------------
Unnecessary-pedantic nit: Your `std::min` call at line 56 is written in a different order, with `std::min(Buffer.size(), NumMicroOpcodes)`


================
Comment at: include/llvm/MCA/Stages/MicroOpQueueStage.h:88
+
+#endif // LLVM_MCA_FETCH_STAGE_H
----------------
nit: Copy-pasta comment.  Now I know what your favorite stage is.


================
Comment at: lib/MCA/Context.cpp:60
+  if (Opts.MicroOpQueueSize)
+    StagePipeline->appendStage(std::move(llvm::make_unique<MicroOpQueueStage>(
+        Opts.MicroOpQueueSize, Opts.DecodersThroughput)));
----------------
Do you need a std::move here?  


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

https://reviews.llvm.org/D59928