[PATCH] D15157: CodeGen peephole: fold redundant phys reg copies

Quentin Colombet via llvm-commits llvm-commits at lists.llvm.org
Wed Dec 2 16:12:43 PST 2015


qcolombet added a comment.

Hi,

Looks almost good.
Two main remarks:

1. Check for regmask in the operands as well
2. Try to add more test cases for coverage

Thanks,
-Quentin


================
Comment at: lib/CodeGen/PeepholeOptimizer.cpp:1427
@@ +1426,3 @@
+    MachineInstr *MI, DenseMap<unsigned, MachineInstr *> &NAPhysToVirtMIs) {
+  assert(MI->isCopy());
+
----------------
Put a message in the assert, even an obvious one :).

================
Comment at: lib/CodeGen/PeepholeOptimizer.cpp:1537
@@ +1536,3 @@
+      if (!MI->isCopy()) {
+        for (const auto &Op : MI->operands()) {
+          // Visit all operands: definitions can be implicit or explicit.
----------------
I’ll make a function out of it and you have to check the reg mask operand as well.
For instance, if we have a pure (side effect free) function call, you may clobber the register and we will miss it.

================
Comment at: test/CodeGen/X86/incdec-and-branch.ll:16
@@ +15,3 @@
+; CHECK-NOT: pushf
+; CHECK-NOT: popf
+
----------------
Could we have some “positive” testing as well, i.e. CHECK lines not just CHECK-NOT?

Also, I would rename the file into peephole-na-copy-folding or something and add more test cases where you check the inline asm case, the definition in the middle, etc.
Anyhow, you get it, something that improves the coverage.
I understand this can be difficult to come with additional test cases, just give it a try and if you can’t find some, then you can’t :).


http://reviews.llvm.org/D15157





More information about the llvm-commits mailing list