[PATCH] D100845: [Debugify][Original DI] Test preservation of original debug var intrinsics in optimizations

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added a comment.

In D100845#2707961 <https://reviews.llvm.org/D100845#2707961>, @Orlando wrote:

> In D100845#2707872 <https://reviews.llvm.org/D100845#2707872>, @djtodoro wrote:
>
>> @Orlando Thanks for looking into this.
>>
>> In D100845#2705685 <https://reviews.llvm.org/D100845#2705685>, @Orlando wrote:
>>
>>> Hi @djtodoro I am looking at this (slowly, sorry!).
>>>
>>> IIUC this checks that the number of debug intrinsics for each non-inlined variable does not decrease after each optimisation pass. There are legitimate reasons for deleting debug intrinsics. `RemoveRedundantDbgInstrs`, for example, which is called in a few places. With that in mind I think I would prefer the table header "Number of bugs" to change as this number seems to be more of an indicator of possible bugs, rather than proof of their existence.
>>
>> Sure. That is why we treat it as "WARNING". The same is happening with dropping of dbg location attached to instructions -- there are places where we cannot salvage !dbg, and we treat it as a warning as well (until we add some super cool logic to distinguish what was reasonable drop or opposite).
>
> That makes sense, cool. Out of curiosity, did you run into many false positives before you found the ADCE bug (D100844 <https://reviews.llvm.org/D100844>)?

I am seeing some false positives for sure (most of these artifital vars locs are being removed after some "constant propagation" passes). We definitely need to investigate how to remove these false positives here (for both, instr and var locations).

> I investigated another couple of dropped locations this morning which both look like false positive results too. One was in ipsccp where a dead block which contains a dbg.value is deleted. The other was in Early CSE w/ MemorySSA, which looks like it was being flagged because it makes a dbg.value undef. I've made an inline comment about this last point - undef dbg.values are currently not counted by this patch and I'm not sure if that should be the case.

I don't know how (yet), but we can (somehow) mark some places in pipeline as "valid for dropping" or something like that. Please note that not all setting of "undef" is the only  thing we can do -- for example, calling the `salvageDebugInfo()` sometimes prevents a pass of making some llvm.dbg.value() first operand as undef.

>>> You might be able to reduce (*) the number of false positive results by ignoring "redundant" debug intrinsics in the count, though maybe that could be follow-up work?
>>
>> That could be follow-up for sure. Thanks. In addition, I am seeing a lot of artifitial variables location dropping (such as `__result`, `__s` etc.), and I am not sure if we should care about these.
>
> I think the artificial variables are still useful to us. It may not matter to users so much if the locations are incorrect, but for us the intrinsics just provide more test coverage for this tool. wdyt?





================
Comment at: llvm/lib/Transforms/Utils/Debugify.cpp:565
+            continue;
+          // Skip undef values.
+          auto Op = DVI->getVariableLocationOp(0);
----------------
Orlando wrote:
> Why skip undef values here? Skipping undef values in the count means that any pass that makes a debug intrinsic undef will be flagged.
> 
> While making a debug intrinsic undef can sometimes be suboptimal (i.e. where a salvage or value replacement is possible), I don't think it ever reduces the correctness of the debug info.
>While making a debug intrinsic undef can sometimes be suboptimal (i.e. where a salvage or value replacement is possible), I don't think it ever reduces the correctness of the debug info.
Hmmm... it does not affect the correctness, but it does affect completeness (e.g. in optimized code, variable should be covered for the places it is alive, but in many cases it is impossible). How are we going to find cases where we have missed to call `salvageDebugInfo()`?
In addition, we can add some checking levels -- e.g. there could be level that doesn't consider "undef" locations, which will reduce the number of false positives, for sure...




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100845