[PATCH] D153469: [dataflow] Replace most BoolValue subclasses with references to Formula (and AtomicBoolValue => Atom and BoolValue => Formula where appropriate)

Fri Jun 23 00:17:20 PDT 2023

mboehme added a comment.

Just some top-level comments to begin with.

In D153469#4439770 <https://reviews.llvm.org/D153469#4439770>, @sammccall wrote:

> Here we start to see benefits: Value becomes less reliant on inheritance, flow conditions are no longer Values that can uselessly bear properties, atomic variables are numbered in order of creation, we'll soon be able to have distinct BoolValues with the same formula (but different properties).

I like where this is going. Specifically, I like that we will soon no longer be relying in `BoolValue` object identity and will be able to have different `BoolValue`s with the same formula. I'm working on a similar change for `StructValue`, so I'm well aware of how problematic it is to share the same `Value` but then modify properties on it -- it seems like a potential source of really puzzling bugs.

> I think I could separate the Value/Arena changes from the ones to use Atom/Formula in DAContext if you think it's useful (the drawback is some risk of ending up in merge hell...)

I don't think that's necessary -- this patch is large in terms of numbers of lines changed, but when you eliminate the mechanical changes, there's not too much going on.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153469