[PATCH] D71799: [Attributor] AAUndefinedBehavior: Check for branches on undef value.

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis marked an inline comment as done.
baziotis added a comment.

> Note that there's somewhat relevant prior art,
>  e.g. llvm/lib/Transforms/Instrumentation/PoisonChecking.cpp (D64215 <https://reviews.llvm.org/D64215>)
>  and https://github.com/AliveToolkit/alive2.
>  Would be great to not have this much duplication, but a single all-powerful one, but not sure it's possible (yet?)

Thanks for the reference.
I guess you mean for things like this: https://github.com/AliveToolkit/alive2/blob/master/tests/unit/undef.opt, i.e. propagating
the undef.
For the PoisonChecking that you referenced, from a quick glance I could not see how it relates :/ Could you be more specific?
It seems that poison checking adds runtime checks.



================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2052-2055
+      if (SimplifiedV.hasValue() && isa<UndefValue>(SimplifiedV.getValue()))
+        UBInsts.insert(&I);
+      else
+        NoUBInsts.insert(&I);
----------------
Note that here it's possibly wrong and I forgot to comment yesterday. I didn't know exactly how to do it but here's the problem.
- If it has a value and it is not undef, then it's not UB -> OK
- If it has a value and it's undef, then it is UB -> OK
But...
- If it doesn't have a value, we consider it not UB. Well, I'm not familiar with the internals of `AAValueSimplify`, but looking comments around, there were some like "No value _yet_". Which means that right now we may not have a value but we could in the future.
And that value may be undef.
This is no problem for this patch as it tries to handle cases where undef is caught in (hasValue && isa<Undef>). But eventually, `AAValueSimplify` could uncover things for us here and we may lose them because we put the instruction to `NoUBInsts`.


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

https://reviews.llvm.org/D71799