[PATCH] D109966: [X86][NFC] structure-return simplificiation

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

All three conditions that use this value are trying to determine if the callee is expected to pop the sret pointer off the stack. They have some duplicate subtarget checks that can probably be simplified. They all check these conditions:

- !IsMCU: MCU doesn't seem to pop the sret value ever
- !Is64Bit: In the TCO eligbility check, this is expressed as Is32bit, but is functionally identical
- !IsMSVC: MSVC doesn't do this callee-pop sret thing
- !IsInReg: sret parameters marked with inreg are not popped (reasonable)

So, can I suggest this simplification:

- replace the `IsMCU` parameter with the subtarget
- move all three subtarget checks into these functions. Consider sharing them, maybe a helper like `isSRetCalleePop`, it's basically `Is32bit && !IsMSVC && !IsMCU`.
- make the helper names more precise: `returnHasCalleePopSRet` / `argsHaveCalleePopSRet`



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:3338
 
   const ISD::ArgFlagsTy &Flags = Outs[0].Flags;
+  if (Flags.isSRet() && !Flags.isInReg())
----------------
There's a subtlety here: LLVM doesn't require that sret arguments come first. However, it would be impossible for the callee to pop off the second argument and leave behind the first, so this convention is only ever used when the sret argument appears first. Therefore only the first argument needs to be checked.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:4649-4652
     // If this is a call to a struct-return function, the callee
     // pops the hidden struct pointer, so we have to push it back.
     // This is common for Darwin/X86, Linux & Mingw32 targets.
     // For MSVC Win32 targets, the caller pops the hidden struct pointer.
----------------
This comment should live with the target checks if those get moved.


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

https://reviews.llvm.org/D109966