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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb marked 5 inline comments as done.
andreadb added inline comments.


================
Comment at: include/llvm/MCA/Stages/MicroOpQueueStage.h:1
+//===---------------------- MicroOpQueue.h ----------------------*- C++ -*-===//
+//
----------------
mattd wrote:
> nit: s/MicroOpQueue.h/MicroOpQueueStage.h/
Thanks for catching it. I will fix it.


================
Comment at: include/llvm/MCA/Stages/MicroOpQueueStage.h:16
+
+#ifndef LLVM_MCA_BUFFER_STAGE_H
+#define LLVM_MCA_BUFFER_STAGE_H
----------------
mattd wrote:
> 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.
You are absolutely right.
The original name of that stage was BufferStage. But then I opted for MicroOpQueueStage. Apparently I forgot to find&replace all the occurrences of buffer stage in this code...


================
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
----------------
mattd wrote:
> Unnecessary-pedantic nit: Your `std::min` call at line 56 is written in a different order, with `std::min(Buffer.size(), NumMicroOpcodes)`
.. or I can just write "minimum between NumMicroOpcodes and the buffer size".


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



================
Comment at: lib/MCA/Context.cpp:60
+  if (Opts.MicroOpQueueSize)
+    StagePipeline->appendStage(std::move(llvm::make_unique<MicroOpQueueStage>(
+        Opts.MicroOpQueueSize, Opts.DecodersThroughput)));
----------------
mattd wrote:
> Do you need a std::move here?  
I'll double check before committing it. Probably not...


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

https://reviews.llvm.org/D59928