[PATCH] D60715: [ISEL] Collect argument's forwarding regs when lowering calls

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: include/llvm/CodeGen/MachineFunction.h:381
+  /// Vector of call argument and its forwarding register.
+  using CallSiteInfo = SmallVector<std::pair<uint32_t, unsigned>, 1>;
+  using CallSiteInfoImpl = SmallVectorImpl<std::pair<uint32_t, unsigned>>;
----------------
How about using a struct with field names instead of a std::pair?


================
Comment at: include/llvm/CodeGen/MachineFunction.h:955
+  /// Update call sites info by deleting entry for /p Old call instruction.
+  /// If /p New is present then transfer /p Old call info to it.
+  void updateCallSiteInfo(const MachineInstr *Old,
----------------
`/p` -> `\p`


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1634
+      return std::move(I->second);
+    return CallSiteInfo();
+  }
----------------
This function appears to reimplement the operator[] of DenseMap?
I guess it doesn't insert the value...



================
Comment at: lib/CodeGen/MIRPrinter.cpp:476
+    // Get call instruction offset from the beginning of block.
+    for (auto E = CallI->getParent()->rend(); CallI != E; CallI++, Offset++);
+    CallLocation.Offset = Offset;
----------------
Can we replace this bodyless loop with something from std::algorithm?


================
Comment at: lib/CodeGen/MachineFunction.cpp:365
+  if (MI->isCall(MachineInstr::IgnoreBundle))
+    assert(CallSitesInfo.find(MI) == CallSitesInfo.end() &&
+           "Call site info is not updated!");
----------------
`assert (!MI->isCall(MachineInstr::IgnoreBundle) || ...)`


================
Comment at: lib/CodeGen/MachineVerifier.cpp:2150
+    if (!CSInfo.first->isCall())
+      report("Call site info refrenceining instruction that is not call", MF);
 }
----------------
typo


================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:140
+  // info.
+  while(Tail != MBB->end()) {
+    MachineInstr *MI = &*Tail;
----------------
clang-format


================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:145
+    ++Tail;
+    MBB->erase(MI);
+  }
----------------
I'd prefer writing two loops (or even better two std::algorithms) for updating and erasing and let the optimizer merge them.


================
Comment at: lib/CodeGen/XRayInstrumentation.cpp:114
+        if (T.isCall())
+          MF.updateCallSiteInfo(&T);
       }
----------------
So how do we make sure that people will call this in their new passes form here on? Can we verify it?


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

https://reviews.llvm.org/D60715