This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking] Fix fragment error for some DSE-shortened stores
ClosedPublic

Authored by Orlando on Apr 17 2023, 9:01 AM.

Details

Summary

shortenAssignment inserts dbg.assigns with fragments describing the dead part of a shortened store after each dbg.assign linked to the store.

Without this patch it doesn't take into account that the dead part of a shortened store may be outside the bounds of a variable of a linked dbg.assign. It also doesn't correctly account for a non-zero offset in the address modifying DIExpression of the dbg.assign (which is possible for fragments now even though whole variables currently cannot have a non-zero offset in their alloca).

Fix this by moving the dead slice into variable-space and performing an intersect of that adjusted slice with the existing fragment.

This fixes a verifier error reported when building fuchsia with assignment tracking enabled: https://ci.chromium.org/ui/p/fuchsia/builders/ci/clang_toolchain.ci.core.x64-release/b8784000953022145169/overview

Diff Detail

Event Timeline

Orlando created this revision.Apr 17 2023, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2023, 9:01 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Orlando requested review of this revision.Apr 17 2023, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 17 2023, 9:01 AM

I've added a couple of inline comments into one of the test to help explain the changes.

llvm/test/DebugInfo/Generic/assignment-tracking/dse/dse-after-memcpyopt-merge.ll
16–21

Prior to the patch each dbg.assign with a fragment overlapping the dead part of the store would get an unlinked dbg.assign placed after it.

With this patch that is still done in most cases, but if the dead slice is exactly the same as the existing fragment the dbg.assign itself is just unlinked. That's why on the left we have 2 CHECKs for both of the overlapped dbg.assigns. On the right we have one for each, plus I added in the other 2 non-overlapped dbg.assigns to improve test coverage.

48

I removed these dbg.assigns to reduce test clutter as they're not needed.

I guess we're finding all the twisty turney passages (all different) through the compiler that generate uncertainty over variable locations. On the one hand this is exactly what we want to be finding with assignment tracking, on the other hand it sucks that we're now needing to preserve so much detail. Discussed offline -- possibly the correct path is to relax the verifier requirements and just accept some imprecision (but not inaccuracy) in these intrinsics, it depends on whether this is the last corner case.

For this particular patch I think it's hard to track exactly what's going on given that there are now a bunch of different fragments flying around. Refactoring into some utilities in DebugInfoMetadata.cpp would be preferred to reduce the amount we're expanding the DSE file by, plus some ascii-art diagrams of exactly what fragments we're working with and manipulating. It's going to be hard to maintain in the future unless it's very plain. IMO as the terms "variable space" and "dest space" don't show up elsewhere in assignment tracking some more standardised nomenclature should be found.

llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp
492–509

Could you elaborate on what these terms mean here (not clear to me)

517–523

I feel there's not enough (mental) context here to work out what's going on -- relative to what?

530–531

Just to make me feel better, could you make the SmallVector a named variable please, simply for ease of reading.

