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.
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 26788 Build 26787: arc lint + arc unit
Event Timeline
Haven't read the whole thing yet, but LLVM naming convention nits first:
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp | ||
---|---|---|
469–470 | Function names should start with a lowercase letter | |
488 | Function names should start with a lowercase letter | |
492 | i and e: Variable names should start with an uppercase letter. I know that feels particularly weird with for loop indices :( | |
493 | Variable names should start with an uppercase letter |
I haven't finished reading other parts, but I think I should understand this AttachedDebugValues constructor first to proceed.
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp | ||
---|---|---|
469–470 | Please move this class to anonymous namespace above, because now this is in the global namespace | |
482 | Defs is a set. Why do we need to test if it already exists in the set when we add it? | |
484 | By the way the loop can be replaced with MRI->def_instructions(Reg) | |
486 | 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. | |
490 | 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; | |
490 | 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 | |
491 | What is isDef set? Maybe you mean Defs set? | |
492 | associated | |
494 | Maybe we can use auto here? | |
498 | You can use Defs.count(&*I) to check for an element's existence | |
506 | Instrs is a set. Why do we need to test if it already exists in the set when we add it? | |
509 | 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. |
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 [[ https://github.com/llvm-mirror/llvm/blob/e25b508818ed061f87d3a97bd3482dca9a726f66/lib/CodeGen/MachineInstr.cpp#L2077-L2092 | collectDebugValues ]] and [[ https://github.com/llvm-mirror/llvm/blob/e25b508818ed061f87d3a97bd3482dca9a726f66/lib/CodeGen/MachineInstr.cpp#L2094-L2102 | changeDebugValuesDefReg ]] in MachineInstr class. Wouldn't using these functions simplify things a lot?
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 | ||
---|---|---|
509 | 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. |
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp | ||
---|---|---|
44 | It's still not in the anonymous namespace I think..? | |
69 | 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? | |
79 | Do we need to loop through all operands just to set the 0th operand? Why don't we just do getOperand(0).setReg(NewReg)? |
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp | ||
---|---|---|
44 | As I understand DBG_VALUE concept, there can be DBG_VALUE without defining MI. Changed name to MachineInstrAdjacentDebugValues | |
69 | 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. |
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 | ||
---|---|---|
44 | Hmm, sorry, how about DebugValueHandler or something? |
lib/Target/WebAssembly/WebAssemblyRegStackify.cpp | ||
---|---|---|
44 | Or, DebugValueManager? |
test/DebugInfo/WebAssembly/dbg-value-move-reg-stackify.mir | ||
---|---|---|
8 | 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
- Rematerializing a def in another BB that has DBG_VALUE attached
- 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
Yep, still working on this one
- 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?
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.
lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp | ||
---|---|---|
14 | Please fix file name and description: it still says WebAssemblyUtilities.cpp | |
48 | 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 | ||
20 | Do we need this here? Can we move this to cpp file? |
- Prune unused #include
- Fix comments
- Reduce default size of vector to 2
- Reverse insert order for move/clone
lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp | ||
---|---|---|
2 | This now exceeds 80 cols.. you can delete some -s to make it fit. | |
lib/Target/WebAssembly/WebAssemblyDebugValueManager.h | ||
13 | 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. |
lib/Target/WebAssembly/WebAssemblyDebugValueManager.h | ||
---|---|---|
13 | It will contains WebAssembly specific methods in the future, when we will try to replace virtual registers with WebAssembly specific locations. |
lib/Target/WebAssembly/WebAssemblyDebugValueManager.h | ||
---|---|---|
2 | This exceeds 80 cols too |
This exceeds 80 cols too