[x86] Don't assume sign-extension of arguments smaller than 32-bits.

[emilio]

Summary

Even though llvm currently will sign-extend in the caller, this is not
guaranteed by the ABI, and thus the callee cannot assume it.

This fixes PR44228 and PR12207.

I still have a few tests to update, but I want to sanity-check this is
reasonable before doing so.

[emilio]

Could I get some feedback on this? Does this look reasonable?

[xbolva00]

llvm/test/CodeGen/X86/x86-64-arg.ll

1

Why deleted? You can update it: Replace this line with:

; RUN: llc < %s -mtriple=x86_64-unknown-unknown | FileCheck %s

and run:
utils/update_llc_test_checks.py llvm/test/CodeGen/X86/x86-64-arg.ll

[emilio]

llvm/test/CodeGen/X86/x86-64-arg.ll

1

This was testing exactly for the wrong assumption, so it seemed like it was not worth keeping. But I'm happy to update it if you prefer.

[emilio]

Oh, and thanks for the utils/update_llc_test_checks.py tip, I was updating them manually and it was driving me crazy :)

[craig.topper]

Shouldn’t we instead stop the sext and zext attributes from being added by the frontend?

[emilio]

update tests

[emilio]

In D71178#1774433, @craig.topper wrote:

Shouldn’t we instead stop the sext and zext attributes from being added by the frontend?

Heh, yeah, I was just going to comment on that, here's my WIP comment:

While I looked through more tests.I think this patch, even though it produces the expected behavior, is wrong... In particular, it seems clang is generating ir with the zeroext / signext annotation, and _that_ is the thing to fix, not this, which just disables the optimization the annotation enables.
(I think. My llvm-foo is not the strongest)

So yeah, I think you're right. I'll look into where does clang generate this. Thanks!

[emilio]

So I started doing that, and D72742 looked different enough that I thought I would submit it separately. Do you think that's on the right track? Or have I missed something? Thanks.

[lebedev.ri]

This seems backwards.
Who added those signext/zeroext attrs to the function arguments?
Presumably they just shouldn't be added if that is not what ABI states.

[emilio]

In D71178#3424386, @lebedev.ri wrote:

This seems backwards.
Who added those signext/zeroext attrs to the function arguments?
Presumably they just shouldn't be added if that is not what ABI states.

Yes, see D72742 for the right approach.

[lebedev.ri]

In D71178#3424445, @emilio wrote:

In D71178#3424386, @lebedev.ri wrote:

This seems backwards.
Who added those signext/zeroext attrs to the function arguments?
Presumably they just shouldn't be added if that is not what ABI states.

Yes, see D72742 for the right approach.

Yep, that seems more like it.

[lebedev.ri]

This review may be stuck/dead, consider abandoning if no longer relevant.
Removing myself as reviewer in attempt to clean dashboard.

llvm/lib/Target/X86/X86ISelLowering.cpp

if (VA.isRegLoc()) {
           RC = &X86::VK64RegClass;
         else
           llvm_unreachable("Unknown argument type!");
 
         unsigned Reg = MF.addLiveIn(VA.getLocReg(), RC);
         ArgValue = DAG.getCopyFromReg(Chain, dl, Reg, RegVT);
       }
 
