This is an archive of the discontinued LLVM Phabricator instance.

[GuardWidening] Make sure widened condition operands are available at insertion point
ClosedPublic

Authored by dmakogon on Feb 28 2023, 12:21 AM.

Details

Summary

When inserting a widened condition we handle the following special case:

L >u C0 && L >u C1  ->  L >u max(C0, C1)

Recently insertion point finding algorithm changed.
Previously we would insert the new condition right before the conditional branch where all L operands were available.
Now we may choose the widenable condition intrinsic call as insertion point and it may happen so that the L operands are computed after the call, so we have to make sure that L operands are available where we want to insert it.

Diff Detail

Event Timeline

dmakogon created this revision.Feb 28 2023, 12:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2023, 12:21 AM
dmakogon requested review of this revision.Feb 28 2023, 12:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 28 2023, 12:21 AM
dmakogon edited the summary of this revision. (Show Details)Feb 28 2023, 12:21 AM

I think the naming here is extremely confusing and misleading. Can you pls rename isAvailableAt into something that better corresponds to its true semantics in a follow-up patch?

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

Why is that guaranteed?

dmakogon edited the summary of this revision. (Show Details)Feb 28 2023, 12:30 AM
dmakogon added inline comments.Feb 28 2023, 12:39 AM
llvm/lib/Transforms/Scalar/GuardWidening.cpp
555

The caller must guarantee that Cond0 and Cond1 are available at InsertPt if it's not null.
Generally we can always insert Cond0 && Cond1 there, so availability must be guaranteed.

I think the naming here is extremely confusing and misleading. Can you pls rename isAvailableAt into something that better corresponds to its true semantics in a follow-up patch?

Yeah, agree. Will do this as a follow-up.

dmakogon added inline comments.Feb 28 2023, 12:49 AM
llvm/lib/Transforms/Scalar/GuardWidening.cpp
555

The caller must guarantee that Cond0 and Cond1 are available at InsertPt if it's not null.

... and in this case L is an operand of Cond0 so it must be available too.

mkazantsev accepted this revision.Feb 28 2023, 1:14 AM

Ok, thanks for clarification!

This revision is now accepted and ready to land.Feb 28 2023, 1:14 AM
This revision was landed with ongoing or failed builds.Feb 28 2023, 1:31 AM
This revision was automatically updated to reflect the committed changes.
dmakogon marked an inline comment as done.Feb 28 2023, 1:31 AM