This patch prevents DBG_VALUE instructions from influencing
LivePhysRegs::stepBackwards and stepForwards. In at least one case,
specifically branch folding, the stepBackwards logic was having an
influence on code generation. The result was that certain code
compiled with '-g -O2' would differ from that compiled with '-O2'
alone. It seems that the original logic, accounting for DBG_VALUE,
was influencing the placement of an IMPLICIT_DEF which had a later
impact on how blocks were processed in branch folding.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
This is surprising: DBG_VALUE should not have any defs so removeDefs() should have no effect and all uses should be marked with the debug flag so none of them should pass the MachineOperand::readsReg() test in addUses. Could you check whether the debug flag is set correctly? Did you try running your test with -verify-machineinstrs?
Comment Actions
Thanks for the reply and helpful response @MatzeB . I do not get any diagnostic issues when running my test with -verify-machineinstrs.
I think what might be happening is that, during tail merging, some undef operands were switched to being non-undef. That wording I ripped from the comment in lib/CodeGen/BranchFolding.cpp: replaceTailWithBranchTo. This the spot in the code where I discovered the issue.
What I noticed is that when I compile one of my tests with '-g -O2', replaceTailWIthBranchTo collects LiveRegs from the 'old' basic block that is losing its tail. That block will eventually jmp to the NewDest basic block. Some of the registers collected in LiveRegs also happen to be live in the NewDest.liveins. Because of this correlation (reg is live-in old and live-in new) an IMPLICIT_DEF is generated in the old basic block. It just happens to be that a DBG_VALUE is associated to that register. Sorry, that might have been hard to follow. Here is some generated MI that I noticed differed between the '-O2' and '-g -O2'. This dump being from '-g -O2'
bb.11: ; predecessors: %bb.10, %bb.12, %bb.14, %bb.16, %bb.18 successors: %bb.21(0x80000000); %bb.21(200.00%) liveins: $rdi $al = IMPLICIT_DEF bb.21 (%ir-block.4): ; predecessors: %bb.20, %bb.11 successors: %bb.22(0x80000000); %bb.22(200.00%) liveins: $rdi, $al DBG_VALUE debug-use $al, debug-use $noreg, !"d", !DIExpression(), debug-location !36; line no:6 renamable $ecx = XOR32rr undef $ecx, undef $ecx, implicit-def dead $eflags
In one of my tests, I noticed that a particular basic block (bb.11) was being removed in -O2 but it was not being removed in '-O2 -g', which had an impact on the way the basic blocks were laid out, and resulted in a code difference. It appeared that this started from an IMPLICIT_DEF being created via replaceTailWithBranchTo, which is what is listed in bb.11 above.
Comment Actions
Just pinging this thread. Does this solution look reasonable, or does it look like we might be masking some other problem?
Comment Actions
The fix looks reasonable to me, but could you create a .mir testcase instead of .ll?
Also, stepForward may require a similar change.
Comment Actions
@mattd: Please upload full diffs when requesting a review in the future (see http://llvm.org/docs/Phabricator.html#phabricator-request-review-web).
It is easier to do a proper review when there is some more context available in Phabricator.
Comment Actions
Thanks for the feedback, and my apologies for not providing additional context in the diff. The updated patch:
- I added an 'isDebugValue' check to LivePhysRegs::stepForward, I was hesitant to add this, as it never seemed problematic, but it doesn't seem wrong to add the check here either.
- I made the test a .mir test instead of .ll. I tried to reduce this as much as possible.
Comment Actions
This looks fine to me, but I'd wait a couple of days to see if Matthias has any concerns.
| lib/CodeGen/LivePhysRegs.cpp | ||
|---|---|---|
| 60 | We should probably skip debug value MIs here as well. The current solution of skipping MI.isDebugValue() is skipping instructions. | |
Comment Actions
Summary and title should probably be updated (as this impacts more than stepBackwards now).
Comment Actions
@bjope's comment about checking isDebug is a valid concern, but I think it goes outside of the scope of this patch. I don't remember seeing any checks for isDebug in other code, and going that path would probably need an agreement from more people that this is indeed the right thing to do.
Comment Actions
Thanks for the feedback @bjope, @kparzysz. I think @bjope has a good point regarding the MI Operands.
I think we can solve the same issue I'm trying to solve here, by just adding a isDebug check to the operands in addUses.
I'd also add the same check to removeDefs. That would make this patch here moot. If you think that is the more correct
thing to do, and it sounds like it might be, then I'd rather do that.
Comment Actions
I thought it would be a more widespread problem, but isDebug is actually checked in the nodbg iterator in MRI.
If you want, go ahead with the change to check isDebug instead.
Comment Actions
If you do not think it is too much trouble, then I'd welcome the alternative approach.
But skipping early on the MI.isDebugValue() in stepForward/stepBackward is OK as well.
In case LivePhysRegs::addUses is used from other methods (now or in the future) it would be good to make sure that it ignores isDebug() operands. Or at least add a comment in the method description that any user should make sure to check MI.isDebugValue() before the call.
Comment Actions
But skipping early on the MI.isDebugValue() in stepForward/stepBackward is OK as well.
In case LivePhysRegs::addUses is used from other methods (now or in the future) it would be good to make sure that it ignores isDebug() operands. Or at least add a comment in the method description that any user should make sure to check MI.isDebugValue() before the call.
The only calls to addUses are in LivePhysRegs.cpp, but I'm not opposed to future-proofing. Let me test out the proposed changes, and if that works out, I'll update this patch.
Comment Actions
Added debug operand checks (isDebug) to: LivePhysRegs: addUses, removeDefs, and stepForward.
This is a more specific patch that addresses the description in the original summary: Avoiding the situation where DBG_VALUE instructions influence codegen.
Comment Actions
I'm following up with this patch. Do the most recent changes that I made (adding the isDebug check on the operands, instead of the instruction), look good? Thanks.