This is an archive of the discontinued LLVM Phabricator instance.

[X86] Add HasAVX512 patterns for SS SSE intrinsics.
AbandonedPublic

Authored by ab on Apr 29 2015, 7:48 PM.

Details

Reviewers
anemet
delena
Summary

These would currently fail to select.

Diff Detail

Event Timeline

ab updated this revision to Diff 24674.Apr 29 2015, 7:48 PM
ab retitled this revision from to [X86] Add HasAVX512 patterns for SS SSE intrinsics..
ab updated this object.
ab edited the test plan for this revision. (Show Details)
ab added reviewers: delena, anemet.
ab set the repository for this revision to rL LLVM.
ab added a subscriber: Unknown Object (MLST).
ab updated this revision to Diff 24682.Apr 29 2015, 10:52 PM
ab updated this object.
ab removed rL LLVM as the repository for this revision.
  • Add S[SD]Zrr->rm folding table entries
  • Cleanup patterns as rm variants aren't needed
delena added inline comments.Apr 30 2015, 12:10 AM
lib/Target/X86/X86InstrAVX512.td
3365

All AVX-512 intrinsics, even FP scalar, come with masks and with rounding mode.
I don't understand where these intrinsics _sse_avx512_ come from.
Due to very complex form of these inrinsics, we add them to X86IntrinsicsInfo.h

ab added inline comments.Apr 30 2015, 7:08 AM
lib/Target/X86/X86InstrAVX512.td
3365

These patterns are just for the SSE1/SSE2 intrinsics (_mm_add_ss, int_x86_sse_add_ss, ...; see tests), which will fail to select when used on AVX512, because the X86InstrSSE.td SI defs are all predicated on "UseAVX" rather than "HaveAVX".

I went with patterns because for _ss, IntrinsicsInfo would turn something like _mm_add_ss to a v4f32 fadd and later addps. We could add an IntrinsicType for _ss _2OP, or maybe even, an additional field in the intrinsic data tables saying that even though an intrinsic has vector type, it's just a scalar op?

delena edited edge metadata.May 7 2015, 12:33 PM

All problems of sse intrinsics should be resolved in X86InstrSSE.td. Please change UseAVX to HasAVX for these intrinsics. I don't think that we should add all SSE intrinsics to AVX512 now.

I don't think that we should add all SSE intrinsics to AVX512 now.

I kind of disagree.
When AVX512 is available we have additional registers we can use for those. I believe we should do the same thing as we do for thumb, i.e., choose the most permissive instruction encoding then shorten the encoding if possible (like the Thumb2SizeReduction pass).

For the long term we may want to find a better solution for such case (same instruction available with different encoding based on extension) to avoid the explosion in size of the description.

What do you think?

Cheers,
-Quentin

ab added a comment.May 7 2015, 3:57 PM

I don't think that we should add all SSE intrinsics to AVX512 now.

I kind of disagree.
When AVX512 is available we have additional registers we can use for those. I believe we should do the same thing as we do for thumb, i.e., choose the most permissive instruction encoding then shorten the encoding if possible (like the Thumb2SizeReduction pass).

I agree, this seems like the way to go; see http://llvm.org/bugs/show_bug.cgi?id=23376.

For the long term we may want to find a better solution for such case (same instruction available with different encoding based on extension) to avoid the explosion in size of the description.

I also agree. In fact I think there are two orthogonal issues here:

  • like I proposed earlier, we should use IntrinsicsInfo for scalar intrinsics. I filed https://llvm.org/bugs/show_bug.cgi?id=23449
  • in lots of cases, the main difference between SSE/AVX/AVX512 instructions is the encoding. Due to the way the backend evolved, they're mostly apart, with lots of duplications (now it's even worse because AVX512 is in a completely different file). There's room for refactoring, I think, but that's a big change.

I can repurpose this review to implement the first one, if people agree? Also, if the second makes sense, I'll file a PR as well.

-Ahmed

What do you think?

Cheers,
-Quentin

Hi,

We encountered with another problem - some sse intrinsics don't work on AVX-512 mode at all, due to UseAVX prefix.
I'm going to fix this, just to let these intrinsics be mapped to AVX instructions.

But I agree, as a long term solution, all these intrinsics should be mapped to AVX-512 instructions. And it is not only scalar FP. SKX covers all AVX and AVX2 instructions with EVEX version.

I'm against writing all this manually, it will be the last option. I'll think how all this mapping may be auto-generated.

  • Elena
ab abandoned this revision.May 21 2015, 5:17 PM

Alright then, for now let's say this is superseded by r237903. Closing.