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

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 3 inline comments as done.
rnk 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();
----------------
dschuff wrote:
> 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.
I agree. Most targets don't need register scavenging, and wasm (the only target that uses vregs past this point) definitely doesn't need it either.


https://reviews.llvm.org/D38597