[TargetLowering][NFC] More efficient emitPatchpoint().

[dantrushin]

Summary

Current implementation of emitPatchpoint() is very inefficient:
for every FrameIndex operand if creates new MachineInstr with
that operand expanded and all other copies as is.
Since PATCHPOINT/STATEPOINT instructions may have *a lot* of
FrameIndex operands, we end up creating and erasing many
machine instructions. But we can do it in single pass, with only
one new machine instruction generated.

[efriedma]

llvm/lib/CodeGen/TargetLoweringBase.cpp

1030

You're making this code a lot harder to follow with the whole OperIdx thing, and it only saves one branch per MachineOperand. Please just do if (!llvm::any_of(MI->operands(), [](MachineOperand &Operand) { return Operand.isFI(); }) return MBB;

[reames]

Agreed with Eli, simply having two passes over operands would be much cleaner.

I do like the removal of the multi pass copy. Definitely moving in the right direction.

[dantrushin]

Address comments

[reames]

LGTM w/minor change suggested.

llvm/lib/CodeGen/TargetLoweringBase.cpp

1035

Minor, optional: I believe you can rewrite this as:
for (auto &Op : MI->operands()) { ...

[stuij]

Seems like this is breaking on buildbot (and also my local checkout): http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/16759

[dantrushin]

In D81181#2076092, @stuij wrote:

Seems like this is breaking on buildbot (and also my local checkout): http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/16759

Are you sure? In your logs I see

E:\build_slave\lldb-x64-windows-ninja\llvm-project\llvm\include\llvm/CodeGen/TargetLowering.h(2971): fatal error C1088: Cannot flush compiler intermediate file: 'C:\Users\BUILDS~1\AppData\Local\Temp\_CL_ea8c083eex': No space left on device

which cannot be caused by my fix, I believe ;)
I'll double check buildbot

[dantrushin]

Ah, I see. I indeed broke the build. Will fix in a moment

[stuij]

ah sorry, I guess that might have been the wrong link. My copy pasting skills are flaky.

what I'm seeing is this:

FAILED: lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/TargetLoweringBase.cpp.o 
/usr/local/bin/c++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/CodeGen -I/home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/lib/CodeGen -I/usr/include/libxml2 -Iinclude -I/home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -O3     -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/TargetLoweringBase.cpp.o -MF lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/TargetLoweringBase.cpp.o.d -o lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/TargetLoweringBase.cpp.o -c /home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/lib/CodeGen/TargetLoweringBase.cpp
/home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/lib/CodeGen/TargetLoweringBase.cpp: In member function ‘llvm::MachineBasicBlock* llvm::TargetLoweringBase::emitPatchPoint(llvm::MachineInstr&, llvm::MachineBasicBlock*) const’:
/home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/lib/CodeGen/TargetLoweringBase.cpp:1037:30: error: ‘OperIdx’ was not declared in this scope
 1037 |       MIB.add(MI->getOperand(OperIdx));
      |                              ^~~~~~~
/home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/lib/CodeGen/TargetLoweringBase.cpp:1054:30: error: ‘OperIdx’ was not declared in this scope
 1054 |       MIB.add(MI->getOperand(OperIdx));
      |                              ^~~~~~~
/home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/lib/CodeGen/TargetLoweringBase.cpp:1060:30: error: ‘OperIdx’ was not declared in this scope
 1060 |       MIB.add(MI->getOperand(OperIdx));

from: http://lab.llvm.org:8011/builders/clang-cmake-aarch64-quick/builds/23856/steps/build%20stage%201/logs/stdio

[dantrushin]

In D81181#2076148, @stuij wrote:

ah sorry, I guess that might have been the wrong link. My copy pasting skills are flaky.

what I'm seeing is this:

FAILED: lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/TargetLoweringBase.cpp.o 
/usr/local/bin/c++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/CodeGen -I/home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/lib/CodeGen -I/usr/include/libxml2 -Iinclude -I/home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment -fdiagnostics-color -ffunction-sections -fdata-sections -O3     -fno-exceptions -fno-rtti -UNDEBUG -std=c++14 -MD -MT lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/TargetLoweringBase.cpp.o -MF lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/TargetLoweringBase.cpp.o.d -o lib/CodeGen/CMakeFiles/LLVMCodeGen.dir/TargetLoweringBase.cpp.o -c /home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/lib/CodeGen/TargetLoweringBase.cpp
/home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/lib/CodeGen/TargetLoweringBase.cpp: In member function ‘llvm::MachineBasicBlock* llvm::TargetLoweringBase::emitPatchPoint(llvm::MachineInstr&, llvm::MachineBasicBlock*) const’:
/home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/lib/CodeGen/TargetLoweringBase.cpp:1037:30: error: ‘OperIdx’ was not declared in this scope
 1037 |       MIB.add(MI->getOperand(OperIdx));
      |                              ^~~~~~~
/home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/lib/CodeGen/TargetLoweringBase.cpp:1054:30: error: ‘OperIdx’ was not declared in this scope
 1054 |       MIB.add(MI->getOperand(OperIdx));
      |                              ^~~~~~~
