Adding debug-info project tag.
Thu, Mar 22
Wed, Mar 21
I'm sorry. I don't have the bandwidth for this right now. Please do take it up, it'd be great to see this upstreamed! If we can get the base version upstreamed, I don't mind quickly iterating on smaller features / tests / things like that.
@bollu Are you still interested in working on this? I'm interested in getting this pass back into upstream LLVM. If you don't have time I'm willing to take over this if you like.
Tue, Mar 20
To see any difference you probably need to compile a heavily templated C++ program with lots of inlining. That said, I don't think that this patch will have a huge performance impact even there, since we don't propagate dbg.values beyond basic blocks outside of their lexical scopes.
DanglingDebugInfoVector is not a std::vector.
Rebased to latest on trunk.
Some small nits inlined, I've mostly looked at the non debug related parts. A lot of my comments are the MIPS .td parts can be summarized as: where possible, modify the underlying *_DESC or MXC1 classes where possible rather than wrapping the definition of the instruction with a 'isRegMove = 1'. There are some cases where it can't be done as the instruction description class is shared multiple other instructions which are not moves.
Updated test to recognize metadata id number.
Mon, Mar 19
I've changed patch to perform locally now by restricting copy instructions recognition to ones that copy to callee saved registers.
I've also added test for mips64 that I've managed to find without making any changes to MIR. Also I've extended this functionality for recognizing some of X86 and ARM instructions (I've marked some of instructions in copyPhysReg TII method). I suppose that this could be extended even more by labeling other instructions as isMoveReg but I'm not familiar with other architectures.
Thanks! LGTM with Reid's comment addressed.
Thanks, this seems like an important fix. :) Tons of dbg.values for redundant inlined copies of this use the same base value.
Yes, thanks Matt! LGTM.
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.
Unless Adrian has any more remarks, this LGTM. Thank you!
Sun, Mar 18
This landed as r327800. @gramanas, if you add "Differential Revision: <URL to the review>" to the last line of your commit message, Phabricator will automatically close the review for you when you commit.
Fri, Mar 16
Thu, Mar 15
Yes that should be fine.
Have you checked whether the debuginfo-tests repository needs to be updated, too?
So excited to see this happening! :)
This is the kind of commit that is bound to break a lot of stuff, so please watch all the bots carefully.
Tue, Mar 13
- Fix test label
Ah, I think there's still a problem with the test.
LGTM. Is it all right if I commit this for you, or would you like to set up an account and commit it yourself?
- fix redundant debugify test