1039836 - GVN: include the dependency() in the hash instead

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: gvn-hash-dependency.patch

In bug 845068, when a hash lookup finds a congruent instruction with a different dependency(), it overwrites the old hash table entry on the theory that the old value will no longer be useful. This is a mistake. For example, in this CFG:

   load
   /  \
call  load
load

We may visit the load in the left child block and discover that it has a different dependency than the load in the top block, but we still want to keep the load in the top block available for when we visit the right block.

The attached patch reverts 2b6ec15fea1b and adds code to MDefinition::valueHash to include the dependency() field in the value hash. With this change, the old values are left in the hash table, but they get spread out among many buckets instead of all being in one.

Comment 1

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8457654 [details] [diff] [review]
gvn-hash-dependency.patch

Review of attachment 8457654 [details] [diff] [review]:
-----------------------------------------------------------------

I agree this is the way to go, but I do not think it matters as of today due to the way the dependencies are filled in our alias analysis.  Sadly, in your example, our alias analysis will mention that the load on the right depends on the call from the left. (due to RPO)

if the left-right blocks are swapped, then the RPO will visit the left block first and we should be able to alias the loads.

We need to improve our alias analysis such as we can have better dependencies.  I intend to do that as soon as I am done with the scalar replacement of arrays, as the alias analysis should reuse the same kind of traversal/transformation as the scalar replacement.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/291b0b70cabb