This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Redesign DebugValueManager
ClosedPublic

Authored by aheejin on Mar 23 2023, 10:53 AM.

Details

Summary

The current DebugValueManager, which is mostly used in RegStackify,
simply sinks DBG_VALUEs along when a def instruction sinks.
(RegStackify only does sinks; it doesn't do hoists.)

But this simple strategy can result in incorrect combinations of
variables' values which would have not been possible in the original
program. In this case, LLVM's policy is to make the value unavailable,
so they will be shown as 'optimized out', rather than showing inaccurate
debug info. Especially, when an instruction sinks, its original
DBG_VALUE should be set to undef. This is well illustrated in the
third example in
https://llvm.org/docs/SourceLevelDebugging.html#instruction-scheduling.

This CL rewrites DebugValueManager with this principle in mind. When
sinking an instruction, it sinks its eligible DBG_VALUEs with it, but
also leaves undef DBG_VALUEs in the original place to make those
variables' values undefined.

Also, unlike the current version, we sink only an eligible subset of
DBG_VALUEs with a def instruction. See comments in the code for
details.

In case of cloning, because the original def is still there, we don't
set its DBG_VALUEs to undef. But we clone only an eligible subset of
DBG_VALUEs here as well.

One consequence of this change is that now we do sinking and cloning of
the def instruction itself within the DebugValueManager's sink and
clone methods. This is necessary because the DebugValueManager needs
to know the original def's location before sinking and cloning in order
to scan other interfering DBG_VALUEs between the original def and the
insertion point. If we want to separate these two, we need to call
DebugValueManager's sink and clone methods before
sinking/cloning the def instruction, which I don't think is a good
design alternative either, because the user of this class needs to pay
extra attention when using it.

Because this change is fixing the existing inaccuracy of the current
debug info, this reduces the variable info coverage in debug info, but
not by a large margin. In Emscripten core benchmarks compiled with
-O1, the coverage goes from 56.6% down to 55.2%, which I doubt will be
a noticeable drop. The compilation time doesn't have any meaningful
difference either with this change.

Diff Detail

Event Timeline

aheejin created this revision.Mar 23 2023, 10:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 10:54 AM
Herald added subscribers: pmatos, asb, wingo and 5 others. · View Herald Transcript
aheejin requested review of this revision.Mar 23 2023, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2023, 10:54 AM

dbg-value-move-reg-stackify.mir was deleted because I think the new dbg-value-reg-stackify.mir covers what it used to cover.

aheejin edited the summary of this revision. (Show Details)Mar 23 2023, 3:32 PM
dschuff accepted this revision.Mar 23 2023, 5:32 PM
dschuff added inline comments.
llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp
217
297
298
llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.h
32

IIRC SmallVectorImpl<MachineInstr *> is preferred over an explicit size for parameters, since any size would work.

41
This revision is now accepted and ready to land.Mar 23 2023, 5:32 PM
aheejin edited the summary of this revision. (Show Details)Mar 23 2023, 6:51 PM
aheejin updated this revision to Diff 507942.Mar 23 2023, 7:09 PM
aheejin marked 5 inline comments as done.

Address comments

aheejin updated this revision to Diff 507944.Mar 23 2023, 7:12 PM

Add missing !s in tests + Remove irrelevant line info

llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.h
32

It doesn't compile. But SmallVector<MachineInstr *> (without the size parameter) seems to work instead.

dschuff added inline comments.Mar 24 2023, 10:18 AM
llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.h
32

oh interesting. out of curiosity I went back and found https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h

Maybe it needs to be updated, or maybe return values are different (or maybe default-construction, or operator= or whatever :)

aheejin updated this revision to Diff 508205.Mar 24 2023, 1:17 PM

Mark two methods const

aheejin updated this revision to Diff 508208.Mar 24 2023, 1:44 PM

Remove test clone_different_bb_same_place

Remove test clone_different_bb_same_place

I'm planning to make a follow-up CL that does nothing in case sinking is a no-op (i.e., sinking to the same place) to prevent unnecessary creation of DBG_VALUE $noregs, and I created sink_to_same_place and clone_different_bb_same_place to show the improvement in that next CL. But I think it doesn't apply to the cloning case, because RegStackify assumes cloning creates new instructions anyway whether the destination is same or not. So deleted that test.

This revision was automatically updated to reflect the committed changes.
hctim added a subscriber: hctim.Mar 29 2023, 1:11 PM

This might've broken builds? Either that, or one of your other three patches.

Nothing looks sanitizer specific, just noticed it on our sanitizer buildbots: https://lab.llvm.org/buildbot/#/builders/70/builds/35614

[310/371] Building CXX object lib/Target/WebAssembly/CMakeFiles/LLVMWebAssemblyCodeGen.dir/WebAssemblyDebugValueManager.cpp.o
FAILED: lib/Target/WebAssembly/CMakeFiles/LLVMWebAssemblyCodeGen.dir/WebAssemblyDebugValueManager.cpp.o 
CCACHE_CPP2=yes CCACHE_HASHDIR=yes /usr/bin/ccache /usr/bin/g++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE -D_LIBCPP_ENABLE_ASSERTIONS -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/b/sanitizer-x86_64-linux-autoconf/build/tsan_debug_build/lib/Target/WebAssembly -I/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/llvm/lib/Target/WebAssembly -I/b/sanitizer-x86_64-linux-autoconf/build/tsan_debug_build/include -I/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/llvm/include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -O2 -g -DNDEBUG -fvisibility=hidden  -fno-exceptions -funwind-tables -fno-rtti -UNDEBUG -std=c++17 -MD -MT lib/Target/WebAssembly/CMakeFiles/LLVMWebAssemblyCodeGen.dir/WebAssemblyDebugValueManager.cpp.o -MF lib/Target/WebAssembly/CMakeFiles/LLVMWebAssemblyCodeGen.dir/WebAssemblyDebugValueManager.cpp.o.d -o lib/Target/WebAssembly/CMakeFiles/LLVMWebAssemblyCodeGen.dir/WebAssemblyDebugValueManager.cpp.o -c /b/sanitizer-x86_64-linux-autoconf/build/llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp: In member function ‘llvm::SmallVector<llvm::MachineInstr*> llvm::WebAssemblyDebugValueManager::getSinkableDebugValues(llvm::MachineInstr*) const’:
/b/sanitizer-x86_64-linux-autoconf/build/llvm-project/llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp:210:10: error: could not convert ‘SinkableDbgValues’ from ‘SmallVector<[...],1>’ to ‘SmallVector<[...],6>’
  210 |   return SinkableDbgValues;
      |          ^~~~~~~~~~~~~~~~~
      |          |
      |          SmallVector<[...],1>
llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp