[PATCH] D41226: LiveDebugValues spill recognition expasnsion

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:451
+            if (RegNext == Reg) {
+              Reg = RegNext;
+              break;
----------------
NikolaPrica wrote:
> aprantl wrote:
> > aprantl wrote:
> > > Isn't that a no-op?
> > The test still passes when I comment out line 449-450. Could you add coverage for these two lines, too?
> I'm not sure what are you asking. Do you think I should add test coverage for this particular condition on 449. If you do, only what I could think of is changing MIR test example in order to hit condition on line 449. It would go something like this then:
>    
>     %r12d = MOV32rr %r15d
>     MOV32mr %rbp, 1, %noreg, -48, %noreg, renamable %r15d :: (store 4 into %stack.0)
>     %edi = MOV32rr killed %r12d
> 
> Then this wouldn't be recognized as spill instruction but it could be mentioned in test that this is changed in testing purpose.
> 
>  Or you are thinking of dispatching condition on 449? I've tried to describe particular situation from MIR with conditions from lines 449 and 448. But this situation could be described as spill instruction after which we expect that copy operation is done and that spill register is killed in the next instruction. Said like that then we don't need condition from line 449.
I'm asking for 100% test coverage for the patch. If a patch is adding new functionality, that functionality should be tested, so (1) we know that it works as intended and (2) future generations of LLVM developers won't be tempted to remove it "because it has no effect (on the testsuite)".
It's fine to add a second testcase to the patch if that makes it easier. Thanks!


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:445
+        if (MI.getParent()->instr_end() == NextInstr)
+          continue;
+        unsigned RegNext = 0;
----------------
Perhaps add a comment here why we are skipping here?


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:449
+          if (MONext.isReg() && getRegIfKilled(MONext, RegNext)) {
+            if (RegNext == Reg)
+              break;
----------------
Perhaps add a comment here why we are breaking out of the loop here?


https://reviews.llvm.org/D41226