This is an archive of the discontinued LLVM Phabricator instance.

[PredicateInfo] Add a common method to interpret predicate as cmp constraint
ClosedPublic

Authored by nikic on Jul 12 2020, 1:25 AM.

Details

Summary

Both users of predicteinfo (NewGVN and SCCP) are interested in getting a cmp constraint on the predicated value. They currently implement separate logic for this. This patch adds a common method for this in PredicateWithCondition (it would be nice to drop the PredicateBase/PredicateWithCondition split ... I saw you had a patch for that).

This enables a missing bit of PredicateInfo handling in SCCP: Now the predicate on the condition itself is also used. For switches it means we know that the switched-on value is the same as the case value. For assumes/branches we know that the condition is true or false.

Diff Detail

Event Timeline

nikic created this revision.Jul 12 2020, 1:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2020, 1:25 AM
nikic updated this revision to Diff 277290.Jul 12 2020, 8:38 AM

Relax assertion. RenamedOp may not be accurate due to https://reviews.llvm.org/D78133#2145094, so don't assert this for now.

fhahn accepted this revision.Jul 15 2020, 10:38 AM

LGTM, thanks!

(it would be nice to drop the PredicateBase/PredicateWithCondition split ... I saw you had a patch for that).

Yes, I have to dig it up again...

llvm/include/llvm/Transforms/Utils/PredicateInfo.h
106

How about returning Optional<std::pair<CmpInst::Predicate &, Value *>>? That would seem more explicit to me , but its mostly personal preference I think :)

This revision is now accepted and ready to land.Jul 15 2020, 10:38 AM
nikic updated this revision to Diff 278873.Jul 17 2020, 11:50 AM

Return Optional<PredicateConstraint>.

What do you think about this variant? Something I have in mind here as a possible followup is to change the way we represent predicates away from storing the raw condition, which has renaming issues we only partially mitigated with RenamedOp, towards storing the PredicateConstraint introduced here directly. This makes sure that we cannot go out of sync between the PredicateInfo construction logic and the retrieval of the condition. (For now all conditions have the form of "cmp", but if needed this could be made polymorphic in the future.)

fhahn added a comment.Jul 19 2020, 5:17 AM

Return Optional<PredicateConstraint>.

What do you think about this variant? Something I have in mind here as a possible followup is to change the way we represent predicates away from storing the raw condition, which has renaming issues we only partially mitigated with RenamedOp, towards storing the PredicateConstraint introduced here directly. This makes sure that we cannot go out of sync between the PredicateInfo construction logic and the retrieval of the condition. (For now all conditions have the form of "cmp", but if needed this could be made polymorphic in the future.)

Thanks, that looks good.

Just referring to a condition in the IR makes things more flexible in theory I think, e.g. if the OtherOp part of the condition is modified after PredicateInfo construction. The current users (NewGVN, SCCP) only do modifications after all analysis is finished, so that would not matter for them. If there are clients in the future that really need this, it's probably fine to think about it then.

This revision was automatically updated to reflect the committed changes.