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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Thanks Matt,



================
Comment at: tools/llvm-mca/InstrBuilder.h:62
 
-  std::unique_ptr<Instruction> createInstruction(unsigned Idx,
-                                                 const llvm::MCInst &MCI);
+  std::unique_ptr<Instruction> createInstruction(const llvm::MCInst &MCI);
 };
----------------
mattd wrote:
> andreadb wrote:
> > Please correctly rebase this patch. This change should not have been here.
> Sorry about that, I was just playing off of the previous patch for this that I was working on.
No problem. It happens :-)


================
Comment at: tools/llvm-mca/Instruction.h:60-68
+  void print(llvm::raw_ostream &OS) const { OS << getSourceIndex(); }
+};
+
+/// A utility routine for dumping InstRef data to an output stream. This is
+/// primarily used for debugging.
+inline llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const InstRef &IR) {
+  IR.print(OS);
----------------
mattd wrote:
> andreadb wrote:
> > This code should be guarded against NDEBUG.
> > Everywhere else in the codebase we use method `dump()` to do debug prints. You should do the same, and move the definition in Instruction.cpp.
> I agree with the NDEBUG.
> 
> This overload is just providing a convenient way to pass an InstRef to an output stream.  We were already passing index values to our debug output.  Since we are now passing around InstRef in most places, I figured it would be nice to overload the output operator for InstRef, so that our existing debug prints would not have to call "getSourceIndex."
Makes sense. Thanks.


================
Comment at: tools/llvm-mca/RetireControlUnit.cpp:31
 // Reserves a number of slots, and returns a new token.
-unsigned RetireControlUnit::reserveSlot(Instruction &IS, unsigned NumMicroOps) {
+unsigned RetireControlUnit::reserveSlot(InstRef *IR, unsigned NumMicroOps) {
   assert(isAvailable(NumMicroOps));
----------------
mattd wrote:
> andreadb wrote:
> > Why a pointer? I think InstRef should always be passed by reference or by copy.
> > This diff doesn't match Upstream! The signature should have been different. reserveSlot accepts an instruction index, and not an instruction reference!
> Did you want to keep the original definition of reserveSlot, by taking an index?   Since we are simplifying the interface where most routines take an InstRef, I think passing a reference would make sense, as it is later used for event notification.  I was hoping to remove all of the Owner->getInstruction() lookups. 
That’s okay. I was just confused by the presence of a pointer.


================
Comment at: tools/llvm-mca/RetireControlUnit.h:50
   struct RUToken {
-    Instruction *IS;
+    InstRef IR;
     unsigned NumSlots; // Slots reserved to this instruction.
----------------
mattd wrote:
> andreadb wrote:
> > It was an unsigned integer before. Now it is an InstRef. Please explain why?
> The reason is that the token is eventually used to signal that a particular instruction has retired, the call to notifyInstructionRetired in RetireControlUnit::cycleEvent.  I was trying to avoid storing an index, since the notification mechanism would have to eventually perform a lookup of that index to get the instruction pointer.  I was trying to decouple eventing from the component containing all of the instruction instances, I do realize that RCU will probably manage ownership of all Instructions at some point.  I was trying to avoid having one component query another MCA component to get an instruction.  We already have the instruction when we generate the RUToken values, so my thought was just to pass that information instead.  
Thanks for the explanation. Makes sense :-).


https://reviews.llvm.org/D46367