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

[Matt Davis via Phabricator via llvm-commits]

mattd marked 5 inline comments as done.
mattd added a comment.

Thanks for the review.   Sorry for building on the previous patch, I should have rebased against ToT.



================
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);
 };
----------------
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.


================
Comment at: tools/llvm-mca/Instruction.h:41
+public:
+  InstRef() : std::pair<unsigned, Instruction *>(0, nullptr) {}
+  InstRef(unsigned Index, Instruction *I)
----------------
andreadb wrote:
> What's the advantage in having this constructor? I think you can remove it.
> All the instruction references should be constructed using the other constructor at line 42.
I was allowing for the case of storing of InstRef objects in a container, not by reference or pointer.  This would only be for the RCU's queue.  But perhaps we remove this default ctor, and just store an Instruction pointer in the RUToken.  I've discussed this more in a comment below.


================
Comment at: tools/llvm-mca/Instruction.h:48
+  Instruction *getInstruction() {
+    assert(isValid() && "Invalid InstRef instance, not initialized.");
+    return second;
----------------
andreadb wrote:
> Not needed. It is okay to return nullptr.
Ok, I'll remove it.  I was trying to detect any states that should not arise, but I wanted to guarantee that they did not occur.  If, for instance, an InstRef was constructed with a nullptr value for the Instruction.  Or some default construction never getting populated.  But, as I mentioned in a comment above, and as you suggested, we can get rid of the default ctor.  But that is somewhat dependent on how we define the RUToken, which is discussed in a different comment below.


================
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);
----------------
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."


================
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));
----------------
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. 


================
Comment at: tools/llvm-mca/RetireControlUnit.cpp:57
     assert(Current.NumSlots && "Reserved zero slots?");
+    assert(Current.IR.isValid() && "Invalid RUToken in the RCU queue.");
     if (!Current.Executed)
----------------
andreadb wrote:
> Can this ever happen?
I'm not sure, so I figured it was a reasonable assertion to make.  


================
Comment at: tools/llvm-mca/RetireControlUnit.h:50
   struct RUToken {
-    Instruction *IS;
+    InstRef IR;
     unsigned NumSlots; // Slots reserved to this instruction.
----------------
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.  


================
Comment at: tools/llvm-mca/RetireControlUnit.h:77
   // Reserves a number of slots, and returns a new token.
-  unsigned reserveSlot(Instruction &IS, unsigned NumMicroOps);
+  unsigned reserveSlot(InstRef *IS, unsigned NumMicroOps);
 
----------------
andreadb wrote:
> This is incorrect. Regardless of the wrong diff.
I agree, it shouldn't be a pointer.  I'm not sure why I changed that, other than it was how things fell out from the previous patch.   Sorry.  I'll fix that. 



https://reviews.llvm.org/D46367