/home/tcwg-buildslave/worker/clang-cmake-aarch64-quick/llvm/llvm/lib/CodeGen/TargetLoweringBase.cpp:1060:30: error: ‘OperIdx’ was not declared in this scope
 1060 |       MIB.add(MI->getOperand(OperIdx));

from: http://lab.llvm.org:8011/builders/clang-cmake-aarch64-quick/builds/23856/steps/build%20stage%201/logs/stdio

Should be fixed now. Can you try again?
Sorry for the inconvenience.

[stuij]

Yup, LLVM now builds and check-all's. Thanks for fixing!

llvm/lib/CodeGen/TargetLoweringBase.cpp

TargetLoweringBase::emitPatchPoint(MachineInstr &InitialMI,
   // (We're currently conservative and mark the deopt slots read/write in
   // practice.)
   // STATEPOINT GC Spill - live-through, read/write, indirect
   // STATEPOINT GC Alloca - live-through, read/write, direct
   // The live-in vs live-through is handled already (the live through ones are
   // all stack slots), but we need to handle the different type of stackmap
   // operands and memory effects here.
 
-  // MI changes inside this loop as we grow operands.
-  for(unsigned OperIdx = 0; OperIdx != MI->getNumOperands(); ++OperIdx) {
+  unsigned OperIdx = 0;
+  unsigned NumOpers = MI->getNumOperands();
+  for (; OperIdx < NumOpers; ++OperIdx)
+    if (MI->getOperand(OperIdx).isFI())
+      break;
+
+  if (OperIdx == NumOpers)
+    return MBB; // No frame indices seen, nothing to do.
+
+  MachineInstrBuilder MIB = BuildMI(MF, MI->getDebugLoc(), MI->getDesc());
+
+  for (unsigned i = 0; i < OperIdx; ++i)
+    MIB.add(MI->getOperand(i));
+
+  // Inherit previous memory operands.
+  MIB.cloneMemRefs(*MI);
+
+  for (; OperIdx < NumOpers; ++OperIdx) {
     MachineOperand &MO = MI->getOperand(OperIdx);
-    if (!MO.isFI())
+    if (!MO.isFI()) {
+      MIB.add(MI->getOperand(OperIdx));
       continue;
+    }
 
     // foldMemoryOperand builds a new MI after replacing a single FI operand
     // with the canonical set of five x86 addressing-mode operands.
     int FI = MO.getIndex();
-    MachineInstrBuilder MIB = BuildMI(MF, MI->getDebugLoc(), MI->getDesc());
 
-    // Copy operands before the frame-index.
-    for (unsigned i = 0; i < OperIdx; ++i)
-      MIB.add(MI->getOperand(i));
     // Add frame index operands recognized by stackmaps.cpp
     if (MFI.isStatepointSpillSlotObjectIndex(FI)) {
       // indirect-mem-ref tag, size, #FI, offset.
       // Used for spills inserted by StatepointLowering.  This codepath is not
       // used for patchpoints/stackmaps at all, for these spilling is done via
       // foldMemoryOperand callback only.
       assert(MI->getOpcode() == TargetOpcode::STATEPOINT && "sanity");
       MIB.addImm(StackMaps::IndirectMemRefOp);
       MIB.addImm(MFI.getObjectSize(FI));
       MIB.add(MI->getOperand(OperIdx));
       MIB.addImm(0);
     } else {
       // direct-mem-ref tag, #FI, offset.
       // Used by patchpoint, and direct alloca arguments to statepoints
       MIB.addImm(StackMaps::DirectMemRefOp);
       MIB.add(MI->getOperand(OperIdx));
       MIB.addImm(0);
     }
-    // Copy the operands after the frame index.
-    for (unsigned i = OperIdx + 1; i != MI->getNumOperands(); ++i)
-      MIB.add(MI->getOperand(i));
 
-    // Inherit previous memory operands.
-    MIB.cloneMemRefs(*MI);
     assert(MIB->mayLoad() && "Folded a stackmap use to a non-load!");
 
     // Add a new memory operand for this FI.
     assert(MFI.getObjectOffset(FI) != -1);
 
     // Note: STATEPOINT MMOs are added during SelectionDAG.  STACKMAP, and
     // PATCHPOINT should be updated to do the same. (TODO)
     if (MI->getOpcode() != TargetOpcode::STATEPOINT) {
       auto Flags = MachineMemOperand::MOLoad;
       MachineMemOperand *MMO = MF.getMachineMemOperand(
           MachinePointerInfo::getFixedStack(MF, FI), Flags,
           MF.getDataLayout().getPointerSize(), MFI.getObjectAlign(FI));
       MIB->addMemOperand(MF, MMO);
     }
-
-    // Replace the instruction and update the operand index.
-    MBB->insert(MachineBasicBlock::iterator(MI), MIB);
-    OperIdx += (MIB->getNumOperands() - MI->getNumOperands()) - 1;
-    MI->eraseFromParent();
-    MI = MIB;
-  }
+  }
   return MBB;
 }
 
 MachineBasicBlock *
 TargetLoweringBase::emitXRayCustomEvent(MachineInstr &MI,
                                         MachineBasicBlock *MBB) const {
   assert(MI.getOpcode() == TargetOpcode::PATCHABLE_EVENT_CALL &&
          "Called emitXRayCustomEvent on the wrong MI!");