Closed Bug 1075601 Opened 5 years ago Closed 5 years ago

GVN: visitDefinition leader can replace an instruction by a discarded one.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(1 file)

No description provided.
Comment on attachment 8498222 [details] [diff] [review]
GVN: Overwrite leader definition if it got discarded.

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

::: js/src/jit/ValueNumbering.cpp
@@ +642,5 @@
>          // Look for a match.
>          VisibleValues::AddPtr p = values_.findLeaderForAdd(def);
>          if (p) {
>              MDefinition *rep = *p;
> +            if (!rep->isDiscarded() && rep->block()->dominates(def->block())) {

In theory, GVN removes instructions from the map when they get discarded (the forget method), so it doesn't expect to see them here. Are you seeing a case where this doesn't happen? If we're failing to forget an instruction when discarding it, we may wish to fix that instead.

On the other hand, seeing this patch makes me wonder whether we should just switch to doing everything this way: checking isDiscarded() here and not ever bothering to remove values from the map when discarding them. It'd reduce the total number of hash table lookups, though the map would hold a higher number of useless elements. Thoughts?
(In reply to Dan Gohman [:sunfish] from comment #2)
> Comment on attachment 8498222 [details] [diff] [review]
> GVN: Overwrite leader definition if it got discarded.
> 
> Review of attachment 8498222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/ValueNumbering.cpp
> @@ +642,5 @@
> >          // Look for a match.
> >          VisibleValues::AddPtr p = values_.findLeaderForAdd(def);
> >          if (p) {
> >              MDefinition *rep = *p;
> > +            if (!rep->isDiscarded() && rep->block()->dominates(def->block())) {
> 
> In theory, GVN removes instructions from the map when they get discarded
> (the forget method), so it doesn't expect to see them here. Are you seeing a
> case where this doesn't happen? If we're failing to forget an instruction
> when discarding it, we may wish to fix that instead.

We discard a constant which might be added by Scalar Replacement.  And we use the discarded constant as a leader after in the next visitBlock.

> On the other hand, seeing this patch makes me wonder whether we should just
> switch to doing everything this way: checking isDiscarded() here and not
> ever bothering to remove values from the map when discarding them. It'd
> reduce the total number of hash table lookups, though the map would hold a
> higher number of useless elements. Thoughts?

I thought that removing elements from the hashtable might be costly, and just replacing it when we see a new instance would be better.
Comment on attachment 8498222 [details] [diff] [review]
GVN: Overwrite leader definition if it got discarded.

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

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> (In reply to Dan Gohman [:sunfish] from comment #2)
> > Comment on attachment 8498222 [details] [diff] [review]
> > GVN: Overwrite leader definition if it got discarded.
> > 
> > Review of attachment 8498222 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: js/src/jit/ValueNumbering.cpp
> > @@ +642,5 @@
> > >          // Look for a match.
> > >          VisibleValues::AddPtr p = values_.findLeaderForAdd(def);
> > >          if (p) {
> > >              MDefinition *rep = *p;
> > > +            if (!rep->isDiscarded() && rep->block()->dominates(def->block())) {
> > 
> > In theory, GVN removes instructions from the map when they get discarded
> > (the forget method), so it doesn't expect to see them here. Are you seeing a
> > case where this doesn't happen? If we're failing to forget an instruction
> > when discarding it, we may wish to fix that instead.
> 
> We discard a constant which might be added by Scalar Replacement.  And we
> use the discarded constant as a leader after in the next visitBlock.

How are constants added by Scalar Replacement different from other constants?

> > On the other hand, seeing this patch makes me wonder whether we should just
> > switch to doing everything this way: checking isDiscarded() here and not
> > ever bothering to remove values from the map when discarding them. It'd
> > reduce the total number of hash table lookups, though the map would hold a
> > higher number of useless elements. Thoughts?
> 
> I thought that removing elements from the hashtable might be costly, and
> just replacing it when we see a new instance would be better.

This sounds reasonable to me.
Attachment #8498222 - Flags: review?(sunfish) → review+
(In reply to Dan Gohman [:sunfish] from comment #4)
> Comment on attachment 8498222 [details] [diff] [review]
> GVN: Overwrite leader definition if it got discarded.
> 
> Review of attachment 8498222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > (In reply to Dan Gohman [:sunfish] from comment #2)
> > > Comment on attachment 8498222 [details] [diff] [review]
> > > GVN: Overwrite leader definition if it got discarded.
> > > 
> > > Review of attachment 8498222 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: js/src/jit/ValueNumbering.cpp
> > > @@ +642,5 @@
> > > >          // Look for a match.
> > > >          VisibleValues::AddPtr p = values_.findLeaderForAdd(def);
> > > >          if (p) {
> > > >              MDefinition *rep = *p;
> > > > +            if (!rep->isDiscarded() && rep->block()->dominates(def->block())) {
> > > 
> > > In theory, GVN removes instructions from the map when they get discarded
> > > (the forget method), so it doesn't expect to see them here. Are you seeing a
> > > case where this doesn't happen? If we're failing to forget an instruction
> > > when discarding it, we may wish to fix that instead.
> > 
> > We discard a constant which might be added by Scalar Replacement.  And we
> > use the discarded constant as a leader after in the next visitBlock.
> 
> How are constants added by Scalar Replacement different from other constants?

They are not supposed to, which is what surprised me when I noticed this was in GVN.
Basically they are added at the time of the allocation to emulate the undefined value of the content.
https://hg.mozilla.org/mozilla-central/rev/418296d64769
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.