ZarkoCA marked 8 inline comments as done.
ZarkoCA added inline comments.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4205
+
+class PPCAIX32TargetCodeGenInfo : public TargetCodeGenInfo {
+public:
----------------
Xiangling_L wrote:
> sfertile wrote:
> > Xiangling_L wrote:
> > > I have a question here. AIX32 falls into PPC32 target, so why we don't
> > > inherit from `PPC32TargetCodeGenInfo` instead?
> > Do we need a separate AIX specific class? We are implementing 2 functions,
> > 1 of which is the same implementation as its `PPC32TargetCodeGenInfo`
> > counterpart. If we have access to the triple, we can return true when the
> > OS is AIX in `PPC32TargetCodeGenInfo::initDwarfEHRegSizeTable`. With the
> > implementations being nearly identical (and after enabling
> > DwarfEHRegSizeTable they will be identical) I think we are better to not
> > add a new class if we can avoid it.
> Not adding a new class makes sense to me if we are sure that
> `DwarfEHRegSizeTable` will be identical/viable for AIX.
I had an offline conversation with @sfertile, verified that the existing
initDwarfEHRegSizeTable() function can work for AIX with minor changes.
At first, I thought it may be useuful to create a stub class for for AIX at
least. But I was able to access the AIX triple info through getABIInfo() and we
can keep the one PPC32 class and diverge for AIX within the function when
required.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4210
+
+ int getDwarfEHStackPointer(CodeGen::CodeGenModule &M) const override {
+ return 1; // r1 is the dedicated stack pointer
----------------
Xiangling_L wrote:
> Is `getDwarfEHStackPointer` necessary to be correct for vararg of AIX to
> work[I guess possibly not]? If not, should it fall into Dwarf related patch
> rather than in this one? BTW, if your `PPCAIX32TargetCodeGenInfo` inherits
> from `PPC32TargetCodeGenInfo` instead as I mentioned above, then it would be
> naturally correct.
It's not related to vaarg but the implementation is pretty simple so we might
as well do it since we are here. I also added a testcase with this enabled.
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
