This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][X86] Teach Optimize LEAs pass to handle debug values
ClosedPublic

Authored by andrewng on Mar 10 2017, 10:01 AM.

Details

Summary

This patch fixes an issue in the Optimize LEAs pass where redundant LEAs were not removed because they were being used by debug values. This issue was discovered when comparing the output of optimized code with and without debug info.

The debug value instructions are now ignored when determining whether LEAs are redundant. The debug values for the redundant LEAs are marked as undefined.

The intention is for a follow up patch which will attempt to preserve the debug values where possible.

Diff Detail

Event Timeline

andrewng created this revision.Mar 10 2017, 10:01 AM
dblaikie edited edge metadata.Mar 10 2017, 10:22 AM

Looks like this change does two things - fixes the "debug info affects optimization" and also does some work to improve optimized debug info quality? Might be worth separating this into two changes to make sure there's good test coverage/easier to review/etc?

lib/Target/X86/X86OptimizeLEAs.cpp
599

LLVM doesn't have much top level const like this - I'd probably leave it off.

Looks like this change does two things - fixes the "debug info affects optimization" and also does some work to improve optimized debug info quality? Might be worth separating this into two changes to make sure there's good test coverage/easier to review/etc?

I'm about to leave work now but I will look on Monday into whether separating this into two makes sense.

lib/Target/X86/X86OptimizeLEAs.cpp
599

No problem, I will remove the const.

Looks like this change does two things - fixes the "debug info affects optimization" and also does some work to improve optimized debug info quality? Might be worth separating this into two changes to make sure there's good test coverage/easier to review/etc?

I'm about to leave work now but I will look on Monday into whether separating this into two makes sense.

On further consideration, I think it makes sense to keep the patch as is because otherwise the first patch would be "lossy", i.e. the output would lose debug information. So in fixing the optimisation, ideally the debug information should be correct and preserved. However, if there are concerns with the "patching" of the debug information, then it might make sense to split the patch. What are your thoughts?

Looks like this change does two things - fixes the "debug info affects optimization" and also does some work to improve optimized debug info quality? Might be worth separating this into two changes to make sure there's good test coverage/easier to review/etc?

I'm about to leave work now but I will look on Monday into whether separating this into two makes sense.

On further consideration, I think it makes sense to keep the patch as is because otherwise the first patch would be "lossy", i.e. the output would lose debug information. So in fixing the optimisation, ideally the debug information should be correct and preserved. However, if there are concerns with the "patching" of the debug information, then it might make sense to split the patch. What are your thoughts?

I have looked in more detail into the DWARF debug generated by this patch (a side-effect of looking at another debug related codegen issue) and it turns out that the DWARF expression created isn't valid. I have a potential solution in progress, but it looks though generating "correct" DWARF for this particular scenario isn't as straightforward as it first appeared!

So as originally suggested, I will split this patch into one that fixes the codegen but removes the debug values and then see if I can create a reasonable patch to preserve the debug values.

andrewng updated this revision to Diff 92142.Mar 17 2017, 7:31 AM
andrewng edited the summary of this revision. (Show Details)

I have now split this patch and updated this review to contain the first part which fixes the codegen issue but doesn't preserve the debug values. I will work on a separate patch which will attempt to preserve the debug values.

dblaikie accepted this revision.Mar 17 2017, 9:13 AM
dblaikie added inline comments.
lib/Target/X86/X86OptimizeLEAs.cpp
602–603

I think you can use:

for (MachineOperand &MO : MRI->use_nodbg_operands(LastVReg))

here, probably?

This revision is now accepted and ready to land.Mar 17 2017, 9:13 AM

Thanks for the review.

lib/Target/X86/X86OptimizeLEAs.cpp
602–603

Yes, at first I thought so too, but inside the loop there is:

MO.setReg(First.getOperand(0).getReg());

which could affect the use iterator. So the original code which increments the use iterator before any changes is safer.

andreadb edited edge metadata.Mar 21 2017, 4:47 AM

I am going to commit this on Andrew's behalf.

This revision was automatically updated to reflect the committed changes.