This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Refactor SalvageDebugInfo and SalvageDebugInfoForDbgValues
ClosedPublic

Authored by chrisjackson on May 13 2020, 7:26 AM.

Details

Summary

This is an alternative to D78398. In this diff we don't hide SalvageDebugInfoForDbgValues but we do simplify the salvaging interface and the algorithm in InstCombine.

Diff Detail

Event Timeline

chrisjackson created this revision.May 13 2020, 7:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 13 2020, 7:26 AM

Hi,

I left some nits inline but otherwise I think this is good.

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3332

we -> We

3355

Is this just printing the address of the clones? Shouldn't it be << *DIClones.back()?

llvm/lib/Transforms/Utils/Local.cpp
1629

While you're here you could updated salvaged -> Salvaged to match llvm style.

1659

nit: I think this would be clearer, but not a strong opinion at all.

if (salvaged)
   return true;

// <undef-code>
return false;
llvm/test/Transforms/InstCombine/debuginfo_add.ll
40

Am I right in thinking that this move is just a result of no longer calling reverse(...) in the debuginfo sink update code in instcombine?

Orlando added inline comments.May 19 2020, 2:29 AM
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
3361

Sorry I didn't pick this up with my other comments. I think this should be auto *DIIClone (or DbgVariableIntrinsic *DIIClone) because DIIClones is SmallVector<DbgVariableIntrinsic *, 2>?

chrisjackson marked an inline comment as done.May 19 2020, 8:07 AM
chrisjackson marked an inline comment as done.
  • Corrected comment
  • Removed unnecessary reference in iteration
  • Added missing pointer dereference

Are there any more suggestions please?

llvm/lib/Transforms/Utils/Local.cpp
1659

Yes I prefer this too.

Orlando accepted this revision.Jun 4 2020, 4:25 AM

Sorry for the delay, LGTM. Please wait 24h to see if anyone else has any further comments.

This revision is now accepted and ready to land.Jun 4 2020, 4:25 AM
This revision was automatically updated to reflect the committed changes.