[PATCH] D67217: [Debuginfo][Instcombiner] Do not clone dbg.declare in TryToSinkInstruction()

[Adrian Prantl via Phabricator via llvm-commits]

aprantl added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3173-3175
+        DII->setOperand(
+            0, MetadataAsValue::get(I->getContext(),
+                                    ValueAsMetadata::get(I->getOperand(0))));
----------------
avl wrote:
> aprantl wrote:
> > jmorse wrote:
> > > As I understand it, this works right now because the only sinkable instructions that dbg.declares point at are casts of other things (like arguments). This is probably fine -- but if dbg.declares operand rules ever change or someone writes IR that doesn't match that expectation, getOperand/setOperand could fail and it won't be clear why.
> > > 
> > > IMO it'd be good to add something like
> > > 
> > >     if (!isa<Cast>(I))
> > >       continue; // dbg.declare points at something it shouldn't
> > > 
> > > so that the failure mode is a debug use-before-def (which I don't believe is an error) rather than a crash.
> > We could probably simplify the model by enforcing that dbg.declares always point at allocas or function arguments directly, That would make code like this one look less murky.
> @aprantl how should this be done in a procedural manner? I mean - Could I create a patch for verifier, which would check "that dbg.declares always point at allocas or function arguments directly" and then fix all places ? Or that question should be previously discussed somehow? 
It may be best to proceed even slower than that. Changing what we accept inside of dbg.declare potentially means that all sorts of producers of LLVM need to be updated, so we need to be mindful of that. I think it would be reasonable to only let dbg.declare point inside an alloca or inside a (stack-allocated) function argument/sret value. But it might be too strict to demand that there is a 1-1 correspondence between allocas and dbg.declares, I think it might be reasonable to also allow bitcast and getelementptr operations to support clients like ASAN — as long as it points into a stack slot.

So I'd start by writing a patch that
- updates LangRef.rst / SourceLevelDebugging.rst
- verifies that the argument of a dbg.declare fullfills these rules
- see if we have any failures (running the llvm testsuite, compiling clang at -O0, -O3, -O1+asan):
- - if they are obvious mistakes, fix them
- - otherwise come back here to discuss what we found
- from there we can gradually tighten the requirements.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67217