[PATCH] D28503: Documentation for the newly added x86 intrinsics.

[Katya Romanova via Phabricator via cfe-commits]

kromanova added inline comments.


================
Comment at: emmintrin.h:1607
+///
+/// This intrinsic corresponds to the <c> VMOVSD / MOVSD </c> instruction.
+///
----------------
RKSimon wrote:
> kromanova wrote:
> > kromanova wrote:
> > > kromanova wrote:
> > > > probinson wrote:
> > > > > should this be VMOVQ/MOVQ instead?
> > > > Probably yes. Let me know if you have a different opinion.
> > > >  
> > > > If I use this intrinsic by itself, clang generates VMOVSD instruction. 
> > > > It happens because the default domain is chooses to generate smaller instruction code. 
> > > > I got confused because I couldn't find Intel's documentation about _mm_loadu_si64, so I just wrote a test like the one below and looked what instructions got generated.
> > > > 
> > > > ```
> > > > __m128i foo22 (void const * __a)
> > > > {
> > > >   return _mm_loadu_si64 (__a);
> > > > }
> > > > ```
> > > > 
> > > > However, if I change the test and use an intrisic to add 2 64-bit integers after the load intrinsics, I can see that VMOVQ instruction gets generated.
> > > > 
> > > > ```
> > > > __m128d foo44 (double const * __a)
> > > > {
> > > >   __m128i first  = _mm_loadu_si64 (__a);
> > > >   __m128i second = _mm_loadu_si64 (__a);
> > > >   return _mm_add_epi64(first, second);
> > > > 
> > > > }
> > > > ```
> > > > 
> > > > So, as you see clang could generate either VMOVSD/MOVSD or VMOVSQ/MOVSQ. I think it makes sense to change the documentation as Paul suggested:
> > > > 
> > > > /// This intrinsic corresponds to the VMOVSQ/MOVSQ.
> > > > 
> > > > Or, alternatively, we could list all the instructions that correspond to this intrinsics:
> > > > 
> > > > /// This intrinsic corresponds to the VMOVSQ/MOVSQ/VMOVSD/MOVSD.
> > > > 
> > > >           
> > > It will be interesting to hear Asaf Badoug opinion, since he added this intrisic. He probably has access to Intel's documentation for this intrinsic too (which I wasn't able to find online).
> > There is a similar situation for one intrisic just a few lines above, namely _mm_loadu_pd. It could generate either VMOVUPD / MOVUPD or VMOVUPS/MOVUPS instructions. 
> > I have actually asked Simon question about it offline just a couple of days ago. 
> > 
> > I decided to kept referring to VMOVUPD / MOVUPD as a corresponding instruction for _mm_loadu_pd. However, if we end up doing things differently for _mm_loadu_si64, we need to do a similar change to _mm_loadu_pd (and probably to some other intrinsics).
> It should be VMOVQ/MOVQ (note NOT VMOVSQ/MOVSQ!). Whatever the domain fixup code does to it, that was the original intent of the code and matches what other compilers says it will (probably) be.
Yep, sorry, inaccurate editing after copy and paste. Thank you for noticing.
I agree should say VMOVQ/MOVQ (similar to what is done for _mm_loadu_pd that we discussed a few days ago).

I will do this change and reload the review shortly.


Repository:
  rL LLVM

https://reviews.llvm.org/D28503