llvm/test/DebugInfo/Generic/assignment-tracking/dse/shorten.ll
45 ↗(On Diff #514261)

Reason for deleting these?

Orlando updated this revision to Diff 514657.Apr 18 2023, 8:52 AM
Orlando marked 3 inline comments as done.

I guess we're finding all the twisty turney passages (all different) through the compiler that generate uncertainty over variable locations. On the one hand this is exactly what we want to be finding with assignment tracking, on the other hand it sucks that we're now needing to preserve so much detail. Discussed offline -- possibly the correct path is to relax the verifier requirements and just accept some imprecision (but not inaccuracy) in these intrinsics, it depends on whether this is the last corner case.

For this particular patch I think it's hard to track exactly what's going on given that there are now a bunch of different fragments flying around. Refactoring into some utilities in DebugInfoMetadata.cpp would be preferred to reduce the amount we're expanding the DSE file by, plus some ascii-art diagrams of exactly what fragments we're working with and manipulating. It's going to be hard to maintain in the future unless it's very plain. IMO as the terms "variable space" and "dest space" don't show up elsewhere in assignment tracking some more standardised nomenclature should be found.

Thank you for reviewing this. I've moved the functions to more appropriate places outside of DeadStoreElimination.cpp, addressed the nits, and added a large comment to the body of calculateFragmentIntersect. wdyt? I could probably chop the first paragraphs before # Example 1 out, but the additional context may be useful?

jmorse accepted this revision.Apr 18 2023, 9:40 AM

Cool, that's a lot easier to understand now -- it's a verbose comment block, but vital for understanding the full context of what's going on for future readers.

Again, this is a gnarly corner but let's hope it's a final one. I've got a curiosity comment, one grammar comment added plus an outstanding one on shorten.ll, but I think this is good to go in.

llvm/include/llvm/IR/DebugInfo.h
227–235

Is there a meaningful distinction between a zero-sized fragment and returning false? (just curious)

llvm/lib/IR/DebugInfo.cpp
1816
This revision is now accepted and ready to land.Apr 18 2023, 9:40 AM

Overall I think this looks fine, so thanks for addressing the issue. I had one small comment and one question I left inline.

Also, the issue was found building Fuchsia. We share infrastructure w/ Chromium, so the URL may have been misleading.

You shouldn't consider any of these blocking.

llvm/lib/IR/DebugInfo.cpp
1797–1800

Why does this take an optional<> as an output argument and return bool? wouldn't it make more sense to just return an optional<>?

1902

I know isPointerOffset() isn't part of this patch, but it's very surprising to find out that it doesn't return bool.

Orlando marked 3 inline comments as done.Apr 19 2023, 1:07 AM

Thank you both for the reviews!

Also, the issue was found building Fuchsia. We share infrastructure w/ Chromium, so the URL may have been misleading.

Aha, I see, I will fix that in the commit message, thanks.

llvm/include/llvm/IR/DebugInfo.h
227–235

Yeah - if the intersect can't be calculated for some reason you may wish to do something different (defensive/safe). Here's the code from DeadStoreElimination:

if (!at::calculateFragmentIntersect(DL, OriginalDest, DeadSliceOffsetInBits,
                                    DeadSliceSizeInBits, DAI,
                                    NewFragment) ||
    !NewFragment) {
  // We couldn't calculate the intersecting fragment for some reason. Be
  // cautious and unlink the whole assignment from the store.
  DAI->setKillAddress();
  DAI->setAssignId(GetDeadLink());
  continue;
}
// No intersect.
if (NewFragment->SizeInBits == 0)
  continue;

In this case we unlink the existing dbg.assign if the intersect couldn't be calculated (return false) to be "safe", or if the entire intersected fragment == the existing fragment (NewFragment == nullopt). In the case of no intersect (NewFragment->SizeInBits == 0) we don't do anything to the the dbg.assign, nor insert any new ones.

llvm/lib/IR/DebugInfo.cpp
1797–1800

It does feel a bit clunky but I wanted to distinguish between "the entire fragment or variable from DAI" (Result == nullopt) and "the intersect couldn't be calculated for some reason" (return false).

Using nullopt to represent the entire fragment or variable from DAI isn't completely weird because debug intrinsics containing no fragment info in their DIExpressions are considered to represent a location for the entire variable, and DIExpression::getFragment returns an optional<>.

Returning optional<> would result in a less clunky interface here. We'd have to move the "is the fragment the same size as DAI's fragment or variable" check done by the caller after instead (which is not unreasonable).

As you said you didn't intend your comments to be blocking please let me know if you'd like to see this change and I'll happily create another patch.

1902

It does feel like the name is slightly misleading. Maybe getPointerOffset would make more sense.

llvm/test/DebugInfo/Generic/assignment-tracking/dse/shorten.ll
45 ↗(On Diff #514261)

Ah I forgot to submit this inline comment after updating the patch. My reasoning was that I was in the neighbourhood, reading the test, dealing with failures while working on my patch. The lifetime intrinsics don't add anything and just clutter the tests. I've left the change out of this patch - I can always make the change separately.

This revision was landed with ongoing or failed builds.Apr 19 2023, 1:33 AM
This revision was automatically updated to reflect the committed changes.
Orlando marked 2 inline comments as done.
Orlando edited the summary of this revision. (Show Details)Apr 19 2023, 5:17 AM
hokein added a subscriber: hokein.Apr 19 2023, 7:02 AM

I was trying to fix the bazel failure https://buildkite.com/llvm-project/upstream-bazel/builds/59967#01879985-8d44-4041-9cd0-a1e41371208e (by adding the :Analysis dep to Core target), and discovered that this patch introduces a cyclic dependency between Core and Analysis:

$ bazel build --config=generic_clang @llvm-project//clang/unittests:ast_matchers_tests

Starting local Bazel server and connecting to it...
project//llvm:Analysis: cycle in dependency graph:
    @llvm-project//clang/unittests:ast_matchers_tests (940e5bfa4f289f5f81247f8a14276f4ff7b178b294d993a599b7dcdd4938fb45)
    @llvm-project//clang:ast (9e8b7842809f71769ff4024fa8ff44105697d8cd0cb0a5bcaa4aede8f4e50ff3)
    @llvm-project//clang:basic (9e8b7842809f71769ff4024fa8ff44105697d8cd0cb0a5bcaa4aede8f4e50ff3)
    @llvm-project//llvm:Instrumentation (9e8b7842809f71769ff4024fa8ff44105697d8cd0cb0a5bcaa4aede8f4e50ff3)
    @llvm-project//llvm:TransformUtils (9e8b7842809f71769ff4024fa8ff44105697d8cd0cb0a5bcaa4aede8f4e50ff3)
.-> @llvm-project//llvm:Analysis (9e8b7842809f71769ff4024fa8ff44105697d8cd0cb0a5bcaa4aede8f4e50ff3)
|   @llvm-project//llvm:Object (9e8b7842809f71769ff4024fa8ff44105697d8cd0cb0a5bcaa4aede8f4e50ff3)
|   @llvm-project//llvm:IRReader (9e8b7842809f71769ff4024fa8ff44105697d8cd0cb0a5bcaa4aede8f4e50ff3)
|   @llvm-project//llvm:AsmParser (9e8b7842809f71769ff4024fa8ff44105697d8cd0cb0a5bcaa4aede8f4e50ff3)
|   @llvm-project//llvm:Core (9e8b7842809f71769ff4024fa8ff44105697d8cd0cb0a5bcaa4aede8f4e50ff3)
`-- @llvm-project//llvm:Analysis (9e8b7842809f71769ff4024fa8ff44105697d8cd0cb0a5bcaa4aede8f4e50ff3)

The DebugInfo.cpp (in Core target) uses the functions from llvm/Analysis/ValueTracking.h (in Analysis target), thus Core target should depend on Analysis (we should probably add the Analysis dep to LLVMCore target in the cmake as well), but Analysis already
depends on Core.

Hi @hokein, thanks for this info.

The library dependency issue was fixed in D148698 which moves the functions needed by DebugInfo.cpp into LLVMCore (in Value.h). I fixed the bazel-build include issue by removing (the now unused / unneeded) #include llvm/Analysis/ValueTracking.h from DebugInfo.cpp in 04452e83ded9bfca0ff272fae9c537dde028daf2.

Hi @hokein, thanks for this info.

The library dependency issue was fixed in D148698 which moves the functions needed by DebugInfo.cpp into LLVMCore (in Value.h). I fixed the bazel-build include issue by removing (the now unused / unneeded) #include llvm/Analysis/ValueTracking.h from DebugInfo.cpp in 04452e83ded9bfca0ff272fae9c537dde028daf2.

Great, thanks for the fix!

k-mana added a subscriber: k-mana.Apr 19 2023, 10:54 AM