[PATCH] D36596: [InstCombine] Don't convert all dbg.declares to dbg.values

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In https://reviews.llvm.org/D36596#841456, @aprantl wrote:

> > I think this patch is an incremental step towards what I sketched out. In the long run, the frontend wouldn't emit dbg.declare, it would emit dbg.value+DW_OP_deref.
>
> That's something I'm not yet sure about. With a dbg.declare it is very clear that the location is not just describing the value of the source variable, but can also be written to by the debugger, when the user wants to assign the variable a new value. The DWARF standard call this implicit vs. explicit location descriptions. We don't really distinguish between these two cases in LLVM yet, but dbg.value vs dbg.declare seems to make sense for it.


Hm, I hadn't considered that. I skimmed section 2.6 of the dwarf 5 standard, and it talks about "single location descriptions" and "location lists". I assumed that the single location case was just an encoding size optimization, and didn't affect whether the variable could be assigned.

I think variable assignment will be even harder to make work than ensuring that we always present accurate debug variable information. Passes like GVN will eliminate loads that were present in the source program. If the user updates a variable, it may not be reflected in the load immediately on the next line. We might want to disable variable assignment completely when a function isn't marked `optnone`.

> That said, this is not something we need to decide on for this patch :-)
> 
> Do you think you could factor the code in this patch into llvm::salvageDebugInfo() (found in Local.cpp)? There doesn't seem to be anything specific to InstCombine and this will make it easier to apply it to other passes later.

Are you suggesting that `salvageDebugInfo` should call `ConvertDbgDeclareToDbgValue(StoreInst*)`? Seems doable.

---

I put together a test case that shows problems after this patch:

  void escape(int *);
  void g(void);
  void f(void) {
    int x = 1;
    g();
    x = 2;
    escape(&x);
  }

Without this patch, instcombine inserts dbg.value calls. After this patch, if you break on `g`, `x` will probably appear uninitialized. It probably isn't hard to fix DSE in this patch, but I suspect there is a long tail of other passes that delete stores as well.

I wasn't able to craft a test case that causes stack coloring to do anything mean.


https://reviews.llvm.org/D36596