[PATCH] D76360: [PPC][AIX] Emit correct Vaarg for 32BIT-AIX in clang

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA marked 2 inline comments as done.
ZarkoCA added inline comments.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4205
 
-CharUnits PPC32_SVR4_ABIInfo::getParamTypeAlignment(QualType Ty) const {
+CharUnits PowerPC32ABIInfo::getParamTypeAlignment(QualType Ty) const {
   // Complex types are passed just like their elements
----------------
jasonliu wrote:
> sfertile wrote:
> > jasonliu wrote:
> > > So for AIX we are taking PowerPC32ABIInfo right now. And we know EmitVAArg of this target does the right thing on AIX after the change. 
> > > But for other functions, for example, getParamTypeAlignment, initDwarfEHRegSizeTable... Are we sure it's doing the right thing on AIX?
> > > If we are not sure, is there anything we want to do (etc, put a comment on where it gets used or at the function definition)? Or are we fine to just leave it as it is and have a TODO in our head?
> > Looking at the values in `initDwarfEHRegSizeTable` it has the right sizes for all the registers. Even though the OS is different the underlying hardware is the same. I'm not sure it's something that makes sense to support for AIX though, in which case I think its valid to return `true` to indicate its not supported. 
> > 
> > `getParamTypeAlignment` is used only in the context of the alignment for vaarg, we should make sure its correct for AIX since supporting vaarg is the scope of this patch.
> In that case, is it better if we have lit test to actually exercise the path in getParamTypeAlignment to show that we actually confirmed the behavior is correct for AIX? And if it is some path we do not care for now(ComplexType? VectorType?), then we would want TODOs on them to show we need further investigation later. 
>But for other functions, for example, getParamTypeAlignment, initDwarfEHRegSizeTable... Are we sure it's doing the right thing on AIX?

The AIX stdarg headers show that alignment for the va_list is dependent on mode, so 4 on 32 and 8 on 64-bit. getParamAlignment looks like it aligns 4 for non-complex non-struct args. 



>If we are not sure, is there anything we want to do (etc, put a comment on where it gets used or at the function definition)? Or are we fine to just leave it as it is and have a TODO in our head?

We already took this path before this patch and I'm correcting some of the behaviour so it's correct for varargs on AIX. I think that we will need to address this depending on when/if we add an AIXABIInfo class.  I added a TODO on line 4247 for just that so I think that implies we will investigate everything required for the correct implementation of the whatever remains after the vaarg handling.  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76360