This is an archive of the discontinued LLVM Phabricator instance.

[GuardWidening] Refactor to work with the list of checks to widen/hoist
ClosedPublic

Authored by aleksandr.popov on Aug 11 2023, 2:01 AM.

Details

Summary

Currently we hoist conditions from widenable branch which are joined to
the widenable_condition by And operation. E.g if we have
br(WC && (c1 && c2)) we will operate with (c1 && c2) unsplitted.

This patch adds more flexibility to that mechanism by supporting work
with the list of checks parsed from the widenable branch.

On that stage patch doesn't change the logic of checks hoisting. In the
example above we will either hoist both checks [c1, c2] or none of them.
But in the future we would improve that logic analyzing each check
separately.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2023, 2:01 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
aleksandr.popov requested review of this revision.Aug 11 2023, 2:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2023, 2:01 AM
anna added a subscriber: anna.Aug 17 2023, 7:57 AM

do you need review for this? Could you pls state the review stack in the description so it is clear the order of reviews. Thanks.

Hi Anna, more priority https://reviews.llvm.org/D157502 to review. It's the root patch of the stack to land.

aleksandr.popov edited the summary of this revision. (Show Details)
anna added inline comments.Aug 24 2023, 7:46 AM
llvm/lib/Transforms/Scalar/GuardWidening.cpp
127–129
215

Pls update comment.

270

Fix comment.

291

Fix comment pls.

300

Update comment.

aleksandr.popov edited the summary of this revision. (Show Details)

Updated comments

aleksandr.popov marked 5 inline comments as done.Aug 25 2023, 8:16 AM
aleksandr.popov added inline comments.
llvm/lib/Transforms/Scalar/GuardWidening.cpp
215

Thanks!

anna added inline comments.Aug 25 2023, 10:37 AM
llvm/lib/Transforms/Scalar/GuardWidening.cpp
548

Nit: unrelated change.

696

Can we return the result instead of returning a boolean and storing the result in a separate argument?
i.e.

std::optional<Value *> GuardWideningImpl::mergeChecks(SmallVectorImpl<Value *> &ChecksToHoist,
                                    SmallVectorImpl<Value *> &ChecksToWiden,
                                    Instruction *InsertPt)

If this works, pls land this separately before this patch (don't need explicit review) and then rebase on this revision. Makes it easier for review. Thanks.

In places where only the boolean is required, you can just check the optional has value (In isWideningCondProfitable for example).

aleksandr.popov marked an inline comment as done.

Rebased

llvm/lib/Transforms/Scalar/GuardWidening.cpp
696

I've extracted widenCondCommon splitting in a separate patch, PTAL: https://reviews.llvm.org/D159009

anna accepted this revision.Aug 30 2023, 2:07 PM

LGTM.

This revision is now accepted and ready to land.Aug 30 2023, 2:07 PM