[PATCH] D82730: [SimplifyCFG] Merge identical basic blocks (WIP)

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D82730#2123286 <https://reviews.llvm.org/D82730#2123286>, @AndrewLitteken wrote:

> We're using a similar method for how to do the Instruction equivalence checking by using `isSameOperationAs` for most of the checking, but is there a reason you are not using `isIdenticalTo` when checking that instructions are equivalent?  It looks like it is doing much of the same thing, including checking if operands and phi nodes are the same.


`isIdenticalTo` isn't being used here, because the operand equality check does not use identity. In particular, for blocks like

  %x1 = add 1, 2
  %y1 = add %x1, 3
  
  %x2 = add 1, 2
  %y2 = add %x2, 3

We need to determine that the blocks are the same, but `%x1` and `%x2` are only equal after remapping. That's what the `ValuesEqual` predicate checks, and how this differs from `isIdenticalTo`.

In D82730#2123353 <https://reviews.llvm.org/D82730#2123353>, @jfb wrote:

> I like this, but it's going to run into the same issues as MergeFuncs. Can you sync with @hiraditya and @vish99 to solve both block comparison and function comparison using the same methodology? The we can turn both of them on by default. Or in other words, how does this not have exactly the same issues MergeFuncs has? The principal one is that the IR comparator goes out of sync with IR definition, and we get miscompiles because of this.


The issue with MergeFuncs is that it does not use simple equality. First, it wants to merge code that is working on different, but bitwise equivalent, types. Second, IIRC it needs an ordering, not just an equality comparison. For that reason it reimplements the comparison logic from scratch. This transform is based on `isSameOperationAs()`, which is a standard instruction comparison operation used by a number of other transforms (including SimplifyCFG).

> Block hashing seems useful. mergeFuncs also hashes IR, but for the whole function. I wonder if hashing blocks should be an analysis pass, which we invalidate when we change IR, and which both this pass as well as MergeFuncs consumes (i.e. try to de-dup functions, then blocks, and use the block hash to create a hash for entire functions).

Possibly, but I suspect it's not worthwhile to treat it as an analysis pass. The hash calculation is not particularly expensive as these things go, and would be invalidated by pretty much any transform.


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

https://reviews.llvm.org/D82730