[Diffusion] rG825235c140e7: Revert "[Sema] Use the canonical type in function isVector"

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak added a comment.

In rG825235c140e7747f686bd7902cb0f9af77590841#909794 <https://reviews.llvm.org/rG825235c140e7747f686bd7902cb0f9af77590841#909794>, @SjoerdMeijer wrote:

> > I'm still not sure why __fp16, which is a storage-only type, is used for the element type of float16x4_t if we want to avoid promotion to a float vector type.
>
> About the types: `__fp16` this is the older, storage-only type. So, just historically, I think this element type was used to describe the neon vectors, which translates to vectors with elements of i16 or f16, which all seems fine.
>
> > Was there a discussion on llvm-dev or phabricator?
>
> I don't think this is necessary, because this is how it's been from let's say the beginning.
>
> > Can we change the element type of the vector to _Float16?
>
> I don't see a benefit of doing this, and making this perhaps intrusive change.
>
> > It seems that the original plan discussed on llvm-dev (http://lists.llvm.org/pipermail/cfe-dev/2017-May/053768.html) was to introduce _Float16 and use it instead of __fp16 for ARMv8.2-A, but the subsequent patches that were committed are making NeonEmitter.cpp emit typedefs of vectors that have __fp16 as the element type.
>
> Then `_Float16` came along, which is a proper half type (not a storage only type), is defined in a C11 extension, and should be used for the ARMv8.2-A native half instructions; the semantics of `__fp16` and `_Float16` are different, and as a result using `__fp16` for the v8.2 half FP instructions is not an option, so it is not a matter of using `_Float16` instead of `__fp16`.


OK, I see. Looking at the history, the half precision vector types defined in arm_neon.h (e.g., `float16x4_t`) were introduced long before `_Float16` or ARMv8.2-A.

> As I said, your problem wasn't clear to me, so let's look at your example:
> 
>> For example, in vneg_f16, __p0 is promoted to a vector of float before it's negated.
>> 
>> __ai float16x4_t vneg_f16(float16x4_t __p0) {
>> 
>>   float16x4_t __ret;
>>    __ret = -__p0;
>>    return __ret;
>> 
>> }
> 
> when I compile this and emit llvm ir I get this:
> 
>   define dso_local <2 x i32> @vneg_f16(<2 x i32> %__p0.coerce) local_unnamed_addr #0 {
>   entry:
>     %0 = bitcast <2 x i32> %__p0.coerce to <4 x half>
>     %fneg = fneg fast <4 x half> %0
>     %1 = bitcast <4 x half> %fneg to <2 x i32>
>     ret <2 x i32> %1
>   }
>    
> 
> so far so good I think: this shows a fneg on vector of halfs.

Yes, I think this is what we want to see. We don't want to treat `__p0` as a strorage-only type and promote it to a vector of floats before negating it.

> Where and when do you see problems? Can you describe this, and also provide your compile commands?

The problem is that, after I committed a patch (which was reverted later, see https://reviews.llvm.org/rGa6150b48cea00ab31e9335cc73770327acc4cb3a) to fix a bug in Sema, clang started to promote  `__p0` to a vector of floats because the element type of `float16x4_t` is `__fp16`.

  typedef __fp16 float16_t;
  typedef __attribute__((neon_vector_type(4))) float16_t float16x4_t;

The bug I'm trying to fix causes clang to generate different IR for `test0` and `test1` in the code below even though the only difference is that the element type of `half_vector1` is a typedef of `__fp16`. In `test1`, the two vectors are added without being promoted to vectors of floats first.

  typedef __fp16 FP16;
  typedef __fp16 half_vector0 __attribute__ ((vector_size (8)));
  typedef FP16 half_vector1 __attribute__ ((vector_size (8)));
  
  // 'a' and 'b' are promoted to vectors of floats before the addition.
  half_vector0 test0(half_vector0 a, half_vector0 b) {
    return a + b;
  }
  
  // Currently 'a' and 'b' aren't promoted to vectors of floats before the addition.
  half_vector1 test1(half_vector1 a, half_vector1 b) {
    return a + b;p
  }

After I fixed the bug, clang started to generate the same IR for `test0` and `test1`, but this had the unwanted side-effect of promoting `__p0` in `vneg_f16` to a vector of float as I explained before.

Just to be sure I'm understanding the semantics of the types defined in arm_neon.h, we don't want to treat `float16x4_t` as a storage-only type like `half_vector0` in the code above and promote it to vectors of floats in the front-end, right? If that is the case, it seems to me that `__fp16` isn't the right type as the element type, since that implies the vector of halves has to be promoted to vector of floats. But I also realize that changing the typedef of `float16x4_t` and using `_Float16` as the element type long after the typedef was introduced can break code that is currently working.

I think I'll leave the typedefs unchanged and just avoid promoting vectors of halves in Sema if they are neon vectors.


BRANCHES
  master, release/10.x

Users:
  ahatanak (Author)

https://reviews.llvm.org/rG825235c140e7