Page MenuHomePhabricator

[DbgInfo] Attempt to fix bug 37149
Needs ReviewPublic

Authored by tyb0807 on May 8 2018, 1:43 PM.




This is only for demonstrating my idea. I don't know the LLVM structures really well (yet), so my implementation is quite hackish (make SlotIndex::getIndex() public for example). So please tell me:
1, Whether my *idea* and my understanding of the problem are correct,
2, What should be the right way to implement the solution.

Thank you all for your help

Diff Detail


Event Timeline

tyb0807 created this revision.May 8 2018, 1:43 PM
aprantl added a subscriber: debug-info.

Could you please update the comment explaining what the new check is checking for?

Urgh, sorry but ... what new check? The only check is just transforming SmallSet::count() into SmallDenseMap::find() + comparing with SmallDenseMap::end(), so just to check whether the interval start was trimmed to the lexical scope (which is already commented in L1210).

davide added a subscriber: davide.May 9 2018, 8:14 AM

So IIUC you are remembering the SlotIndex that the interval was trimmed to instead of assuming that it was the one immediately preceding the beginning to the interval?

I think it would help to add a testcase to illustrate what the situation that this is improving on.


+ , and the SlotIndex they were trimmed to.?

Have you ran the LLVM regression tests? In particular, does the test added in D35953 (test/DebugInfo/X86/live-debug-variables.ll) still work as expected?

There is a pass called LexicalScopes. This computes the lexical scope for a debug location, consisting of a series of instruction ranges. The code added in D35953 made sure the computed intervals were within the debug location's lexical scope. If it wasn't it was trimmed to the lexical scope.

Looking at PR37149, you are saying that the locations are wrong due to the trimming. In this patch you are remembering the original untrimmed start index, and using that instead of the trimmed one. On the surface this appears to (partially) undo D35953. However, I can see why the test added in D35953 may still pass (this is why I want you to confirm whether you ran the tests). D35953 was concerned with the case where a live interval could be incorrectly split during register allocation. Effectively, this change means we use the trimmed intervals during register allocation, but reinstate the untrimmed starts (not the ends) when re-inserting the debug values (the call to emitDebugValues() actually occurs during the Virtual Register Rewriter).

My main concern is ensuring that D35953 still passes. However, I am worried that for the trimmed locations to be wrong, the lexical scopes must also be wrong. I'll be interested to see what the lexical scopes are for your testcase in PR37149.

I'll be interested to see what the lexical scopes are for your testcase in PR37149.

To be clearer, I'd like a reduced testcase, and instructions how to demonstrate the issue. Thanks! This could be used as the basis for a future test.

@aprantl Yes that is exactly what I am doing.
@rob.lougher As you pointed out, I was hoping that this change would not change the splitting in RegAlloc, but apparently it is not the case. The test added in D35953 does not pass anymore. Instead of

[0x000000000000001f, 0x000000000000003c): DW_OP_reg3 RBX

we now have

[0x000000000000000e,  0x0000000000000013): DW_OP_reg2 RCX
[0x0000000000000013,  0x000000000000003c): DW_OP_reg3 RBX

I do not understand why though... RegAlloc should not be broken this way :(. I'll update a reduced test case reflecting the PR37149's issue so you can tell me how it should be fixed.

Thanks for the help!


I've posted a reduced test case in

@rob.lougher Btw, I have taken a look at the test added in D35953 (test/DebugInfo/X86/live-debug-variables.ll). We always have 2 DBG_VALUE instructions:

DBG_VALUE debug-use $ecx, debug-use $noreg, !"i4", ...
DBG_VALUE $ebx, debug-use $noreg, !"i4", ...

The only difference is just the placement of the second DBG_VALUE in the MIR. So IIUC, we should always expect 2 location entries in the debug_loc section, shouldn't we?