[PATCH] D84792: [InstSimplify/NewGVN] Add option to control the use of undef.

Wed Jul 29 11:06:56 PDT 2020

fhahn added a comment.

In D84792#2182195 <https://reviews.llvm.org/D84792#2182195>, @nikic wrote:

>> Is the goal here that it's legal to replace the output value of SimplifyInstruction with the input value, in addition to the normal replacement of the input value with the output value? Or do we specifically care about literal UndefValue constants somehow, as opposed to values which are undef? If we had a PoisonValue constant, would we also need to exclude it?
>
> I think the only thing we need to prevent is that InstSimplify assumes a specific value for (or any kind of constraint on) an undef value. We can still allow other refinement, and I don't think poison values are problematic in this context either, because they don't have a consistency requirement (we don't have to pick one particular value for the poison -- and generally can't).

Yeah as mentioned earlier, assuming different values for undef constants is the biggest issue in practice. Thinking about it again, requiring replacement in both directions would be too limiting due to undef (e.g. we cannot replace a constant with an instruction, unless we know none of the operands are poison).

> Hm, this is a tricky case. I agree that doing this fold is not right, but it also seems hard to really enforce, because of how many value comparisons there are (and not all of them are problematic). I think it may be better to ignore this particular case until we have evidence that it can cause issues in practice (the cases where this could occur are a lot more narrow, because at the very least it involves two undef values from distinct sources).

Yes I think we would have to accept that `CanUseUndef` is 'best-effort' and incomplete. Not great, but I don't think there's any practical way to provide a complete implementation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84792