[PATCH] D153717: [SCCP] replace valuestate.isConstant with helper isConstant

Tue Jun 27 01:08:34 PDT 2023

nikic added a comment.

In D153717#4447859 <https://reviews.llvm.org/D153717#4447859>, @StephenFan wrote:

> In D153717#4447064 <https://reviews.llvm.org/D153717#4447064>, @nikic wrote:
>
>> This only makes a difference when the binop returns a poison result, right? I think it's unfortunate that this ends up replacing that with an undef value.
>
> Exactly. Replace a poison value with an undef value is not a good idea, because As LangRef says, PoisonValue is stronger than UndefValue, it enables more optimizations. But actually, I really don't understand that, or in other words, I can't find an example to convince myself. Do you @nikic know that or do you mind telling me the answer? :)

For example `and i32 %x, poison` is `poison`, but `and i32 %x, undef` is not `undef` (it would be folded to `i32 0`).

>> I think it would be good to introduce a new lattice value for poison, which allows us to explicitly represent this and avoid refining poison to undef. Especially in conjunction with D153718 <https://reviews.llvm.org/D153718>, which is kind of weird in we claim no undefs but the result is represented as under.
>
> This is a good idea. Or I also had an idea that we don't need the `undef` enum in `ValueLattice`, because no matter `UndefValue` or `PoisonValue`, they are all constant. And when we visit instructions in SCCPSolver, we should do instructionsimplify for instructions that one of their operands is constant. If we want to judge if a valuelattice is undef, we can just judge like `isa<UndefValue>(ConstVal)`. What's your @nikic opinion?

I think the reason for the separate undef state is to make sure it does not accidentally get treated like a constant. The core problem is that SCCP allows a lattice transition undef -> C (which basically only exists to support phi nodes with undef operands). So if you initially folded some operation using undef as op(undef) = C2 (which makes an implicit choice of undef) and then transition undef -> C, then op(C) = C3 where possibly C2 != C3. This would violate the lattice structure.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153717