[PATCH] D41863: [AArch64] Fix incorrect LD1 of 16-bit FP vectors in big endian

[Oliver Stannard via Phabricator via llvm-commits]

olista01 added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:5852
 def : Pat<(v4f16 (bitconvert (v4i16 FPR64:$src))),
-                             (v4f16 (REV64v4i16 FPR64:$src))>;
+                             (v4f16 FPR64:$src)>;
 def : Pat<(v4f16 (bitconvert (v8i8  FPR64:$src))),
----------------
efriedma wrote:
> pbarrio wrote:
> > SjoerdMeijer wrote:
> > > Don't think I understand why this is now a special case. Why is this one different from the other patterns here?
> > Earlier in the file there is an explanation about why we need to insert REVs when we do bitcasts in big endian (~line 5600). At the end, the following comment suggests that identity conversions (e.g. same-size float-to-int conversions) do not need it:
> > 
> > ```
> > 
> >  // Most bitconverts require some sort of conversion. The only exceptions are:
> >  //   a) Identity conversions -  vNfX <-> vNiX
> > ```
> > 
> > It makes sense to me that a type conversion in big endian between vectors with elements of the same size does not need a byte reversal. This reversal should have been done right before, otherwise the original vector (in this case, with type v4f16) would have also been incorrect.
> There should be a testcase specifically for this change, then.  Maybe something like this, to force the conversion:
> 
> ```
> %x = add <4 x i16> %i, 1
> %y = bitcast <4 x i16> %x to <4 x half>
> %z = fpext <4 x half> %y tp <4 x float>
> ```
> 
> Please put this in a separate patch from the AArch64TargetLowering::addTypeForNEON changes.  Also, please move the pattern out of the IsBE predicate.
(discussed in person with Pablo, noting here for the record)

We think some of the other patterns here are also wrong, especially the vNf16<->vNi16 ones, which obviously don't need REV instructions. We should fix all of them at once.


Repository:
  rL LLVM

https://reviews.llvm.org/D41863