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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Matt,

This diff has not been taken against top of trunk. Could you please rebase the patch and then create a correct diff?
For example, class Instuction never had a field named `SourceIndex`!

Also, see my inline comments.

Thanks,
Andrea



================
Comment at: tools/llvm-mca/Dispatch.cpp:329
   assert(!CarryOver && "Cannot dispatch another instruction!");
-  unsigned NumMicroOps = NewInst->getDesc().NumMicroOps;
+  Instruction *IS = IR.getInstruction();
+  const InstrDesc &Desc = IS->getDesc();
----------------
Use an instruction reference here.
`Instruction &IS = *IR.getInstruction();`


================
Comment at: tools/llvm-mca/InstrBuilder.cpp:429-431
+InstrBuilder::createInstruction(const MCInst &MCI) {
   const InstrDesc &D = getOrCreateInstrDesc(MCI);
+  std::unique_ptr<Instruction> NewIS = llvm::make_unique<Instruction>(D);
----------------
Same. You should rebase 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);
 };
----------------
Please correctly rebase this patch. This change should not have been here.


================
Comment at: tools/llvm-mca/Instruction.h:20
 #include "llvm/Support/MathExtras.h"
+#include "llvm/Support/raw_ostream.h"
 #include <memory>
----------------
Please remove this. See my comment below.
Essentially, raw_ostream is only used for debugging purposes here. Move the printing functionality to the cpp file, and guard the declaration of the printing functionality using a check on NDEBUG. This include can then be removed.


================
Comment at: tools/llvm-mca/Instruction.h:41
+public:
+  InstRef() : std::pair<unsigned, Instruction *>(0, nullptr) {}
+  InstRef(unsigned Index, Instruction *I)
----------------
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.


================
Comment at: tools/llvm-mca/Instruction.h:48
+  Instruction *getInstruction() {
+    assert(isValid() && "Invalid InstRef instance, not initialized.");
+    return second;
----------------
Not needed. It is okay to return nullptr.


================
Comment at: tools/llvm-mca/Instruction.h:53
+  const Instruction *getInstruction() const {
+    assert(isValid() && "Invalid InstRef instance, not initialized.");
+    return second;
----------------
Not needed.


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


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


================
Comment at: tools/llvm-mca/RetireControlUnit.cpp:40
   unsigned TokenID = NextAvailableSlotIdx;
-  Queue[NextAvailableSlotIdx] = {&IS, NormalizedQuantity, false};
+  Queue[NextAvailableSlotIdx] = {*IR, NormalizedQuantity, false};
   NextAvailableSlotIdx += NormalizedQuantity;
----------------
This is wrong.


================
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)
----------------
Can this ever happen?


================
Comment at: tools/llvm-mca/RetireControlUnit.h:50
   struct RUToken {
-    Instruction *IS;
+    InstRef IR;
     unsigned NumSlots; // Slots reserved to this instruction.
----------------
It was an unsigned integer before. Now it is an InstRef. Please explain why?


================
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);
 
----------------
This is incorrect. Regardless of the wrong diff.


================
Comment at: tools/llvm-mca/Scheduler.cpp:412-413
                                [&](const QueueEntryTy &Entry) {
-                                 const Instruction &IS = *Entry.second;
-                                 const InstrDesc &D = IS.getDesc();
+                                 const Instruction *IS = Entry.second;
+                                 const InstrDesc &D = IS->getDesc();
                                  return Resources->canBeIssued(D);
----------------
``` const InstrDesc &D = Entry.second->getDesc(); ```


https://reviews.llvm.org/D46367