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

Thu Apr 22 03:31:30 PDT 2021

Orlando added a comment.

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 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.

>> 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);
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100845