[PATCH] D123794: llvm-reduce: Handle cloning MachineFrameInfo and stack objects

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

I fear that having the cloning code in `tools/llvm-reduce` makes it easy for people to miss when they add new fields to `MachineFrameInfo`. Have you considered adding a `MachineFrameInfo::clone(...)` method instead so the code lives closer to the rest of `MachineFrameInfo`? (same comment probably applies to more of the machine function cloning code, so we can move the code around in an independent commit or another time...)

Either way LGTM



================
Comment at: llvm/tools/llvm-reduce/ReducerWorkItem.cpp:81-82
+
+    // FIXME: These fields need tests. They seem to be missing from
+    // serialization.
+    if (SrcMFI.isStatepointSpillSlotObjectIndex(i))
----------------
Maybe this should just be announced in the commit message. Having a `FIXME` comment here feels slightly off to me since we're not gonna add a test into this particular file or at this line.


================
Comment at: llvm/tools/llvm-reduce/ReducerWorkItem.cpp:98-99
+  // Remap the frame indexes in the CalleeSavedInfo
+  std::vector<CalleeSavedInfo> CalleeSavedInfo = SrcMFI.getCalleeSavedInfo();
+  for (auto &CSInfo : CalleeSavedInfo) {
+    if (!CSInfo.isSpilledToReg())
----------------



================
Comment at: llvm/tools/llvm-reduce/ReducerWorkItem.cpp:111
+
+  // FIXME: Needs test, missing MIR serialization.
+  if (SrcMFI.hasFunctionContextIndex()) {
----------------
as above


================
Comment at: llvm/tools/llvm-reduce/ReducerWorkItem.cpp:130
+  // Clone blocks.
+  for (auto &SrcMBB : *SrcMF)
+    Src2DstMBB[&SrcMBB] = DstMF->CreateMachineBasicBlock();
----------------



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

https://reviews.llvm.org/D123794