This is an archive of the discontinued LLVM Phabricator instance.

[SimpleLoopUnswitch] Unswitch AND/OR conditions of selects
ClosedPublic

Authored by caojoshua on May 29 2023, 9:41 PM.

Details

Summary

If a select's condition is a AND/OR, we can unswitch invariant operands.
This patch uses existing logic from unswitching AND/OR's for branch
conditions.

This patch fixes the Cost computation for unswitching selects to have
the cost of the entire loop, since unswitching selects do not remove
branches. This is required for this patch because otherwise, there are
cases where unswitching selects of AND/OR is beating out unswitching of
branches.

This patch also prevents unswitching of logical AND/OR selects. This
should instead be done by unswitching of AND/OR branch conditions.

Diff Detail

Event Timeline

caojoshua created this revision.May 29 2023, 9:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2023, 9:41 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
caojoshua published this revision for review.May 29 2023, 9:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 29 2023, 9:55 PM
nikic added inline comments.Jun 13 2023, 6:18 AM
llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
2875–2878

I think we should also add a && !SI->getType()->isIntegerTy(1) check here to ensure this does not unswitch part of a logical and/or, as these should be handled by other code.

3322

Hm, why do guards need to be handled the same ways as selects? Don't we effectively save the code dominated by the guard in one of the loops?

caojoshua edited the summary of this revision. (Show Details)
  • don't change how we handle cost for guards
  • don't unswitch logical and/or selects
caojoshua marked 2 inline comments as done.Jun 14 2023, 12:16 AM
caojoshua added inline comments.
llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
2875–2878

Yes, I agree. End result in default O3 pipeline is the same, but we have more in pass simplifications if we use and/or branch unswitches.

3322

Yes, that's a mistake. Only doing this for selects now.

nikic accepted this revision.Jun 14 2023, 12:26 AM

LGTM

This revision is now accepted and ready to land.Jun 14 2023, 12:26 AM
This revision was landed with ongoing or failed builds.Jun 14 2023, 12:53 AM
This revision was automatically updated to reflect the committed changes.
caojoshua marked 2 inline comments as done.