Page MenuHomePhabricator

[NewGVN] Add ops as dependency if we cannot find a leader for ValueOp.
ClosedPublic

Authored by fhahn on Jan 17 2018, 6:33 AM.

Details

Summary

If those operands change, we might find a leader for ValueOp, which
could enable new phi-of-op creation.

This fixes a case where we missed creating a phi-of-ops node. With
this patch, bootstrapping clang/llvm works with -enable-newgvn, whereas
without it, the "value changed after iteration" assertion is triggered.

Diff Detail

Repository
rL LLVM

Event Timeline

fhahn created this revision.Jan 17 2018, 6:33 AM
dberlin added a comment.EditedJan 17 2018, 8:06 AM

This is definitely not right, unfortunately.

This is already taken care of by code below:

// When these operand changes, it could change whether there is a
// leader for us or not, so we have to add additional users.
if (isa<PHINode>(Op)) {
  Op = Op->DoPHITranslation(PHIBlock, PredBB);
  if (Op != OrigOp && Op != I)
    Deps.insert(Op);
} else if (auto *ValuePHI = RealToTemp.lookup(Op)) {
  if (getBlockForValue(ValuePHI) == PHIBlock)
    Op = ValuePHI->getIncomingValueForBlock(PredBB);
}

By the end of this loop, Deps should contain the set of operands we depend on.

Note that the only values that need to be added are phis, because everything else should be the same as the original IR, and *already* a user. You do not have to represent dependencies that are already represented in the IR

IE
given

1 foo = something
2 b = use foo

2 will already be re-evaluated if foo changes, it's already a user of 1's result.

If foo is a phi we translate, *then* we have to add it to someone's users because it is different than the original IR.

I would suggest starting by checking for the invariants you expect at the end of makePossiblePhiOfOps and understanding where they are not being met.

fhahn added a comment.Jan 17 2018, 8:26 AM

Will do, thanks!

mcrosier added a subscriber: mcrosier.
fhahn updated this revision to Diff 137415.Mar 7 2018, 9:28 AM
fhahn retitled this revision from [NewGVN] Re-evaluate phi of ops if expr operands change. to [NewGVN] Add ops as dependency if we cannot find a leader for ValueOp..
fhahn edited the summary of this revision. (Show Details)

Updated to add additional users for the translated operands of the current ValueOp, if we cannot find a leader for the ValueOp.

If those operands change, we might find a leader for ValueOp, which
could enable new phi-of-op creation.

This fixes a case where we missed creating a phi-of-ops node. With
this patch, bootstrapping clang/llvm works with -enable-newgvn, whereas
without it, the "value changed after iteration" assertion is triggered.

This is based on @dberlin's patch shared for D43865, because it is
slightly clearer what's going on, but it does not really depend on it.

ping. Does this make sense?

Sorry, i'll stare at this again.

dberlin accepted this revision.Apr 18 2018, 3:23 PM

Sorry for the delay. I've mostly convinced myself this isn't wrong, so let's go with it for now.

This revision is now accepted and ready to land.Apr 18 2018, 3:23 PM
fhahn edited the summary of this revision. (Show Details)Apr 19 2018, 6:39 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Apr 20 2018, 9:32 AM

Thank you very much for all the feedback & patience :)