[PATCH] D76931: [ValueLattice] Distinguish between constant ranges with/without undef.

Fri Mar 27 12:36:13 PDT 2020

nikic added a comment.

In D76931#1946577 <https://reviews.llvm.org/D76931#1946577>, @efriedma wrote:

> The natural definition of undef extends to branches without any special extensions.  It might make sense to redefine it to have some poison-like characteristics if that would make our life easier; I don't think any frontends would care.  But I think really, you're just trading one set of problems for another.  For example, if you have a switch on "x & -2", right now we can eliminate the "&".  If branching on poison is UB, that requires freezing x.

I didn't really get your example, could you maybe spell it out in more detail?

I believe we already have a simple upper bound on which optimizations we're going to lose if we make this change: MSan already considers branching on undef to be UB, and we already block optimizations that have the potential to introduce branch on undef where it did not exist before if MSan is enabled. From a quick look, there currently seem to be only two places related to branching:

https://github.com/llvm/llvm-project/blob/8896d123154c4606d11f6b6f97c33d5a455cd9ea/llvm/lib/Transforms/Utils/SimplifyCFG.cpp#L3809-L3812
https://github.com/llvm/llvm-project/blob/ee7510dc86656b739881466fddd59253d008139d/llvm/lib/Transforms/Scalar/LoopUnswitch.cpp#L719-L727

So there seem to be just two transforms that are potentially affected (and iirc LoopUnswitch was one of the motivating cases for freezing, so probably that would go away once that is done?)

On the other hand we have any code that is inferring information from predicates. If branch on undef is not UB, I suspect that most of CVP is rendered unsound/useless. Probably parts of SCEV that reason about branch conditions as well. And probably parts of ValueTracking, e.g. we use dominating conditions for non-zero checks. Thankfully at least assumes are not affected.

> Ultimately, I'm think trying to redefine undef at this point would act as a distraction for the poison work.  That seems worse than whatever minor improvement we might get from redefining it.

I'm not sure why this would be a distraction from the poison work. Aligning the UB definitions for undef and poison seems like something that would make that work more straightforward, as we have less different semantics to deal with.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76931