This is an archive of the discontinued LLVM Phabricator instance.

[NewGVN] Track simplification dependencies for phi-of-ops.
ClosedPublic

Authored by fhahn on Apr 6 2021, 1:14 PM.

Details

Summary

If we are using a simplified value, we need to add an extra
dependency this value , because changes to the class of the
simplified value may require us to invalidate any decision based on
that value.

This is done by adding such values as additional users, however the
current code does not excludes temporary instructions.

At the moment, this means that we miss those dependencies for
phi-of-ops, because they are temporary instructions at this point. We
instead need to add the extra dependencies to the root instruction of
the phi-of-ops.

This patch pushes the responsibility of adding extra users to the
callers of createExpression & performSymbolicEvaluation. At those
points, it is clearer which real instruction to pick.

Alternatively we could either pass the 'real' instruction as additional
argument or use another map, but I think the approach in the patch makes
things a bit easier to follow.

Fixes PR35074.

Diff Detail

Event Timeline

fhahn created this revision.Apr 6 2021, 1:14 PM
fhahn requested review of this revision.Apr 6 2021, 1:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2021, 1:14 PM
ruiling added inline comments.Apr 7 2021, 12:22 AM
llvm/lib/Transforms/Scalar/NewGVN.cpp
1874–1900

I think we also need to return the PI->Condition as ExtraDep together with the expression for such cases, this is what I saw in the case PR49873 that we failed to mark the 'real' instruction (i.e. op-of-phi) depends on this predicate compare.

1908

Do we need to check that I is not a temp instruction before making it depends on other instruction? I think the I here may be a temp instruction if this is called from makePossiblePHIOfOps()->FindLeaderForInst()->performSymbolicEvaluation()->performSymbolicCmpEvaluation().

fhahn updated this revision to Diff 337686.Apr 15 2021, 3:20 AM

Update performSymbolicCmpEvaluation to return ExprResult, so ExtraDeps created by the call to createExpression can be handled properly. Also add an assert to ExprResult to make sure ExtraDep is handled to the destructor.

fhahn added inline comments.Apr 15 2021, 3:45 AM
llvm/lib/Transforms/Scalar/NewGVN.cpp
1874–1900

Thanks, I'll give that a try, but I think it's worth to do this as follow-up patch.

1908

That's a good point, thanks! I updated performSymbolicCmpEvaluation to directly return ExprResult as well, so this case should also be handled properly in the caller now.

fhahn updated this revision to Diff 337750.Apr 15 2021, 7:08 AM

Update addExtraUserTo to take AdditionalUsers instead of callback to simplify code.

fhahn added inline comments.Apr 15 2021, 7:14 AM
llvm/lib/Transforms/Scalar/NewGVN.cpp
1874–1900

I updated performSymbolicPredicateInfoEvaluation in D100560 to use the same approach to register additional dependencies for the predicate simplification.

I think it makes sense to add the users at the upper call level where the real instruction is known. Just one comment inline.

llvm/lib/Transforms/Scalar/NewGVN.cpp
692

For consistency with the rest of the GVN APIs, I wound't add this functionality inside ExprResult.
I think an API akin to addAdditionalUsers, which queries the ExprResult for the dependency field and adds the user would maintain this consistency. It can even call addAdditionalUsers after the first two checks; and in the dependent patch, it would also call addPredicateUsers.
What do you think?

fhahn updated this revision to Diff 339784.Apr 22 2021, 2:28 PM

Replace addExtraUsers with an addAdditionalUsers variant that takes ExprResult as argument.

fhahn added inline comments.Apr 22 2021, 2:30 PM
llvm/lib/Transforms/Scalar/NewGVN.cpp
692

Thanks, I think that makes a lot of sense! I updated the patch to use a new addAdditionalUsers variant which takes a ExprResult argument. I'll update the other patches tomorrow, if that looks good.

asbirlea accepted this revision.Apr 22 2021, 3:28 PM

Thank you! Looks good.

A note for the dependent patch, if it will also adds predicate users, perhaps the name addAdditionalUsers need not be overloaded (though addAdditionalAndPredicateUsers is a bit verbose).
If it makes sense to inline the predicate API into the new one, and remove it, I'd be ok with that.

This revision is now accepted and ready to land.Apr 22 2021, 3:28 PM
This revision was landed with ongoing or failed builds.Apr 23 2021, 1:49 AM
This revision was automatically updated to reflect the committed changes.