[PATCH] D135930: [X86] Add AVX-NE-CONVERT instructions.

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added a comment.

LGTM.



================
Comment at: clang/lib/Headers/avx512vlbf16intrin.h:164
+#define _mm_cvtneps_pbh(A) \
+  ((__m128bh)__builtin_ia32_vcvtneps2bf16128((__v4sf)(A)))
 
----------------
FreddyYe wrote:
> pengfei wrote:
> > FreddyYe wrote:
> > > pengfei wrote:
> > > > pengfei wrote:
> > > > > RKSimon wrote:
> > > > > > Is there no way for __attribute__ to allow different attribute permutations?
> > > > > > 
> > > > > > Also, can we keep the __builtin_ia32_cvtneps2bf16_128 naming convention?
> > > > > > Is there no way for attribute to allow different attribute permutations?
> > > > > 
> > > > > We have discussed this problem with GCC folks. There are two problems here:
> > > > > 1. Unlike builtins, function attributes are more generic. It may introduce a lot of checks between callers and callees. I had a research to limit it to `__always_inline__` functions only. However, Clang handles inlining in middle-end, we don't have such information in the front-end. Besides, we don't know how to merge different permutations if they are inlining to the same function.
> > > > > 2. We don't know how to put the permutations into IR's function attributes. We need to preserve all permutations for inlining reference, but the backend needs a determine feature list rather than selective.
> > > > It's better to use `__builtin_ia32_cvtneps2bf16_128`.
> > > I think __builtin_ia32_vcvtneps2bf16128 is also a "right" name.
> > > 
> > > See __builtin_ia32_vfmaddsubph256, __builtin_ia32_minph256...
> > > 
> > > And I admit naming conventions of clang builtins as well as LLVM IR builtins are confusing right now.
> > The problem here is `16128` is a bit confusing, a `_` breaks it into 2 number.
> > But I'm not insist on it :)
> I did a try but found __builtin_ia32_cvtneps2bf16_256 existed for avx512bf16, and it's used for mask intrinsic lowering currently. What about not change this time? We can do a refine patch later for avx512bf16 builtins since they also have some redundant FE/codegen logics for 256/512 mask intrinsics.
No problem.


================
Comment at: llvm/test/CodeGen/X86/avxneconvert-intrinsics-shared.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -O0 -verify-machineinstrs -mtriple=x86_64-unknown-unknown --show-mc-encoding -mattr=+avxneconvert,+avx512bf16,+avx512vl | FileCheck %s --check-prefix=AVX512BF16-COMMON
+; RUN: llc < %s -O0 -verify-machineinstrs -mtriple=i686-unknown-unknown --show-mc-encoding -mattr=+avxneconvert,+avx512bf16,+avx512vl | FileCheck %s --check-prefix=AVX512BF16-COMMON
----------------
Remove `-O0`


================
Comment at: llvm/test/CodeGen/X86/avxneconvert-intrinsics.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -O0 -verify-machineinstrs -mtriple=x86_64-unknown-unknown --show-mc-encoding -mattr=+avxneconvert | FileCheck %s --check-prefixes=CHECK,X64
+; RUN: llc < %s -O0 -verify-machineinstrs -mtriple=i686-unknown-unknown --show-mc-encoding -mattr=+avxneconvert | FileCheck %s --check-prefixes=CHECK,X86
----------------
ditto.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135930/new/

https://reviews.llvm.org/D135930