This is an archive of the discontinued LLVM Phabricator instance.

Preserve important metadata in JumpThreadingPass::unfoldSelectInstr
AcceptedPublic

Authored by mmendell on Jul 18 2023, 1:02 PM.

Details

Reviewers
nikic
Summary

Useful metadata like llvm.loop can be lost when the new branch instruction is created. Grab that metadata from the old branch.
Add a correct LIT test to check

Diff Detail

Event Timeline

mmendell created this revision.Jul 18 2023, 1:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2023, 1:02 PM
mmendell requested review of this revision.Jul 18 2023, 1:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 18 2023, 1:02 PM
nikic accepted this revision.Jul 18 2023, 2:02 PM

LGTM, but I do wonder whether we aren't working around a different issue here. JumpThreading generally does not perform threading across loop headers (it's behind a default-disabled option). As far as I understand, the select unfolding is just done to enable future threading. In this case, we unfold the select, but then can't thread it because that would require threading across a header.

We're left with what appear to me an unprofitable transform: We now have two loop latches, which is non-canonical. LoopSimplify will have to convert this back into a single latch.

So my thinking is that we should probably prevent select unfolding from happening here in the first place. I see that tryToUnfoldSelectInCurrBB() already guards against this, but tryToUnfoldSelec() does not.

This revision is now accepted and ready to land.Jul 18 2023, 2:02 PM