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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
