This is an archive of the discontinued LLVM Phabricator instance.

[ControlHeightReduction] Freeze condition when converting select to branch
ClosedPublic

Authored by nikic on May 11 2022, 9:19 AM.

Details

Summary

While select conditions can be poison, branch on poison is immediate UB. As such, we need to freeze the condition when converting a select into a branch.

Diff Detail

Event Timeline

nikic created this revision.May 11 2022, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 9:19 AM
nikic requested review of this revision.May 11 2022, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2022, 9:19 AM
nikic added inline comments.May 11 2022, 9:20 AM
llvm/test/Transforms/PGOProfile/chr.ll
1293

This is the only non-trivial change. Apparently it restores the result to what it was before https://github.com/llvm/llvm-project/commit/1bf8f9e228546bd54ef9739aa808b71b97ea6051, so this is probably fine.

fhahn added a subscriber: spatel.May 11 2022, 9:42 AM
fhahn added inline comments.
llvm/test/Transforms/PGOProfile/chr.ll
1293

It would be interesting to know what simplification we are missing and why. Looks like @spatel landed 1bf8f9e228546bd54ef9739aa808b71b97ea6051,, perhaps he has an idea?

1306

Do you think the results would be better if we would freeze J0/I0?

aqjune added inline comments.May 11 2022, 9:15 PM
llvm/test/Transforms/PGOProfile/chr.ll
1182

It seems this freeze is introduced even if the source code (entry) had a conditional branch.
Is it because CHR first converts the conditional branch into select and then convert it back into a conditional branch?

nikic added inline comments.May 12 2022, 2:12 AM
llvm/test/Transforms/PGOProfile/chr.ll
1182

%0 is used in select conditions further down, that's where the freeze comes from. It then gets hoisted up by InstCombine so that everything uses a common variable.

We can exclude that %0 is poison because we're branching on a condition involving it first, but we cannot exclude that it has undef bits based on that.

1293

Hm, now that I look closer, my initial impression here was incorrect. We're missing this fold: https://alive2.llvm.org/ce/z/WrXbGk

spatel added inline comments.May 12 2022, 5:29 AM
llvm/test/Transforms/PGOProfile/chr.ll
1293

Generalized:
https://alive2.llvm.org/ce/z/8wPmWx

I think instcombine gets that without the and interference, so there's some range optimization that might be generalized?

In any case, the diff from the earlier patch was a happy accident, so a regression here doesn't have to hold this patch up.

nikic added inline comments.May 12 2022, 6:51 AM
llvm/test/Transforms/PGOProfile/chr.ll
1293

I think the most general form of this is https://alive2.llvm.org/ce/z/NccNBe, that is (a & b) | c -> c if b implies c. We should be able to do something based on isImpliedCondition() here.

nikic added inline comments.May 13 2022, 3:33 AM
llvm/test/Transforms/PGOProfile/chr.ll
1293

https://reviews.llvm.org/D125530 should fix this regression.

nikic updated this revision to Diff 429220.May 13 2022, 6:13 AM

Rebase to fix regression in test_chr_14.

nikic updated this revision to Diff 429224.May 13 2022, 6:35 AM

Rebase to retain the negateICmpIfUsedByBranchOrSelectOnly() optimization even if freeze is used. This shouldn't matter much end-to-end, but it reduces test diff by avoiding variable naming changes.

fhahn accepted this revision.May 16 2022, 12:38 AM

LGTM, thanks!

This revision is now accepted and ready to land.May 16 2022, 12:38 AM
This revision was landed with ongoing or failed builds.May 16 2022, 1:37 AM
This revision was automatically updated to reflect the committed changes.