[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