[PATCH] D124435: [X86] Always extend the integer parameters in callee

[Reid Kleckner via Phabricator via cfe-commits]

rnk added inline comments.


================
Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:120
   bool SRetAfterThis : 1;   // isIndirect()
   bool InReg : 1;           // isDirect() || isExtend() || isIndirect()
   bool CanBeFlattened: 1;   // isDirect()
----------------
Instead of introducing a new ABIInfo::Kind value, should this be a boolean flag that is part of ABIInfo::Extend? That seems like it would be simpler.


================
Comment at: clang/test/CodeGen/X86/integer_argument_passing.c:2
+// RUN: %clang_cc1 -O2 -triple -x86_64-linux-gnu %s -emit-llvm -o - | FileCheck %s --check-prefixes=EXTEND,CHECK
+// RUN: %clang_cc1 -O2 -triple -i386-linux-gnu %s -emit-llvm -o - | FileCheck %s --check-prefixes=EXTEND,CHECK
+// RUN: %clang_cc1 -O2 -triple -i386-pc-win32 %s -emit-llvm -o - | FileCheck %s --check-prefixes=EXTEND,CHECK
----------------
LiuChen3 wrote:
> pengfei wrote:
> > pengfei wrote:
> > > LiuChen3 wrote:
> > > > pengfei wrote:
> > > > > Maybe we can remove the tests for i386 given it's only for 64 bits ABI?
> > > > According to the meaning of `ConservativeExtend`, I think the 32bit ABI needs to be modified as well:
> > > > https://godbolt.org/z/W1Ma1T3f3
> > > > The dump of currently clang-cl:
> > > > ```
> > > > _square:
> > > >         movb    4(%esp), %al
> > > >         mulb    %al
> > > >         mulb    8(%esp)
> > > >         retl
> > > > 
> > > >         .def    _baz;
> > > >         .scl    2;
> > > >         .type   32;
> > > >         .endef
> > > >         .section        .text,"xr",one_only,_baz
> > > >         .globl  _baz
> > > >         .p2align        4, 0x90
> > > > _baz:
> > > >         movswl  4(%esp), %eax
> > > >         pushl   %eax
> > > >         calll   _bar
> > > >         addl    $4, %esp
> > > >         retl
> > > > ```
> > > > Of course with this patch the behavior of clang-cl is still different from cl.exe, but I think it fits the meaning of `ConservativeExtend`.
> > > My point was, i386 is passing arguments by stack. The extensions don't make sense under the circumstances. That's what I understood the comments in above test 2007-06-18-SextAttrAggregate.c
> > Oh, seems I misunderstood it. The stack still needs extensions since it's aligned to 4 bytes. But from the above output, the clang-cl is wrong, because it extends on caller which MSVC extends on callee. So back the another question, we should change for Windows too, right?
> I just change the behavior of win32. Windows 64 will always do the extensions in callee. I didn't support `ConservativeExtend` for win64. The dump IR of currently clang-cl is:
> ```
> define dso_local i8 @square(i8 noundef %a, i8 noundef %b) local_unnamed_addr #0 {
> ...
> }
> 
>   %call = tail call i32 @bar(i16 noundef %conv) #3
> ...
> }
> 
> define dso_local i32 @baz(i32 noundef %num) local_unnamed_addr #1 {
> ...
>   %call = tail call i32 @bar(i16 noundef %conv) #3
> ...
> }
> ```
> I think maybe we don't need to do the extension both in caller and callee for WIN64?  
I agree, I think there is only one use of getExtend in the win64 argument code, and it is for bool / i1:
https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/TargetInfo.cpp#L4359

Basically, on Windows, we never extend. That seems to correspond to the behavior we observed in MSVC.

Regarding Windows x86_32, I personally think your behavior change is correct. We shouldn't rely on MSVC to extend its arguments for us. In any case, it's conservatively correct, and hopefully nobody cares about minor efficiency issues for x86_32.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124435