This is an archive of the discontinued LLVM Phabricator instance.

[X86] Restore the pavg intrinsics.
ClosedPublic

Authored by craig.topper on Apr 14 2019, 6:12 PM.

Details

Summary

The pattern we replaced these with may be too hard to match as demonstrated by
PR41496 and PR41316.

This patch proposes to restore the intrinsics and then we can start focusing
on the optimizing the intrinsics.

I've mostly reverted the original patch that removed them. Though I modified
the avx512 intrinsics to not have masking built in.

Event Timeline

craig.topper created this revision.Apr 14 2019, 6:12 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2019, 6:12 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Add the clang changes

Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2019, 7:39 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nikic added a subscriber: nikic.Apr 15 2019, 12:13 AM
RKSimon accepted this revision.Apr 15 2019, 4:39 AM

LGTM - there's too many different optimizations and canonicalizations that can occur on such a pattern to be able to match all of the permutations.

We can probably add some InstCombine optimizations (e.g. where the avg can't overflow etc.) and keep the existing DAG matching.

This revision is now accepted and ready to land.Apr 15 2019, 4:39 AM
This revision was automatically updated to reflect the committed changes.

Though I modified the avx512 intrinsics to not have masking built in.

Do we need autoupgrade support from the old avx512 intrinsics to the new avx512 intrinsics?

Though I modified the avx512 intrinsics to not have masking built in.

Do we need autoupgrade support from the old avx512 intrinsics to the new avx512 intrinsics?

Yes, and the code at 1548 in AutoUpgrade.cpp does that. There are existing tests in avx512bw-intrinsics-upgrade.ll and avx512bwvl-intrinsics-upgrade.ll that were previously testing upgrade from old intrinsics to pure IR that now just test old intrinsics to new intrinsics. The codegen is the same so they weren't affected by the patch.

Okay, thanks.