[PATCH] D91338: [X86] Zero-extend pointers to i64 for x86_64

[Harald van Dijk via Phabricator via llvm-commits]

hvdijk added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:3073
         Val = lowerRegToMasks(Val, VA.getValVT(), VA.getLocVT(), dl, DAG);
       } else
         Val = DAG.getNode(ISD::TRUNCATE, dl, VA.getValVT(), Val);
----------------
pengfei wrote:
> Is some risk here? It is used to truncate i1 only before.
If `LocVT` and `ValVT` are different, it is the responsibility of this function to take a `LocVT` DAG node and convert it to a `ValVT` DAG node, and LLVM will assert if it fails to do so. If `isExtInLoc()` returns true, `LocVT` and `ValVT` will be different, and there is no other code to handle that conversion, so any non-bit-vector extensions of return values would already be broken and would fail that assert. I think the fact that it did not cause problems before is simply because it never came up before for any type other than bit vectors.


================
Comment at: llvm/test/CodeGen/X86/musttail-varargs.ll:350
 ; LINUX-X32-NEXT:    movl 4(%edi), %r11d
+; LINUX-X32-NEXT:    movl %edi, %edi
 ; LINUX-X32-NEXT:    jmpq *%r11 # TAILCALL
----------------
pengfei wrote:
> I have some doubt on the implementation. Is caller always needs zeroext for the pointer even if it knows the upper bits are zero? Form line 309, the callers seems know the upper bits are zero.
This is a general missing optimization in LLVM that affects targets other than X86 as well, you are correct that this `movl %edi, %edi` is not needed if the high bits of `%rdi` are already guaranteed to be zero. LLVM can lose that information. I have not looked at this case in detail, but I have previously seen this be a problem where a node is a copy of a function parameter: since it is not the function parameter, merely a copy of its value, the fact that it is a zero-extended i32 value is not available. Since the generated code is correct, just suboptimal, I did not think it is necessary to fix that at the same time.


================
Comment at: llvm/test/CodeGen/X86/x32-function_pointer-2.ll:17
 ; CHECK: mov{{l|q}}	%{{e|r}}si, %{{e|r}}[[REG:.*]]{{d?}}
-; CHECK: callq	*%r[[REG]]
+; CHECK: callq	*%r
   tail call void %foo(i8* %h) nounwind
----------------
pengfei wrote:
> This file should not be affected, right?
This file is affected because `callq` can take either the copy of `*%rsi` like it did before, or `*%rsi` directly since it knows it is already zero-extended.


================
Comment at: llvm/test/CodeGen/X86/x86-64-sret-return.ll:13
 ; X32ABI-LABEL: bar:
-; X32ABI: movl %edi, %eax
+; X32ABI: mov{{l|q}} %{{r|e}}di, %{{r|e}}ax
 
----------------
pengfei wrote:
> I saw other tests all use 64-bit instructions. In which case we may use 32-bit instruction?
When we need to copy one 64-bit register to another 64-bit register, we can use the 64-bit move instructions to preserve the high bits, or the 32-bit move instructions to zero out the high bits. When the high bits are known to be zero, the 64-bit move instructions and the 32-bit move instructions have the same effect, since zeroing out the high bits when they are already zero does nothing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91338