[PATCH] D46367: [llvm-mca] Avoid exposing index values in the MCA interfaces.

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

A few minor nits. Otherwise LGTM.

Thanks!



================
Comment at: tools/llvm-mca/HWEventListener.h:52
+  HWInstructionEvent(unsigned type, const InstRef &IR)
+      : Type(type), IR(IR) {}
 
----------------
Can you use a different name for the InstRef argument and the class field. Using the same name for both makes me feel a bit uncomfortable.


================
Comment at: tools/llvm-mca/Instruction.h:20
 #include "llvm/Support/MathExtras.h"
+#include "llvm/Support/raw_ostream.h"
 #include <memory>
----------------
I still think you should remove this include. It is only there because of method print, which should be also guarded against NDEBUG.


================
Comment at: tools/llvm-mca/Instruction.h:52
+
+  void print(llvm::raw_ostream &OS) const { OS << getSourceIndex(); }
+};
----------------
I think this should be guarded against NDEBUG.


================
Comment at: tools/llvm-mca/LSUnit.h:19
 
+#include "Instruction.h"
 #include "llvm/Support/Debug.h"
----------------
I think you can just forward declare InstRef inside namespace mca. You probably don't need to include the entire "Instruction.h". Uses of InstRef are all in the cpp file (that file includes already Instruction.h).


https://reviews.llvm.org/D46367