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

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

I think that it should be possible to write a test case for this. You don't really have to expose a *bug* here but just show that the exposed uses are added to the outlined function. That's sufficient for a testcase.

I think that you can probably do this by copying + editing one of the existing outliner testcases. A good test to base it off of might be `llvm/test/CodeGen/AArch64/machine-outliner-calls.mir`, since it's pretty simple.



================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1247
         // code.
-        auto CopyDefsAndUpdateCalls = [&CallInst](MachineInstr &MI) {
-          for (MachineOperand &MOP : MI.operands()) {
+        SmallSet<unsigned, 2> UseRegs;
+        MachineBasicBlock::reverse_iterator Last =
----------------
Can this be `Register` instead of `unsigned`?


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1257
+        // of code. The exposed uses need to be copied in the outlined range.
+        for (; Iter != Last; Iter++) {
+          MachineInstr *MI = &*Iter;
----------------
Can you pull the variables into the loop header to fit this sort of style?

```
for (MachineBasicBlock::reverse_iterator Iter = (stuff), Last = (stuff); Iter != Last; ++Iter)
```

Then it's clear that the variables are only used in the loop.


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1269-1270
                   true /* isImp = true */));
+              if (UseRegs.count(MOP.getReg()))
+                UseRegs.erase(MOP.getReg());
+            } else if (!MOP.isUndef())
----------------
Comment?


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1271-1272
+                UseRegs.erase(MOP.getReg());
+            } else if (!MOP.isUndef())
+              UseRegs.insert(MOP.getReg());
           }
----------------
Add a brace?


================
Comment at: llvm/lib/CodeGen/MachineOutliner.cpp:1271-1272
+                UseRegs.erase(MOP.getReg());
+            } else if (!MOP.isUndef())
+              UseRegs.insert(MOP.getReg());
           }
----------------
paquette wrote:
> Add a brace?
Comment?


================
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.
----------------
tellenbach wrote:
> 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.
+1 for making type explicit


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71217