[PATCH] D38597: [PEI] Remove required properties and use 'if' instead of std::function

[Derek Schuff via Phabricator via llvm-commits]

dschuff added inline comments.


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:201-205
+    if (UsesCalleeSaves)
+      scavengeFrameVirtualRegs(Fn, *RS);
 
-      // Clear any vregs created by virtual scavenging.
-      Fn.getRegInfo().clearVirtRegs();
+    // Clear any vregs created by virtual scavenging.
+    Fn.getRegInfo().clearVirtRegs();
----------------
MatzeB wrote:
> This feels wrong, we should only `clearVirtRegs()` when we actually scavenged them. Checking the code it seems that the clearing is already done in `scavengeFrameVirtualRegs()` nowadays so we can remove it here. Maybe we should also skip the whole `if (UsesCalleeSaves)` then too as it seems target can already control this bit via TRI::requiresRegisterScavenging`.
+1 in general. I'm ambivalent on removing `if (UsesCalleeSaves)` altogether. It's true that no target I can imagine would have `UsesCalleeSaves` be false and `requiresRegisterScavenging` be true, but having it explicit makes it a bit more obvious. I'd be OK either way I think.


================
Comment at: llvm/lib/CodeGen/PrologEpilogInserter.cpp:492
                                    const MBBVector &RestoreBlocks) {
+  assert(Fn.getProperties().hasProperty(
+      MachineFunctionProperties::Property::NoVRegs));
----------------
I think this is the right assertion, but there should be a comment here about why we are doing it manually instead of using `getRequiredProperties`, since that would be the expected thing.


https://reviews.llvm.org/D38597