Created attachment 20469 [details] 1.ll We've been looking at cases where debugify currently fails. Here is an example (courtesy of Stephen Wilks) of a test-case that was reduced at source-level using the condition of "[SROA]: FAIL" being present in the debugify output. The reduced output is obviously somewhat silly with the assignment to qaz on the last line being completely redundant. I sadly don't have time to look more deeply at the moment, but before going further and reporting more failures I'd like to verify whether the presence of debugify fails should always be considered a bug, or whether in some cases a debugify fail could be expected in the presence of things like completely redundant operations or do we need to enforce some sanity checking on the testcases before and during reduction first? $ clang --version clang version 7.0.0 (trunk 335577) (llvm/trunk 335591) Target: x86_64-pc-windows-msvc Thread model: posix InstalledDir: e:\work\upstream-llvm\build-vs2015-native-ninja\bin $ cat 1.cpp int foo(int); void bar() { int baz[6] = {}; foo(baz[2]); long qaz = (long)baz; } $ clang -O0 -Xclang -disable-O0-optnone -S -emit-llvm 1.cpp -o 1.ll $ opt 1.ll -instcombine -sroa -disable-output -debugify-each WARNING: Missing line 2 WARNING: Missing line 8 WARNING: Missing line 9 WARNING: Missing line 10 ERROR: Missing variable 2 ERROR: Missing variable 8 CheckFunctionDebugify [Combine redundant instructions]: FAIL ERROR: Instruction with empty DebugLoc in function ?bar@@YAXXZ -- %baz.sroa.2 = alloca [3 x i32] WARNING: Missing line 1 WARNING: Missing line 2 WARNING: Missing line 4 WARNING: Missing line 5 CheckFunctionDebugify [SROA]: FAIL CheckFunctionDebugify [Module Verifier]: PASS
> I'd like to verify whether the presence of debugify fails should always be considered a bug [...] An error report from -check-debugify should always be considered a compiler bug. It would be cumbersome if we couldn't rely on this -- so much so that we should consider relaxing -check-debugify rather than allowing it to emit false positives. I'm not sure when I'll be able to look at this. I'll put it on my queue and take a stab at it unless someone beats me to it.
> An error report from -check-debugify should always be considered a compiler > bug. It would be cumbersome if we couldn't rely on this -- so much so that > we should consider relaxing -check-debugify rather than allowing it to emit > false positives. Perfect. Thanks! Being able to rely on that property greatly simplifies the task of bringing up some automatic test-case creation for debugify errors. It's likely that we'll be filing more bugs in the coming days as that comes online. I'll poke around internally to see if we can donate a bit more effort toward the cause of helping to actually fix some of them, rather than just raising them :).
I think this might be an instance where we should relax the 'ERROR: Missing variable 2' report from -check-debugify. InstCombine is just doing DCE. We could teach it to insert dbg.value(undef)'s as it does DCE, but this has no user-visible benefit I can discern. The DWARF is the same with/without the extra dbg.value.
I relaxed -check-debugify in r335647. I think one of Anastasis' patches may resolve the empty location bug. Anastasis, wdyt?
Most of the InstCombine issues come from some "ERROR: Missing variable x" reports in -check-debugify output. Indeed, most of the ones that I've investigated seem to be the result of DCE. But should we reduce this to a warning in all cases? It seems to me that it might not catch bugs from transformations that should keep the dbg.value but don't for some reason. OTOH I agree that false positives are worse, just like you said. > I think one of Anastasis' patches may resolve the empty location bug. Anastasis, wdyt? Those patches instruct clang to emit artificial DL in store, that mem2reg and SROA can then use in order to produce PHI's with DL and preserve the scope. In this particular case, the DI isn't generated from clang so I don't thing that my patches help. This looks like a case of a SROA's failure to preserve DL. It seems to me that the SROA generated alloca doesn't get any DL for some reason. I can have a look at it in greater detail if you agree.
(In reply to Grammenos Anastasis from comment #5) > Most of the InstCombine issues come from some "ERROR: Missing variable x" > reports in -check-debugify output. I think we need a better (i.e, higher-level) way to gauge where debug value loss is occurring. These individual messages are useful for tracking specific problems down, but don't help us prioritize parts of the compiler to work on. Per-pass DI loss statistics -- even if they do factor in expected behavior like DCE -- would help prioritize work a bit better. > Indeed, most of the ones that I've > investigated seem to be the result of DCE. But should we reduce this to a > warning in all cases? It seems to me that it might not catch bugs from > transformations that should keep the dbg.value but don't for some reason. If there were a reliable way to distinguish dbg.value loss due to DCE from loss due to replacement, we could keep the error. Unfortunately there isn't. FWIW the warning messages are still useful for tracking down bugs, just as the warning messages for missing DebugLocs are. > OTOH I agree that false positives are worse, just like you said. > > > I think one of Anastasis' patches may resolve the empty location bug. Anastasis, wdyt? > > Those patches instruct clang to emit artificial DL in store, that mem2reg > and SROA can then use in order to produce PHI's with DL and preserve the > scope. In this particular case, the DI isn't generated from clang so I don't > thing that my patches help. Ah, right! > This looks like a case of a SROA's failure to preserve DL. It seems to me > that the SROA generated alloca doesn't get any DL for some reason. I can > have a look at it in greater detail if you agree. Yes, that would be great.
The SROA part of this bug is resolved with this patch: https://reviews.llvm.org/D48640 Since Vedant relaxed the -check-debugify in r335647 the above should no longer FAIL. That is not to say that InstCombine doesn't drop DI when it shouldn't have as it is evidenced by the "Missing variable" warnings.
Great, then this is fixed.