Clang is barely using undef these days. LLVM, on the other hand, continues to add new tests with undef every day.
One of @regehr’s students (@leewei05) has been removing uses of br undef from the tests so they can be checked with Alive2. Even after removing hundreds of such cases(!), the usage of undef still increased!
(note that about 1/3 of LLVM’s tests triggers UB, and so Alive2 can’t verify whether the optimizations are correct or not).
I can’t stress enough how important it is to remove undef from LLVM. It’s existence makes many optimizations wrong. Every new use of undef is just extra work down the line and potentially more miscompilations.
TL;DR: Please don’t use undef in tests and avoid tests with UB!
Was bugpoint updated to generate poison values instead of undef? I know it’s deprecated in favor of llvm-reduce but it is probably still used for one reason or another.
I wonder if it’d be worthwhile to have a specific automated check/bot that undef doesn’t appear in test-cases in PRs?
Your data seems to show that some automated check might be needed to get the behavioural change to happen to no longer use undef in test cases?
No, bugpoint doesn’t emit undef anymore AFAICT (just grepped for Undef there). So the undef is either coming from manually-reduced test cases or from other tools out there.
That would be awesome! In the old days I had an alert in phabricator, and I played the role of the bot by sending messages manually
I don’t know how to add checks to github, but I think it would be great to have like an automatic review suggesting to replace undef with something else. It could even run Alive2 and warn that the test is fully UB.
I don’t know how to do that either, but I think that @tstellar and @DavidSpickett are probably the people with most experience on setting up new github bots that do checks.
Could the thing to look for be as simple as “is there a string “undef” anywhere in any of the changed lines in any *.ll files”?
Also, is any use of undef forbidden, or just the ones with UB? For example, according to examples in the language reference you create a struct value from scratch with insertvalue instructions on an undef: https://llvm.org/docs/LangRef.html#insertvalue-instruction
Oh dear, that example needs to be updated.
Fresh Structs/vectors should be created with poison. Most (all?) such code in LLVM has been changed already.
undef is not forbidden; there are still cases where it must be used (reading unitialized memory). Everything else should be transitioned to poison.
If you go the route of adding flags to the test suites that are run, that stuff is in llvm-project/.ci at main · llvm/llvm-project · GitHub and you can also trigger this from a draft PR (these builds are actually run on Buildkite at the moment but that detail shouldn’t be an issue for you).
Substrings and comments should be easy enough to ignore (at least for 99% of cases). As long as it’s treated as a warning and not a failure, some simple regex check (like ^[^;]+\bundef\b) would suffice.
undef is a valid MIR token and is never going away.
I also think “never use undef” is too extreme of a policy. If undef is going to exist, it needs test coverage to show it works correctly. There are an endless supply of bugs when these special case values are used. Not only should undef not be avoided, they are actively required to be covered in tests.
Agreed that some tests are needed. The review comment is just a comment and can be ignored by reviewers.
It’s just that without some nudging, the situation is getting out of control, with the number of tests using undef doubling every couple of years. And most uses are not justified.
That said, people have been mostly ignoring bugs that only reproduce with undef inputs. Myself included because I don’t want to waste too much effort on something that will get fixed eventually by itself.
@nlopes We’ve accumulated a lot of test files with a “XXXX-inseltpoison.ll” sibling - usually these files are similar, but some have diverged…
What’s the best thing to do with these? If they have the same tests should we just delete the XXXX variant (and optionally remove the -inseltpoison.ll postfix from the other)?
Vectors are the most advanced part of LLVM with respect to poison AFAICT.
I would say that most tests are redundant (because the vector is fully initialized and thus the initial vector doesn’t matter) and can be removed.
But we shouldn’t remove the tests that use non-fully initialized vectors. I was just crawling through the list of bugs and there’s at least one bug because of a partially-initialized undef vector. This needs to be fixed to match only poison vectors:
; Transforms/VectorCombine/X86/load.ll
define <4 x float> @load_f32_insert_v4f32(ptr dereferenceable(16) align(16) %p) {
%s = load float, ptr dereferenceable(16) align(16) %p, align 16
%r = insertelement <4 x float> undef, float %s, i32 0
ret <4 x float> %r
}
=>
define <4 x float> @load_f32_insert_v4f32(ptr dereferenceable(16) align(16) %p) {
%#1 = load <4 x float>, ptr dereferenceable(16) align(16) %p, align 16
%r = shufflevector <4 x float> %#1, <4 x float> poison, 0, 4294967295, 4294967295, 4294967295
ret <4 x float> %r
}
Transformation doesn't verify! (unsound)
ERROR: Target is more poisonous than source
Source value: < poison, #x00000000 (+0.0), #x00000000 (+0.0), #x00000000 (+0.0) >
Target value: < poison, poison, poison, poison >
I think we have migrated all vector initializing code to use poison, so these kinds of bugs are low risk, but it’s still a bug.
The comments can be ignored, but if the signal to noise ratio is too low, people will just end up ignoring the comment, which is the opposite of what we want. It seems like there are a couple small changes (ignoring undef in MIR for example) that could probably greatly improve the signal to noise ratio.
It needs to be a bit smarter than just files. It should have some understanding of what the output is, and only identify undef as it appears as an IR value (e.g. <valid-type-name> undef in the common case). It’s still complaining on .ll files that check output MIR containing undefs.
Plus, MIR does embed some IR which could still trigger the warning.
I also think it is warning on diffs that are removing undefs, so it’s impossible to make it happy by removing undef uses
