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

Mon Jun 26 00:44:24 PDT 2023

StephenFan added a comment.

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? :)

> 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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153717