[PATCH] D138943: [WebAssembly] Enable LiveDebugValues analysis

[Heejin Ahn via Phabricator via llvm-commits]

aheejin added inline comments.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/LiveDebugValues.cpp:75-79
-  MachineFunctionProperties getRequiredProperties() const override {
-    return MachineFunctionProperties().set(
-        MachineFunctionProperties::Property::NoVRegs);
-  }
-
----------------
jmorse wrote:
> Doing this makes sense for WASM, but check that there are no virtual registers remains an important sanity check for other targets -- mixing vregs and physregs would definitely generate broken results. Would you be able to add an assertion somewhere else for everything but WASM? (I don't think this method can know what the target arch is).
Added an assertion in the beginning of `LiveDebugValues::runOnMachineFunction`.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp:742
-                      [](auto RegNo) {
-                        return RegNo < LocIndex::kFirstInvalidRegLocation;
-                      }) &&
----------------
dschuff wrote:
> aheejin wrote:
> > dschuff wrote:
> > > aheejin wrote:
> > > > dschuff wrote:
> > > > > So I assume this is being removed because we are using VRegs instead of physregs, and VRegs have arbitrary large unsigned values (greater than 1 << 30). 
> > > > > Looking at what that might mean: It seems that `LocIndex` is relying on this in some way.
> > > > > Aside from there, the other place register values are checked against `kFirstInvalidRegLocation` is in `getUsedRegs`, where it seems to only collect values in the valid physreg range. so it seems that `getUsedRegs` will never return VRegs. It seems that `getUsedRegs is only used in the case of RegMask machine operand (which IIRC wasm doesn't use and which only make sense for physregs anyway).
> > > > > 
> > > > > So... I guess this seems like overall it should work? It does seem odd that we are using LocIndex but violating one of its primary assumptions. Is there anything else that could potentially go wrong?
> > > > I deleted this thinking simply like 1. this assertion failed on Wasm and 2. Wasm used virtual registers, so the assertion failure made sense. But yeah, this doesn't feel very satisfactory after all.
> > > > 
> > > > I don't think this will cause any functional problems in other archs. They are all using all physical registers after all. (This analysis runs at the very end of the backend pipeline.) But the funny thing is, Wasm is NOT supposed use `RegisterKind` at all. What does it even mean when debug info resides within a virtual register (which doesn't really exist in Wasm physically) at the end of compilation? All usable debug info should be either in a local or a stack location, or an immediate. So encountering a (virtual) register here in Wasm basically means it is a dangling debug info, which will be ignored when written to binary.
> > > > 
> > > > I'm not sure we can consider this kind of dangling `DBG_VALUE`  hanging around a critical bug to fix. While it is a bug, it can happen quite often, when a pass does not handle its corresponding `DBG_VALUE` when transforming/deleting/splitting a normal instruction. Preserving as much as `DBG_VALUE` is more like an optimization problem; not preserving some of them doesn't mean the program should crash.
> > > > 
> > > > So in short, I had to disable this to make it run on many Wasm programs, which have some dangling `DBG_VALUE`s at the end. I think maybe more tidy solution here would be to make the pass ignore register-based `DBG_VALUE` in case of Wasm, and re-enable this assertion..? But to do that we have to check the architecture at some point, which feels a little ugly to me, but maybe that's better than this.
> > > > 
> > > > WDYT?
> > > Ah, I had forgotten that we aren't using VRegs here either (I guess I didn't realize that this pass runs after explicit locals). I think in that case it does make more sense to just say that this pass should not encounter register values with wasm at all, so the register code doesn't need to handle VRegs. Probably we should have some comment to that effect somewhere.
> > > 
> > > I guess it's a bit unfortunate that we'll have dangling register locations anyway. Is there a straightforward way to either ignore them explicitly or maybe drop them before or at the beginning of this pass?
> > How about dropping all of them at the end of `WebAssemblyDebugFixup` pass? It runs at the very end, and it also does tasks related to debug info, so adding something there doesn't seem like a stretch. Or we can add a new pass, but that pass will be tiny.
> > 
> > If you think doing that in `WebAssemblyDebugFixup` is a good idea, I'll make a CL that does that separately and will land it first before landing this, and re-enable the assertion here.
> If I'm understanding you right, any DBG_VALUEs that have Register operands after explicit locals are dead and can't be encoded into the binary anyway, right? So I think it makes sense to drop them, and WebAssemblyDebugFixup seems like a reasonable place to me.
Nullified the dangling register-based `DBG_VALUE`s in D139675, and revived this assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138943