[PATCH] D79428: [WebAssembly] Added Debug Fixup pass

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added a comment.

> @aheejin I didn't think such a test was necessary, since a) the current code is not explicitly dependent on this adjacency anymore, b) we have code elsewhere (e.g. WebAssemblyDebugValueManager) that does assume this adjacency, so if there's any def + dbg_value that is not adjacent, this likely will require RegStackify to be fixed, and more likely this pass will continue to work. And c) we don't have an example yet where this is even produced.

`WebAssemblyDebugValueManager` does not assume this adjacency. It calls <https://github.com/llvm/llvm-project/blob/96d85726b0fc3c5154265cea156483a3f63ebe21/llvm/lib/Target/WebAssembly/WebAssemblyDebugValueManager.cpp#L23> `MachineInstr::collectDebugValues` <https://github.com/llvm/llvm-project/blob/96d85726b0fc3c5154265cea156483a3f63ebe21/llvm/lib/CodeGen/MachineInstr.cpp#L2123-L2138>, which traverses the whole BB to collect debug values instructions for a matching register. But when I change the order of `llvm.dbg.value`s in your example and run it with `-print-machineinstrs`, somehow instruction selection manages to emit `DBG_VALUE`s after their defs. Not sure it is always guaranteed though, as in the link @dschuff pasted. Anyway, to test this case we need an mir test case, which I think is an overkill too.

But I still think reverse traversing of `Stack` is worth it, given that the register will be most likely to be on top of the stack? (This is also a thing I suggested, but you didn't answer that one)

And on the other note, in general, even when you think some of my suggestions are unnecessary, I'd greatly appreciate if you answer them by explaining why they are unnecessary, rather than just ignoring them.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79428/new/

https://reviews.llvm.org/D79428