[debugify] fails from Combine redundant instructions and SROA #37290
Comments
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. |
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.
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. |
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.
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.
Ah, right!
Yes, that would be great. |
|
The SROA part of this bug is resolved with this patch: Since Vedant relaxed the -check-debugify in r335647 the above should no longer |
|
Great, then this is fixed. |
|
mentioned in issue #37301 |
Extended Description
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
The text was updated successfully, but these errors were encountered: