This is an archive of the discontinued LLVM Phabricator instance.

Remove the obsolete "offset" parameter from @llvm.dbg.value
ClosedPublic

Authored by aprantl on Jul 27 2017, 10:54 AM.

Details

Summary

Time for some belated spring cleaning!

This patch removes the offset parameter from @llvm.dbg.value a.k.a. DbgValueIntrinsic.

There is no situation where this rarely-used argument cannot be substituted with a DIExpression and removing it allows us to simplify the DWARF backend and bits of SelectionDAG.

Note that this patch does not yet remove any of the newly dead code. I'm planning to follow this up with a couple of smaller cleanup commits.

Diff Detail

Repository
rL LLVM

Event Timeline

aprantl created this revision.Jul 27 2017, 10:54 AM

For those living downstream I should also note that the IR testcase updates are not strictly necessary, since the auto upgrade code also works for assembler input, but I think it is less confusing to have the testcases use the up-to-date version of the intrinsics.

dexonsmith edited edge metadata.Jul 27 2017, 11:16 AM

Should there also be an update to LangRef?

For those living downstream I should also note that the IR testcase updates are not strictly necessary, since the auto upgrade code also works for assembler input, but I think it is less confusing to have the testcases use the up-to-date version of the intrinsics.

I feel like it would be better to error, and require the upgrade. Is that not practical for intrinsics?

Should there also be an update to LangRef?

For those living downstream I should also note that the IR testcase updates are not strictly necessary, since the auto upgrade code also works for assembler input, but I think it is less confusing to have the testcases use the up-to-date version of the intrinsics.

I feel like it would be better to error, and require the upgrade. Is that not practical for intrinsics?

LLParser::ValidateEndOfModule() always calls UpgradeCallsToIntrinsic(), therefore special-casing just the debug info intrinsics would be inconsistent.

Should there also be an update to LangRef?

For those living downstream I should also note that the IR testcase updates are not strictly necessary, since the auto upgrade code also works for assembler input, but I think it is less confusing to have the testcases use the up-to-date version of the intrinsics.

I feel like it would be better to error, and require the upgrade. Is that not practical for intrinsics?

LLParser::ValidateEndOfModule() always calls UpgradeCallsToIntrinsic(), therefore special-casing just the debug info intrinsics would be inconsistent.

I think the call to UpgradeCallsToIntrinsic() should be removed -- it's a holdover from when LLVM actually supported old .ll files. But that smells like a different yak... no need to fix that here.

The code changes and test/Bitcode/upgrade-dbg-value.ll LGTM.

I spot-checked the other testcases, and couldn't find any that had anything other than i64 0. Is that expected? Or can you point me at a non-trivial one?

The code changes and test/Bitcode/upgrade-dbg-value.ll LGTM.

I spot-checked the other testcases, and couldn't find any that had anything other than i64 0. Is that expected? Or can you point me at a non-trivial one?

We have been phasing out any remaining use of the offset field and replaced it with DIExpressions. This is also why the upgrade merely strips any dbg.value intrinsics with nonzero offsets. It's effectively dead code.

dexonsmith accepted this revision.Jul 28 2017, 7:39 AM

The code changes and test/Bitcode/upgrade-dbg-value.ll LGTM.

I spot-checked the other testcases, and couldn't find any that had anything other than i64 0. Is that expected? Or can you point me at a non-trivial one?

We have been phasing out any remaining use of the offset field and replaced it with DIExpressions. This is also why the upgrade merely strips any dbg.value intrinsics with nonzero offsets. It's effectively dead code.

Got it. LGTM!

This revision is now accepted and ready to land.Jul 28 2017, 7:39 AM
This revision was automatically updated to reflect the committed changes.