-      // If this is an 8 or 16-bit value, it is really passed promoted to 32
-      // bits.  Insert an assert[sz]ext to capture this, then truncate to the
-      // right size.
-      if (VA.getLocInfo() == CCValAssign::SExt)
-        ArgValue = DAG.getNode(ISD::AssertSext, dl, RegVT, ArgValue,
-                               DAG.getValueType(VA.getValVT()));
-      else if (VA.getLocInfo() == CCValAssign::ZExt)
-        ArgValue = DAG.getNode(ISD::AssertZext, dl, RegVT, ArgValue,
-                               DAG.getValueType(VA.getValVT()));
-      else if (VA.getLocInfo() == CCValAssign::BCvt)
+      if (VA.getLocInfo() == CCValAssign::BCvt)
         ArgValue = DAG.getBitcast(VA.getValVT(), ArgValue);
 
       if (VA.isExtInLoc()) {
         // Handle MMX values passed in XMM regs.
         if (RegVT.isVector() && VA.getValVT().getScalarType() != MVT::i1)
           ArgValue = DAG.getNode(X86ISD::MOVDQ2Q, dl, VA.getValVT(), ArgValue);
         else if (VA.getValVT().isVector() &&
                  VA.getValVT().getScalarType() == MVT::i1 &&

llvm/test/CodeGen/X86/addr-mode-matcher-2.ll

 ; X86-NEXT:    pushl %eax
 ; X86-NEXT:    calll bar
 ; X86-NEXT:    addl $4, %esp
 ; X86-NEXT:    jmp .LBB0_2
 ;
 ; X64-LABEL: foo:
 ; X64:       # %bb.0:
 ; X64-NEXT:    pushq %rax
-; X64-NEXT:    testl %edi, %edi
+; X64-NEXT:    testb %dil, %dil
 ; X64-NEXT:    je .LBB0_1
 ; X64-NEXT:  # %bb.3:
 ; X64-NEXT:    popq %rax
 ; X64-NEXT:    retq
 ; X64-NEXT:  .LBB0_1: # %.preheader
 ; X64-NEXT:    movl %esi, %eax
 ; X64-NEXT:    .p2align 4, 0x90
 ; X64-NEXT:  .LBB0_2: # =>This Inner Loop Header: Depth=1

llvm/test/CodeGen/X86/avx512-regcall-NoMask.ll

 ; WIN64-NEXT:    vaddsd %xmm1, %xmm0, %xmm0
 ; WIN64-NEXT:    vcvttsd2si %xmm0, %eax
 ; WIN64-NEXT:    retq
 ;
 ; LINUXOSX64-LABEL: test_argRetMixTypes:
 ; LINUXOSX64:       # %bb.0:
 ; LINUXOSX64-NEXT:    vcvtss2sd %xmm1, %xmm1, %xmm1
 ; LINUXOSX64-NEXT:    vaddsd %xmm0, %xmm1, %xmm0
+; LINUXOSX64-NEXT:    movsbl %al, %eax
 ; LINUXOSX64-NEXT:    vcvtsi2sd %eax, %xmm2, %xmm1
 ; LINUXOSX64-NEXT:    vaddsd %xmm1, %xmm0, %xmm0
 ; LINUXOSX64-NEXT:    vcvtsi2sd %ecx, %xmm2, %xmm1
 ; LINUXOSX64-NEXT:    vaddsd %xmm1, %xmm0, %xmm0
 ; LINUXOSX64-NEXT:    vcvtsi2sd %rdx, %xmm2, %xmm1
 ; LINUXOSX64-NEXT:    vaddsd %xmm1, %xmm0, %xmm0
-; LINUXOSX64-NEXT:    vcvtsi2sd %edi, %xmm2, %xmm1
+; LINUXOSX64-NEXT:    movswl %di, %eax
+; LINUXOSX64-NEXT:    vcvtsi2sd %eax, %xmm2, %xmm1
 ; LINUXOSX64-NEXT:    vaddsd %xmm1, %xmm0, %xmm0
 ; LINUXOSX64-NEXT:    vcvtsi2sdl (%rsi), %xmm2, %xmm1
 ; LINUXOSX64-NEXT:    vaddsd %xmm1, %xmm0, %xmm0
 ; LINUXOSX64-NEXT:    vcvttsd2si %xmm0, %eax
 ; LINUXOSX64-NEXT:    retq
   %8 = fpext float %1 to double
   %9 = fadd double %8, %0
   %10 = sitofp i8 %2 to double

llvm/test/CodeGen/X86/mask-negated-bool.ll

; CHECK-NEXT:    retq
   %neg = sub i32 0, %ext
   %and = and i32 %neg, 1
   ret i32 %and
 }
 
 define i32 @mask_negated_zext_bool2(i1 zeroext %x) {
 ; CHECK-LABEL: mask_negated_zext_bool2:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    movl %edi, %eax
+; CHECK-NEXT:    movzbl %dil, %eax
 ; CHECK-NEXT:    retq
   %ext = zext i1 %x to i32
   %neg = sub i32 0, %ext
   %and = and i32 %neg, 1
   ret i32 %and
 }
 
 define <4 x i32> @mask_negated_zext_bool_vec(<4 x i1> %x) {
; CHECK-NEXT:    retq
   %neg = sub i32 0, %ext
   %and = and i32 %neg, 1
   ret i32 %and
 }
 
 define i32 @mask_negated_sext_bool2(i1 zeroext %x) {
 ; CHECK-LABEL: mask_negated_sext_bool2:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    movl %edi, %eax
+; CHECK-NEXT:    movzbl %dil, %eax
 ; CHECK-NEXT:    retq
   %ext = sext i1 %x to i32
   %neg = sub i32 0, %ext
   %and = and i32 %neg, 1
   ret i32 %and
 }
 
 define <4 x i32> @mask_negated_sext_bool_vec(<4 x i1> %x) {

llvm/test/CodeGen/X86/x86-64-arg.ll

-; RUN: llc < %s | grep "movl  %edi, %eax"
-; The input value is already sign extended, don't re-extend it.
-; This testcase corresponds to:
-;   int test(short X) { return (int)X; }
-
-target datalayout = "e-p:64:64"
-target triple = "x86_64-apple-darwin8"
-
-
-define i32 @test(i16 signext  %X) {
-entry:
-        %tmp12 = sext i16 %X to i32             ; <i32> [#uses=1]
-        ret i32 %tmp12
-}
-