[PATCH] D58726: [DebugInfo][Docs] Explicitly document how dbg.value intrinsics are interpreted in optimized code

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: docs/SourceLevelDebugging.rst:462
+
+  define i32 @foo(i32 %bar, i1 %cond) {
+    call @llvm.dbg.value(metadata i32 0, metadata !1, metadata !2)
----------------
Did you intend to leave out dbg.value in this example? 
Now it looks identical to the "Finally.." version below which is a little bit confusing.


================
Comment at: docs/SourceLevelDebugging.rst:487
+    %addoper = select i1 %cond, i32 11, i32 12
+    call @llvm.dbg.value(metadata i32 undef, metadata !1, metadata !2)
+    %toret = add i32 %bar, %addoper
----------------
I do not think that we "must" introduce the undef in this example. We might prefer to do it (because it is a simple solution).
But since there are no other context given here, and the value 0 is a valid state for the variable it isn't wrong to present the variable as having the value 0, up until it is incremented. Well, that is at least my opinion...

Although, if you for example add a function call like this:

  define i32 @foo(i32 %bar, i1 %cond) {
    call @llvm.dbg.value(metadata i32 0, metadata !1, metadata !2)
    br i1 %cond, label %truebr, label %falsebr
  truebr:
    %tval = add i32 %bar, 1
    call @dbg.value(metadata i32 %tval, metadata !1, metadata !2)
    call @gazonk()
    br label %exit
  falsebr:
    %fval = add i32 %bar, 2
    call @dbg.value(metadata i32 %fval, metadata !1, metadata !2)
    call @gazonk()
    br label %exit
  exit:
    %merge = phi [ %tval, %truebr ], [ %fval, %falsebr ]
    call @dbg.value(metadata i32 %merge, metadata !1, metadata !2)
    %toret = add i32 %merge, 10
    call @dbg.value(metadata i32 %toret, metadata !1, metadata !2)
    ret i32 %toret
  }

then we should get

  define i32 @foo(i32 %bar, i1 %cond) {
    call @llvm.dbg.value(metadata i32 0, metadata !1, metadata !2)
    call @llvm.dbg.value(metadata i32 undef, metadata !1, metadata !2)
    call @gazonk()
    %addoper = select i1 %cond, i32 11, i32 12
    %toret = add i32 %merge, %addoper
    call @dbg.value(metadata i32 %toret, metadata !1, metadata !2)
    ret i32 %toret
  }

In my example I think it could be wise to mark `!1` as undef before the call to gazonk. The value 0 isn't a valid state for `!1` at the call. Still not sure if we must do it.

Although I guess the transformation in neither example is done in one single step. We will probably salvage the dbg.value inside the `truebr`and `falsebr`blocks along the way while sinking. So it might be likely that we end up with the undef as in your example. However, I can't see that the undef "must" be there given your example (at least it doesn't help me understand why/when it must be added).


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58726