Page MenuHomePhabricator

[WebAssembly] Fix updating/moving DBG_VALUEs in RegStackify
ClosedPublic

Authored by yurydelendik on Jan 7 2019, 11:13 AM.

Details

Summary

As described in PR40209, there can be issues in DBG_VALUEs handling when multiple defs present in a BB. This patch
adds logic for detection of related to def DBG_VALUEs and localizes register update and movement to found DBG_VALUEs.

Diff Detail

Repository
rL LLVM

Event Timeline

yurydelendik created this revision.Jan 7 2019, 11:13 AM

Haven't read the whole thing yet, but LLVM naming convention nits first:

lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
501 ↗(On Diff #180527)

Variable names should start with an uppercase letter

520 ↗(On Diff #180527)

Function names should start with a lowercase letter

539 ↗(On Diff #180527)

Function names should start with a lowercase letter

543 ↗(On Diff #180527)

i and e: Variable names should start with an uppercase letter. I know that feels particularly weird with for loop indices :(

fix code style

yurydelendik marked 4 inline comments as done.Jan 7 2019, 5:49 PM

I haven't finished reading other parts, but I think I should understand this AttachedDebugValues constructor first to proceed.

lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
477 ↗(On Diff #180594)

Please move this class to anonymous namespace above, because now this is in the global namespace

492 ↗(On Diff #180594)

By the way the loop can be replaced with MRI->def_instructions(Reg)

494 ↗(On Diff #180594)

It looks like you assume Op is a use and not a def in this loop, right? Maybe you should use not reg_operands but def_operands.

498 ↗(On Diff #180594)

So by this point it looks like you assume Op is a use and not a def, right? Maybe you should also add a clause to exclude defs as well, like

if (Op.isDef())
  continue;
498 ↗(On Diff #180594)

Oh I meant to delete this comment and I didn't. If you use MRI.def_operands(Reg)) at the top of the loop, you wouldn't need this. Sorry

499 ↗(On Diff #180594)

What is isDef set? Maybe you mean Defs set?

500 ↗(On Diff #180594)

associated

502 ↗(On Diff #180594)

Maybe we can use auto here?

506 ↗(On Diff #180594)

You can use Defs.count(&*I) to check for an element's existence

514 ↗(On Diff #180594)

Instrs is a set. Why do we need to test if it already exists in the set when we add it?

517 ↗(On Diff #180594)

Sorry, but I can't understand what this loop is doing. I thought what we were supposed to do to fix the 2nd point in PR40209 was we needed to figure out which DBG_VALUE instruction was associated with which def, in case there were multiple defs to a register within a BB.

What this loop seems to be doing is gather all use operands into Operands and get all defining instructions into Instrs, and I can't figure out how we are gonna figure out the mapping between MI and DBG_VALUE.

490 ↗(On Diff #180527)

Defs is a set. Why do we need to test if it already exists in the set when we add it?

And one thing I'm not sure is, surely this kind of logic, moving DBG_VALUEs with instructions when you move instructions around, will be needed in many places in the backend. Is there a more general way to handle things? How do other passes, such as many optimization passes that move instruction in lib/Codegen solve this problem? Have you taken a look at other passes in lib/CodeGen that moves code, such as MachineSink or MachineLICM?

And I also happened to notice there are functions called collectDebugValues and changeDebugValuesDefReg in MachineInstr class. Wouldn't using these functions simplify things a lot?

  • Use collectDebugValues
  • Rename AdjacentDebugValues and DefDIs
yurydelendik marked 13 inline comments as done.Jan 9 2019, 8:47 AM

I haven't finished reading other parts, but I think I should understand this AttachedDebugValues constructor first to proceed.

I over-complicated search for MI's DBG_VALUE, but essentially it is collectDebugValues -- using that instead. Thank you for finding and pointing this out. I could not find this initially.

lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
517 ↗(On Diff #180594)

The collectDebugValues defines relationship between MI and DBG_VALUE. For the updateRef we could use changeDebugValuesDefReg, but we may need to perform subsequent move of the updated regs, so I just kept move the same as the second part of the changeDebugValuesDefReg.

aheejin added inline comments.Jan 9 2019, 6:42 PM
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
43 ↗(On Diff #180853)

It's still not in the anonymous namespace I think..?
Also, not sure about the name AdjacentDebugValues.. which sounds like there are also non-adjacent DBG_VALUE instructions.

68 ↗(On Diff #180853)

I don't think we need to pass TII here.. It looks like you can call MachineFunction::CloneMachineInstrBundle to clone an instruction, which WebAssemblyInstrInfo::duplicate is calling anyway?

78 ↗(On Diff #180853)

Do we need to loop through all operands just to set the 0th operand? Why don't we just do getOperand(0).setReg(NewReg)?

  • Move MachineInstrAdjacentDebugValues
  • Use CloneMachineInstr
yurydelendik marked 6 inline comments as done.Jan 10 2019, 11:26 AM
yurydelendik added inline comments.
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
43 ↗(On Diff #180853)

As I understand DBG_VALUE concept, there can be DBG_VALUE without defining MI. Changed name to MachineInstrAdjacentDebugValues

68 ↗(On Diff #180853)

I'm not sure if CloneMachineInstrBundle will be a right choice here: if it's applicable to DBG_VALUE, and, also, RematerializeCheapDef tried to clone origin MI via TII->reMaterialize, probably breaking any bundle. Use CloneMachineInstr for now.

yurydelendik marked 2 inline comments as done.

Remove TII param from clone()

One thing I'm curious about is, is this kind of logic that gathers/moves DBG_VALUEs only needed in RegStackify? Don't you need that also in other passes? What about your new ExplicitLocal patch? If you need that in several place, how about moving this class to WebAssemblyUtilities or something and use it from many places?

lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
43 ↗(On Diff #180853)

Hmm, sorry, how about DebugValueHandler or something?

aheejin added inline comments.Jan 10 2019, 5:20 PM
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
43 ↗(On Diff #180853)

Or, DebugValueManager?

aheejin added inline comments.Jan 10 2019, 6:22 PM
test/DebugInfo/WebAssembly/dbg-value-move-reg-stackify.mir
7 ↗(On Diff #181150)

Can we specify the whole body so that it shows the two previous defs of %1 and their DBG_VALUEs don't change but only the last def w/ its DBG_VALUE does? Also, here the new register is %2, but I don't think it is guaranteed. So how about something like

# CHECK: %1:i32 = I32_WRAP_I64 %0
# CHECK: DBG_VALUE %1
# CHECK: %1:i32 = CALL_I32 @bar
# CHECK: DBG_VALUE %1
# CHECK: %[[NEWREG:.*]]:i32 = CALL_I32 @bar
# CHECK: DBG_VALUE %[[NEWREG]]
# CHECK: CALL_VOID @foo, %[[NEWREG]]

Oh and I think we need test cases for

  1. Rematerializing a def in another BB that has DBG_VALUE attached
  2. LOCAL_TEE case within a BB that has multiple defs to the same register (as in the current test case), to see clone() is working
  • Move WebAssembly::DebugValueManager into WebAssemblyUtilities.*
  • Fix testing of dbg-value-move-reg-stackify.mir
yurydelendik marked 3 inline comments as done.Jan 11 2019, 7:42 AM

Oh and I think we need test cases for

  1. Rematerializing a def in another BB that has DBG_VALUE attached

Yep, still working on this one

  1. LOCAL_TEE case within a BB that has multiple defs to the same register (as in the current test case), to see clone() is working

The test in llvm/test/DebugInfo/WebAssembly/dbg-value-move-2.ll . Would you like to see it in MIR format?

Revert bad test change

  • Fix trivially clonable instruction moving/cloning
aheejin added a comment.EditedJan 11 2019, 3:26 PM

Oh and I think we need test cases for

  1. Rematerializing a def in another BB that has DBG_VALUE attached

Yep, still working on this one

  1. LOCAL_TEE case within a BB that has multiple defs to the same register (as in the current test case), to see clone() is working

The test in llvm/test/DebugInfo/WebAssembly/dbg-value-move-2.ll . Would you like to see it in MIR format?

Is this a case that you rematerialize an instruction from another BB and move its DBG_VALUE along with it? I don't think so, because the earlier code didn't handle this case and this test was still passing.

And a minor thing: should we have a separate file for each of the test cases? Can we have both of them in dbg-value-move.ll, and merge mir cases in thie CL in one file?

lib/Target/WebAssembly/WebAssemblyUtilities.cpp
389 ↗(On Diff #181368)

Sorry, looking at the size of class, I guess it deserves an own file of its own, like DebugValueManager.cpp/h, other than putting this into Utilities. Sorry for going back and forth.

And another question: I haven't been a reviewer for your other debug info related CLs so I don't know, but are there any other places or your pending CLs that can make use of this new class?

And another question: I haven't been a reviewer for your other debug info related CLs so I don't know, but are there any other places or your pending CLs that can make use of this new class?

I check pass CLs, and looks like they do not need DebugValueManager functionality. There will be https://reviews.llvm.org/D52634 and this will benefit from having the DebugValueManager. Also there is an issue I was going to report later, that might need such functionality.

  • Fix WebAssemblyDebugValueManager::move
  • Remove DebugValueManager::reMaterialize
aheejin added inline comments.Jan 11 2019, 6:10 PM
lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp
13 ↗(On Diff #181396)

Please fix file name and description: it still says WebAssemblyUtilities.cpp

47 ↗(On Diff #181396)

For move and clone functions, in the current code I think we are reversing the order of DBG_VALUE instructions if there are multiple. I'm not sure if we should preserve the order of those, but unless there's reason not to, I guess it's better to preserve the order? We can do this by simply reversely iterating the DbgValues vector.

lib/Target/WebAssembly/WebAssemblyDebugValueManager.h
19 ↗(On Diff #181396)

Do we need this here? Can we move this to cpp file?

aheejin added inline comments.Jan 11 2019, 10:53 PM
lib/Target/WebAssembly/WebAssemblyDebugValueManager.h
19 ↗(On Diff #181396)

Looks like we don't need this in cpp file either. We just need a declaration
class MachineInstr;

24 ↗(On Diff #181396)

I don't think we need these two declarations?

  • Prune unused #include
  • Fix comments
  • Reduce default size of vector to 2
  • Reverse insert order for move/clone
yurydelendik marked 5 inline comments as done.Jan 14 2019, 6:39 AM
aheejin added inline comments.Jan 14 2019, 4:10 PM
lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp
2 ↗(On Diff #181549)

This now exceeds 80 cols.. you can delete some -s to make it fit.

lib/Target/WebAssembly/WebAssemblyDebugValueManager.h
13 ↗(On Diff #181549)

Nit: Is any functionality of this class actually WebAssembly specific? I don't think so... Actually we can have something like this in CodeGen/ or wherever, but putting that there might take some more work or take some more convincing of people, so I'm not necessarily suggesting it now, but anyway, this class does not contain anything wasm-specific.

yurydelendik marked an inline comment as done.Jan 14 2019, 4:14 PM
yurydelendik added inline comments.
lib/Target/WebAssembly/WebAssemblyDebugValueManager.h
13 ↗(On Diff #181549)

It will contains WebAssembly specific methods in the future, when we will try to replace virtual registers with WebAssembly specific locations.

aheejin added inline comments.Jan 14 2019, 4:17 PM
lib/Target/WebAssembly/WebAssemblyDebugValueManager.h
2 ↗(On Diff #181549)

This exceeds 80 cols too

Top comment fix

aheejin accepted this revision.Jan 14 2019, 4:49 PM

LGTM! Thanks.

This revision is now accepted and ready to land.Jan 14 2019, 4:49 PM
This revision was automatically updated to reflect the committed changes.
yurydelendik marked 2 inline comments as done.