[PATCH] D105263: [X86] AVX512FP16 instructions enabling 1/6

[Jameson Nash via Phabricator via llvm-commits]

vtjnash added a comment.
Herald added a subscriber: StephenFan.
Herald added a project: All.

I was tracking back a recent ABI break (also failing now in gcc 12, so maybe this irregularity is intentional), and was concerned that this commit is observed to cause the platform ABI to change depending on the feature flags of the current compilation unit. Prior to this change, f16 was always treated as i16 for the purpose of the calling-convention (e.g. returned in %ax). But after this change, the ABI of the value is now inconsistent between compile units. I made a small change to one of the existing tests to show this. Note how the callq result was in %ax without this mattr flag, and in %xmm0 with this mattr flag added. But the function known as "identity.half" is external, and did not change between those two calls to the llvm.

  diff --git a/llvm/test/CodeGen/X86/half.ll b/llvm/test/CodeGen/X86/half.ll
  index 46179e7d9113..8c1b8c4b76ff 100644
  --- a/llvm/test/CodeGen/X86/half.ll
  +++ b/llvm/test/CodeGen/X86/half.ll
  @@ -5,6 +5,8 @@
   ; RUN:   | FileCheck %s -check-prefixes=CHECK,CHECK-LIBCALL,BWOFF
   ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -mattr=+f16c -fixup-byte-word-insts=1 \
   ; RUN:    | FileCheck %s -check-prefixes=CHECK,BWON,BWON-F16C
  +; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -mattr=+avx512fp16 -fixup-byte-word-insts=0 \
  +; RUN:    | FileCheck %s -check-prefixes=CHECK-CC
   ; RUN: llc < %s -mtriple=i686-unknown-linux-gnu -mattr +sse2 -fixup-byte-word-insts=0  \
   ; RUN:    | FileCheck %s -check-prefixes=CHECK-I686
  
  @@ -163,16 +199,31 @@ define void @test_trunc32(float %in, half* %addr) #0 {
     ret void
   }
   
  +declare half @identity.half(half)
  +
   define void @test_trunc64(double %in, half* %addr) #0 {
   ; CHECK-LABEL: test_trunc64:
   ; CHECK:       # %bb.0:
   ; CHECK-NEXT:    pushq %rbx
   ; CHECK-NEXT:    movq %rdi, %rbx
   ; CHECK-NEXT:    callq __truncdfhf2 at PLT
  +; CHECK-NEXT:    # kill: def $ax killed $ax def $eax
  +; CHECK-NEXT:    movl %eax, %edi
  +; CHECK-NEXT:    callq identity.half at PLT
   ; CHECK-NEXT:    movw %ax, (%rbx)
   ; CHECK-NEXT:    popq %rbx
   ; CHECK-NEXT:    retq
   ;
  +; CHECK-CC-LABEL: test_trunc64:
  +; CHECK-CC:       # %bb.0:
  +; CHECK-CC-NEXT:    pushq %rbx
  +; CHECK-CC-NEXT:    movq %rdi, %rbx
  +; CHECK-CC-NEXT:    vcvtsd2sh %xmm0, %xmm0, %xmm0
  +; CHECK-CC-NEXT:    callq identity.half at PLT
  +; CHECK-CC-NEXT:    vmovsh %xmm0, (%rbx)
  +; CHECK-CC-NEXT:    popq %rbx
  +; CHECK-CC-NEXT:    retq
  +;
   ; CHECK-I686-LABEL: test_trunc64:
   ; CHECK-I686:       # %bb.0:
   ; CHECK-I686-NEXT:    pushl %esi
  @@ -181,12 +232,16 @@ define void @test_trunc64(double %in, half* %addr) #0 {
   ; CHECK-I686-NEXT:    movsd {{.*#+}} xmm0 = mem[0],zero
   ; CHECK-I686-NEXT:    movsd %xmm0, (%esp)
   ; CHECK-I686-NEXT:    calll __truncdfhf2
  +; CHECK-I686-NEXT:    # kill: def $ax killed $ax def $eax
  +; CHECK-I686-NEXT:    movl %eax, (%esp)
  +; CHECK-I686-NEXT:    calll identity.half at PLT
   ; CHECK-I686-NEXT:    movw %ax, (%esi)
   ; CHECK-I686-NEXT:    addl $8, %esp
   ; CHECK-I686-NEXT:    popl %esi
   ; CHECK-I686-NEXT:    retl
     %val16 = fptrunc double %in to half
  -  store half %val16, half* %addr
  +  %val16b = call half @identity.half(half %val16)
  +  store half %val16b, half* %addr
     ret void
   }

Is this intentional? We do already have code to handle the ABI dependency on vector-sizes, and could add this to the list of flags that change the ABI (i.e. we disable it if it will break the ABI), but wanted to confirm first if that was the intent here.

discovered from https://github.com/JuliaLang/julia/issues/44829


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105263