Page MenuHomePhabricator

[WebAssembly] Move/clone DBG_VALUE during WebAssemblyRegStackify pass
ClosedPublic

Authored by yurydelendik on Jul 6 2018, 10:04 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

yurydelendik created this revision.Jul 6 2018, 10:04 AM
vsk added a subscriber: mattd.Jul 6 2018, 10:10 AM
mattd added inline comments.Jul 6 2018, 10:49 AM
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
472 ↗(On Diff #154430)

I'm wondering if we should check that MI is nullptr here, or maybe assert? I know parent MI's can be initialized to nullptr and can be cleared, but I'm not sure if that would ever be the case at this point.

534 ↗(On Diff #154430)

Minor nit: I'd suggest running clang-format here, usually formals split between two lines end up following the opening parenthesis, lining up with each other.

585 ↗(On Diff #154430)

I would reword this to be imperative: "Move or clone corresponding DBG_VALUEs to the 'Insert' location.

Changes:

  • add asserts for MI
  • run clang-format
  • comment rewording
yurydelendik marked 3 inline comments as done.Jul 9 2018, 7:31 AM

Fix DBG_VALUE instruction cloning.

mattd added a comment.EditedJul 11 2018, 7:00 PM

Thanks Yury, it looks like you addressed my immediate concerns. I didn't see anything glaringly wrong, when I looked at the original patch. You should add a few reviewers and tag this as "debug-info" by editing the revision and updating the "Tags" field. That should get your patch a bit more exposure. Do you happen to have a smaller/reduced reproducer? I'm wondering how much of the test can be trimmed down, for instance, you declared a variable 't' but it's unused.

  • Reducing the test case further: remove attributes, lifetime markers and non-"a" variable metadata.
  • Add test for MoveAndTeeForMultiUse changes

Do you happen to have a smaller/reduced reproducer?

It is really hard for me to create reduced test case. I modified the test cases to remove some metadata per https://reviews.llvm.org/D48994#1164085 recommendation.

Rebased on top of D48994

yurydelendik retitled this revision from [WIP] [WebAssembly] Move/clone DBG_VALUE during WebAssemblyRegStackify pass to [WebAssembly] Move/clone DBG_VALUE during WebAssemblyRegStackify pass.Aug 22 2018, 11:53 AM

Otherwise LGTM, although I'm not an expert in DebugInfo either.

lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
470 ↗(On Diff #162014)

I think this can just be something like for (const auto &Op : MRI.reg_operands(Reg)) (and below)

Use for (const auto &Op :

Rebase (adjust test/DebugInfo/WebAssembly/dbg-value-move.ll)

dschuff accepted this revision.Sep 24 2018, 3:27 PM

otherwise LGTM

test/DebugInfo/WebAssembly/dbg-value-move.ll
10 ↗(On Diff #166762)

Was this generated with any optimization flags in clang? (maybe just also add the clang command line as well as the sources in these files?)

This revision is now accepted and ready to land.Sep 24 2018, 3:27 PM

Add instructions how to regenerate dbg-value-move*.ll files

yurydelendik marked an inline comment as done.Sep 25 2018, 11:44 AM
This revision was automatically updated to reflect the committed changes.

This seems to have broken some of the emscripten tests (binaryen2.test_poppler and binaryen2.test_the_bullet) that we run on the wasm waterfall:
https://logs.chromium.org/v/?s=chromium%2Fbb%2Fclient.wasm.llvm%2Flinux%2F35933%2F%2B%2Frecipes%2Fsteps%2FExecute_emscripten_testsuite__emwasm_%2F0%2Fstdout

This seems to have broken some of the emscripten tests (binaryen2.test_poppler and binaryen2.test_the_bullet) that we run on the wasm waterfall:
https://logs.chromium.org/v/?s=chromium%2Fbb%2Fclient.wasm.llvm%2Flinux%2F35933%2F%2B%2Frecipes%2Fsteps%2FExecute_emscripten_testsuite__emwasm_%2F0%2Fstdout

Looks like MRI.hasOneUse checks also DBG_VALUE register usages (that causes assert to fail). I need to check if use_nodbg_begin() will fix the issue. (FWIW OneUseDominatesOtherUses uses xx_nodbg_yyy operations in some places)

We we revert this or get it fixed? Its currently breaking the wasm waterfall: https://wasm-stat.us/console

We we revert this or get it fixed? Its currently breaking the wasm waterfall: https://wasm-stat.us/console

The fix just to pass tests is

diff --git a/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp b/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
index 5b6a91dbc9d..6583057e7fb 100644
--- a/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
+++ b/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
@@ -428,8 +428,10 @@ static bool OneUseDominatesOtherUses(unsigned Reg, const MachineOperand &OneUse,
         if (!TargetRegisterInfo::isVirtualRegister(DefReg) ||
             !MFI.isVRegStackified(DefReg))
           return false;
-        assert(MRI.hasOneUse(DefReg));
-        const MachineOperand &NewUse = *MRI.use_begin(DefReg);
+        auto It = MRI.use_nodbg_begin(DefReg);
+        assert(It != MRI.use_nodbg_end());
+        const MachineOperand &NewUse = *It;
+        assert(++It == MRI.use_nodbg_end());
         const MachineInstr *NewUseInst = NewUse.getParent();
         if (NewUseInst == OneUseInst) {
           if (&OneUse > &NewUse)

It will take care of WebAssemblyRegStackify, but it will be nice to see it at MRI::hasOneUse.

Let me know if I need to backup entire patch and attempt to fix hasOneUse first.

Not exactly sure what you mean; do you mean that hasOneUse should return true if there is only one non-debug use? That makes sense, maybe it should be hasOneNonDbgUse or something. But please either commit this fix and then follow up, or revert this patch and re-apply with hasOneUse fixed.

This still breaks two emscripten tests, test_the_bullet and test_poppler
In particular, it creates a wasm module that fails to typecheck, seemingly because of a reordering of instructions. Commenting out MoveDebugValues and CloneDebugValues is needed to avoid this.
The simplest reproduction I can find is:

wasm32-clang++ $EMSCRIPTEN/tests/bullet/src/BulletCollision/NarrowPhaseCollision/btPersistentManifold.cpp \
  -D__EMSCRIPTEN__=1 \
  -I$EMSCRIPTEN/tests/bullet/src/ \
  -nostdlib \
  -Xclang -nostdsysteminc \
  -Xclang -isystem$EMSCRIPTEN/system/include/libc \
  -Xlinker --no-entry \
  -Xlinker --allow-undefined \
  -O2 -g -o test.wasm

where EMSCRIPTEN is the path to emscripten's main directory (containing emcc.py et al). This creates a test.wasm that is invalid, quickly verifiable with wabt's wasm2wat

I think this has unfortunately caused a regression :( -- https://bugs.llvm.org/show_bug.cgi?id=39158

er sorry, looks like some problems may already be known!

Forgot to add, need to add __attribute__((used, visibility("default"))) to btPersistentManifold::clearUserCache in that cpp file

aheejin added a comment.EditedJan 2 2019, 9:00 PM

It looks DBG_VALUE instruction handling in RegStackify has several problems.

  1. MoveDebugValues and UpdateDebugValuesReg do not seem to consider the case where there are multiple defs to a single register within a BB. Currently they treat all DBG_VALUE instruction to a register as a group and move / update them together whenever any one of the definitions of the register in a BB is stackified. A sample test case with multiple defs within a BB that fails is attached in PR40209.

2-3 are written inline.

I also filed PR40209 with the same contents for easy tracking.

llvm/trunk/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
486

Here Op.setReg(NewReg) modifies the iterator MRI.reg_operands(Reg) while traversing it. So this ends up hitting only the first element and bailing out. This function has problems even without this bug because of the 1 above though.

542

This function does not consider a case that an instruction to be rematerialized is in another BB. Even though RegStackify mostly work within a BB, when a def is trivially rematerializable, i.e., const_i32 instruction, that instruction can be another BB and can be copied to this BB. But CloneDebugValues limit the search within this BB by MI->getParent() == &MBB clause, so in that case the debug value for the rematerialized instruction will not be cloned. A test case can be made trivially, so I'm not gonna attach an mir file for that, but it should look like this:

bb1:
  %3 = const_i32 3 ; This is trivially rematerializable
  ...

bb2:
  ...
  use %3