Page MenuHomePhabricator

[DebugInfo] Use variadic debug values to salvage BinOps and GEP instrs with non-const operands
Needs ReviewPublic

Authored by StephenTozer on Wed, Nov 18, 8:50 AM.

Details

Summary

This patch follows the complete implementation of variadic debug values, making use of them to salvage arithmetic operations with two or more non-const operands; this includes the GetElementPtr instruction, and most Binary Operator instructions.

The majority of the real work in this patch is in Local.cpp, where the salvage function has been expanded in complexity for GEP and BinOp instructions, which have thus been refactored out into separate functions. The GEP implementation required the creation of a new function collectOffset which behaves similarly to accumulateConstantOffset, except that in addition to any constant offset it also fetches any non-constant offsets and returns them in a map of [SSA Value -> Factor].

Outside of these larger changes, the remainder of this patch consists of small utility functions used in salvaging, and updating uses of the salvage functions. Not all places that use salvaging have been updated to make use of variadic salvaging yet (such as salvaging in ISel), as additional changes need to be made in those areas to support this form of salvaging. They should not be difficult to implement in future, however.

Diff Detail

Event Timeline

StephenTozer created this revision.Wed, Nov 18, 8:50 AM
StephenTozer requested review of this revision.Wed, Nov 18, 8:50 AM
StephenTozer added inline comments.Wed, Nov 18, 8:55 AM
llvm/test/Transforms/Reassociate/undef_intrinsics_when_deleting_instructions.ll
6

A note for reviewers on the changes made to this test: this test has been modified to prevent it failing after this patch is applied. The failure introduced by this patch is not an error, but the opposite: the test relies on intrinsics being made undef when the instruction they relied on is deleted, but as of this patch we are able to salvage them so they will not be set undef. In order to preserve the spirit of this test, I changed it to instead reassociate float arithmetic instructions, which cannot currently be salvaged and so results in the intrinsic being undef'd like before.

dstenb added a subscriber: dstenb.Wed, Nov 18, 9:24 AM

Exciting!

I took the liberty to add some review comments whilst familiarizing myself with the code.

llvm/lib/IR/DebugInfoMetadata.cpp
1369

Nit: std::max(Result, ExprOp.getArg(0) + 1) ?

llvm/lib/Transforms/Utils/Local.cpp
1759–1760

Should this say DIExpression perhaps? DWARFv5 supports binary operations on base types wider than 64 bits, but LLVM does not support such expressions.

1872

Could the creation of this utility function and getSalvageOpsForBinOp be done as a preceding NFC refactoring commit? Just to make it slightly easier to see what the difference in behavior is.

(minor drive-by comment, but I'm not the right person to do more in depth review of this patch, unfortunately)

llvm/test/Transforms/Reassociate/undef_intrinsics_when_deleting_instructions.ll
6

Any chance of using something that'd be even more robust by using, for instance, a call to an external 'pure' function where the return value is used to define the debug value, but is otherwise unused - there would the instruction could be deleted but there would be no way the value could be reconstituted no matter how much we improve debug info in the future

Thanks!! So, this should be the most important one :) I would really like to see some measurements (e.g. llvm-locstats) before/after this patch.
Have you tried building llvm-project itself with this?

(Neither arc patch D91722 nor curl -L 'https://reviews.llvm.org/D91722?download=1' | patch -p1 can apply the patch locally. Can you upload via arc diff or git format-patch?)

This is nice!

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

Nit: Instead of having 20 calls to push_back, would it make sense to have a helper function that just does switch(opcode) { case Instruction::Add: return dwarf::DW_OP_plus; ...}?