[PATCH] D105271: [PowerPC][ELF]make sure local variable space does not overlap with parameter save area

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

Thanks for the review @jsji. Updated accordingly.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3915
       }
-  }
+  } else if (Flags.getByValSize() >= 8)
+    // For 64-bit ELF v2, passing by value object whose size is no less than 8
----------------
jsji wrote:
> Should we check the ELFV2ABI here?  
> 
> Also type check? 
I added ABI check, I think we don't need the type check, as here the type is byval and it is always a pointer to a structure or a scalar type, we just need to know its size.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3916
+  } else if (Flags.getByValSize() >= 8)
+    // For 64-bit ELF v2, passing by value object whose size is no less than 8
+    // bytes will be copied to parameter save area.
----------------
jsji wrote:
> comment are referring to the opposite? Should we describe the ABI about size >=8?
I think saving the byval parameter (size>=8bytes) into the stack is not required by ABI. This is for compatibility with GCC. See  https://github.com/llvm/llvm-project/blob/21fd8759529707b0f4430ebe8f27a01edc7f655e/llvm/lib/Target/PowerPC/PPCISelLowering.cpp#L6172-L6181.
I think we need to reinvestigate this requirement for byval parameter on Linux. Saving byval parameter in caller's stack frame is not necessary IMO, for example, we don't do this on AIX. And from the latter comments of the above codes, GCC seems also does not require this in its implementation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105271