llvm-reduce: Preserve frame index values when cloning function

[arsenm]

Summary

Previously the specific values used for fixed frame indexes was in
reverse order in the cloned function from the original, and a map was
used to adjust all frame indexes to the potentially new values. Insert
the fixed objects in reverse to avoid this. This simplifies other
code, since now we don't need to track down all frame indexes. This
will allow targets that store frame indexes in MachineFunctionInfo to
simply copy the values.

Note this isn't directly observable in the test since the resulting
MIR print/parse can shuffle the IDs around (in particular the final
serialization implicitly strips out dead objects).

[arsenm]

ping

[markus]

In general I am not too familiar with the frame index stuff so I cannot really comment much on that besides that it looks reasonable. From the point of view of the guy who did the initial MIR support work for llvm-reduce it looks like a good improvement and a step in the right direction.

[arsenm]

a27b9ab391d1e0749e37b7db93b0fae18e23ecf6

llvm/tools/llvm-reduce/ReducerWorkItem.cpp

 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/IRReader/IRReader.h"
 #include "llvm/Support/SourceMgr.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 
-// FIXME: Preserve frame index numbers. The numbering is off for fixed objects
-// since they are inserted at the beginning. This would avoid the need for the
-// Src2DstFrameIndex map and in the future target MFI code wouldn't need to
-// worry about it either.
 static void cloneFrameInfo(
     MachineFrameInfo &DstMFI, const MachineFrameInfo &SrcMFI,
-    const DenseMap<MachineBasicBlock *, MachineBasicBlock *> Src2DstMBB,
-    DenseMap<int, int> &Src2DstFrameIndex) {
+    const DenseMap<MachineBasicBlock *, MachineBasicBlock *> &Src2DstMBB) {
   DstMFI.setFrameAddressIsTaken(SrcMFI.isFrameAddressTaken());
   DstMFI.setReturnAddressIsTaken(SrcMFI.isReturnAddressTaken());
   DstMFI.setHasStackMap(SrcMFI.hasStackMap());
   DstMFI.setHasPatchPoint(SrcMFI.hasPatchPoint());
   DstMFI.setUseLocalStackAllocationBlock(
       SrcMFI.getUseLocalStackAllocationBlock());
   DstMFI.setOffsetAdjustment(SrcMFI.getOffsetAdjustment());
 
static void cloneFrameInfo(
   DstMFI.setCVBytesOfCalleeSavedRegisters(
       SrcMFI.getCVBytesOfCalleeSavedRegisters());
 
   if (MachineBasicBlock *SavePt = SrcMFI.getSavePoint())
     DstMFI.setSavePoint(Src2DstMBB.find(SavePt)->second);
   if (MachineBasicBlock *RestorePt = SrcMFI.getRestorePoint())
     DstMFI.setRestorePoint(Src2DstMBB.find(RestorePt)->second);
 
-  for (int i = SrcMFI.getObjectIndexBegin(), e = SrcMFI.getObjectIndexEnd();
+
+  auto CopyObjectProperties = [](MachineFrameInfo &DstMFI,
+                                 const MachineFrameInfo &SrcMFI, int FI) {
+    if (SrcMFI.isStatepointSpillSlotObjectIndex(FI))
+      DstMFI.markAsStatepointSpillSlotObjectIndex(FI);
+    DstMFI.setObjectSSPLayout(FI, SrcMFI.getObjectSSPLayout(FI));
+    DstMFI.setObjectZExt(FI, SrcMFI.isObjectZExt(FI));
+    DstMFI.setObjectSExt(FI, SrcMFI.isObjectSExt(FI));
+  };
+
+  for (int i = 0, e = SrcMFI.getNumObjects() - SrcMFI.getNumFixedObjects();
        i != e; ++i) {
     int NewFI;
 
-    if (SrcMFI.isFixedObjectIndex(i)) {
-      NewFI = DstMFI.CreateFixedObject(
-          SrcMFI.getObjectSize(i), SrcMFI.getObjectOffset(i),
-          SrcMFI.isImmutableObjectIndex(i), SrcMFI.isAliasedObjectIndex(i));
-    } else if (SrcMFI.isVariableSizedObjectIndex(i)) {
+    assert(!SrcMFI.isFixedObjectIndex(i));
+    if (SrcMFI.isVariableSizedObjectIndex(i)) {
       NewFI = DstMFI.CreateVariableSizedObject(SrcMFI.getObjectAlign(i),
                                                SrcMFI.getObjectAllocation(i));
     } else {
       NewFI = DstMFI.CreateStackObject(
           SrcMFI.getObjectSize(i), SrcMFI.getObjectAlign(i),
           SrcMFI.isSpillSlotObjectIndex(i), SrcMFI.getObjectAllocation(i),
           SrcMFI.getStackID(i));
       DstMFI.setObjectOffset(NewFI, SrcMFI.getObjectOffset(i));
     }
 
-    if (SrcMFI.isStatepointSpillSlotObjectIndex(i))
-      DstMFI.markAsStatepointSpillSlotObjectIndex(NewFI);
-    DstMFI.setObjectSSPLayout(NewFI, SrcMFI.getObjectSSPLayout(i));
-    DstMFI.setObjectZExt(NewFI, SrcMFI.isObjectZExt(i));
-    DstMFI.setObjectSExt(NewFI, SrcMFI.isObjectSExt(i));
+    CopyObjectProperties(DstMFI, SrcMFI, i);
+
+    (void)NewFI;
+    assert(i == NewFI && "expected to keep stable frame index numbering");
+  }
+
+  // Copy the fixed frame objects backwards to preserve frame index numbers,
+  // since CreateFixedObject uses front insertion.
+  for (int i = -1; i >= (int)-SrcMFI.getNumFixedObjects(); --i) {
+    assert(SrcMFI.isFixedObjectIndex(i));
+    int NewFI = DstMFI.CreateFixedObject(
+      SrcMFI.getObjectSize(i), SrcMFI.getObjectOffset(i),
+      SrcMFI.isImmutableObjectIndex(i), SrcMFI.isAliasedObjectIndex(i));
+    CopyObjectProperties(DstMFI, SrcMFI, i);
 
-    Src2DstFrameIndex[i] = NewFI;
+    (void)NewFI;
+    assert(i == NewFI && "expected to keep stable frame index numbering");
   }
 
   for (unsigned I = 0, E = SrcMFI.getLocalFrameObjectCount(); I < E; ++I) {
     auto LocalObject = SrcMFI.getLocalFrameObjectMap(I);
     DstMFI.mapLocalFrameObject(LocalObject.first, LocalObject.second);
   }
 
-  // Remap the frame indexes in the CalleeSavedInfo
-  std::vector<CalleeSavedInfo> CalleeSavedInfos = SrcMFI.getCalleeSavedInfo();
-  for (CalleeSavedInfo &CSInfo : CalleeSavedInfos) {
-    if (!CSInfo.isSpilledToReg())
-      CSInfo.setFrameIdx(Src2DstFrameIndex[CSInfo.getFrameIdx()]);
-  }
-
-  DstMFI.setCalleeSavedInfo(std::move(CalleeSavedInfos));
+  DstMFI.setCalleeSavedInfo(SrcMFI.getCalleeSavedInfo());
 
   if (SrcMFI.hasStackProtectorIndex()) {
-    DstMFI.setStackProtectorIndex(
-        Src2DstFrameIndex[SrcMFI.getStackProtectorIndex()]);
+    DstMFI.setStackProtectorIndex(SrcMFI.getStackProtectorIndex());
   }
 
   // FIXME: Needs test, missing MIR serialization.
   if (SrcMFI.hasFunctionContextIndex()) {
-    DstMFI.setFunctionContextIndex(
-        Src2DstFrameIndex[SrcMFI.getFunctionContextIndex()]);
+    DstMFI.setFunctionContextIndex(SrcMFI.getFunctionContextIndex());
   }
 }
 
 static std::unique_ptr<MachineFunction> cloneMF(MachineFunction *SrcMF) {
   auto DstMF = std::make_unique<MachineFunction>(
       SrcMF->getFunction(), SrcMF->getTarget(), SrcMF->getSubtarget(),
       SrcMF->getFunctionNumber(), SrcMF->getMMI());
   DenseMap<MachineBasicBlock *, MachineBasicBlock *> Src2DstMBB;
   DenseMap<Register, Register> Src2DstReg;
-  DenseMap<int, int> Src2DstFrameIndex;
 
   auto *SrcMRI = &SrcMF->getRegInfo();
   auto *DstMRI = &DstMF->getRegInfo();
 
   // Clone blocks.
   for (MachineBasicBlock &SrcMBB : *SrcMF)
     Src2DstMBB[&SrcMBB] = DstMF->CreateMachineBasicBlock();
 
   const MachineFrameInfo &SrcMFI = SrcMF->getFrameInfo();
   MachineFrameInfo &DstMFI = DstMF->getFrameInfo();
 
   // Copy stack objects and other info
-  cloneFrameInfo(DstMFI, SrcMFI, Src2DstMBB, Src2DstFrameIndex);
+  cloneFrameInfo(DstMFI, SrcMFI, Src2DstMBB);
 
   // Remap the debug info frame index references.
   DstMF->VariableDbgInfos = SrcMF->VariableDbgInfos;
-  for (MachineFunction::VariableDbgInfo &DbgInfo : DstMF->VariableDbgInfos)
-    DbgInfo.Slot = Src2DstFrameIndex[DbgInfo.Slot];
 
   // FIXME: Need to clone MachineFunctionInfo, which may also depend on frame
   // index and block mapping.
 
   // Create vregs.
   for (auto &SrcMBB : *SrcMF) {
     for (auto &SrcMI : SrcMBB) {
       for (unsigned I = 0, E = SrcMI.getNumOperands(); I < E; ++I) {
for (auto &SrcMI : SrcMBB) {
         DstMO.clearParent();
         // Update vreg.
         if (DstMO.isReg() && Src2DstReg.count(DstMO.getReg())) {
           DstMO.setReg(Src2DstReg[DstMO.getReg()]);
         }
         // Update MBB.
         if (DstMO.isMBB()) {
           DstMO.setMBB(Src2DstMBB[DstMO.getMBB()]);
-        } else if (DstMO.isFI()) {
-          // Update frame indexes
-          DstMO.setIndex(Src2DstFrameIndex[DstMO.getIndex()]);
         }
 
         DstMI->addOperand(DstMO);
       }
       DstMI->setMemRefs(*DstMF, SrcMI.memoperands());
     }
   }