[PATCH] D71217: Fix incorrect logic in maintaining the side-effect of compiler generated outliner functions

[David Tellenbach via Phabricator via llvm-commits]

tellenbach added a comment.

As @lebedev.ri already said, is it possible to add a test for this? I know it is probably covered in your test for D71027 <https://reviews.llvm.org/D71027> but an independent test would be great!



================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1244
               MachineFunctionProperties::Property::TracksLiveness)) {
         // Helper lambda for adding implicit def operands to the call
         // instruction. It also updates call site information for moved
----------------
This comment is not correct anymore: The helper lambda is gone.


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1250
+            std::next(CallInst.getReverse());
+        MachineBasicBlock::reverse_iterator Iter = EndIt.getReverse();
+        // Copy over the defs in the outlined range.
----------------
I guess this can be moved into the `for`-loop since it's not needed outside of the loop scope.


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1258
+        for (; Iter != Last; Iter++) {
+          MachineInstr *MI = &*Iter;
+          for (MachineOperand &MOP : MI->operands()) {
----------------
Can't this just be `MachineInstr *MI = Iter;`? You could actually just use `Iter` directly and omit the new variable.


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1276
+            MI->getMF()->eraseCallSiteInfo(MI);
         };
+        for (auto I : UseRegs)
----------------
This `;` can be removed.


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1277
         };
-        // Copy over the defs in the outlined range.
-        // First inst in outlined range <-- Anything that's defined in this
-        // ...                           .. range has to be added as an
-        // implicit Last inst in outlined range  <-- def to the call
-        // instruction. Also remove call site information for outlined block
-        // of code.
-        std::for_each(CallInst, std::next(EndIt), CopyDefsAndUpdateCalls);
+        for (auto I : UseRegs)
+          // If it's a exposed use, add it to the call instruction.
----------------
Can this be `const &`? You could even think about making this type explicit since it's not very verbose but no strong opinion on this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71217