[PATCH] D143425: Reland "[X86][ABI] Don't preserve return regs for preserve_all/preserve_most CCs""

[Phoebe Wang via Phabricator via llvm-commits]

pengfei added a comment.

In D143425#4111499 <https://reviews.llvm.org/D143425#4111499>, @AntonBikineev wrote:

> In D143425#4110057 <https://reviews.llvm.org/D143425#4110057>, @pengfei wrote:
>
>> In D143425#4109985 <https://reviews.llvm.org/D143425#4109985>, @AntonBikineev wrote:
>>
>>> In D143425#4109955 <https://reviews.llvm.org/D143425#4109955>, @pengfei wrote:
>>>
>>>>> This covers the broken scenario.
>>>>
>>>> Can you explain it in more detail? I checked the test locally but found no difference in codegen with or without the new change.
>>>
>>> Did you include the original (broken) CL in the baseline? The CL was reverted, so you'd have to apply it to see the codegen diff. But the diff is basically this:
>>
>> No, I compared between commenting out `if (ShouldDisableArgRegs) {` and not, and didn't see any difference. Or I misunderstood something?
>
> Indeed, the tests didn't cover this case. Thanks! I added new tests dynamic-regmask-preserve-*.ll to check exactly the preserved RegMask on the caller side. Now the difference between the broken commit is much clearer on the caller side. For example, commenting out `if (ShouldDisableArgRegs) {` would remove `rdi`, `rsi`, `rdx`, `rcx`, `r8` and their subregisters from the `CustomRegMask` in the call:
>
>   CALL64pcrel32 @callee1, CustomRegMask($bh,$bl,$bp,$bph,$bpl,$bx,$ch,$cl,$cx,$dh,$di,$dih,$dil,$dl,$dx,$ebp,$ebx,$ecx,$edi,$edx,$esi,$hbp,$hbx,$hcx,$hdi,$hdx,$hsi,$rbp,$rbx,$rcx,$rdi,$rdx,$rsi,$si,$sih,$sil,$r8,$r9,$r10,$r12,$r13,$r14,$r15,$r8b,$r9b,$r10b,$r12b,$r13b,$r14b,$r15b,$r8bh,$r9bh,$r10bh,$r12bh,$r13bh,$r14bh,$r15bh,$r8d,$r9d,$r10d,$r12d,$r13d,$r14d,$r15d,$r8w,$r9w,$r10w,$r12w,$r13w,$r14w,$r15w,$r8wh,$r9wh,$r10wh,$r12wh,$r13wh,$r14wh,$r15wh), implicit $rsp, implicit $ssp, implicit $rdi, implicit $rsi, implicit $rdx, implicit $rcx, implicit $r8, implicit-def $rsp, implicit-def $ssp, implicit-def $rax
>       ADJCALLSTACKUP64 0, 0, implicit-def dead $rsp, implicit-def dead $eflags, implicit-def dead $ssp, implicit $rsp, implicit $ssp

That makes sense to me, thanks for the test!

> The updated revision also contains a slight functional change: for functions with the `sret` parameter (function returning a struct by value) I still preserve `%rax`. The standard SysV CC requires the `sret` (i.e. `%rdi`) be copied into `%rax`, but for the `preserve_*` CCs this is redundant and is not actually used, since it's always the same as the preserved `%rdi`.

I doubt about it, given it is ABI specified. Are there existing callers use `RAX` to access date rather than `rdi`?



================
Comment at: llvm/test/CodeGen/X86/dynamic-regmask-preserve-most.ll:1
+; RUN: llc -mtriple=x86_64-apple-darwin -stop-after machine-sink %s -o %t.mir
+; RUN: FileCheck %s < %t.mir
----------------
Why choose `machine-sink` as the check point?


================
Comment at: llvm/test/CodeGen/X86/dynamic-regmask-preserve-most.ll:3
+; RUN: FileCheck %s < %t.mir
+; RUN: llc %t.mir -mtriple=x86_64-apple-darwin -run-pass machine-sink
+
----------------
What's this RUN checking for?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143425