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

[ZarkoCA]

Summary

This patch contains only the FE changes previously found in https://reviews.llvm.org/D76130.
Currently all PPC32-bit targets take the PPC32SVRABI path, this patch branches within that path to consume the va_list emitted for AIX and emit the correct VAArg.

[sfertile]

clang/lib/Basic/Targets/PPC.h

373

Minor nit: target --> targets.

clang/lib/CodeGen/TargetInfo.cpp

4175

This name and comment is misleading, the class is used for both SVR4 and Darwin, and after this patch AIX. We need to fix the name comment to reflect that.

[hubert.reinterpretcast]

clang/lib/CodeGen/TargetInfo.cpp

4232

No need for two spaces. Add comma after "Currently".

clang/test/CodeGen/aix-vararg.c

4

Can we use built-in types and functions in place of a header inclusion?

[jasonliu]

clang/test/CodeGen/aix-vararg.c

10

As part of a "va_..." family, do we also want to test va_copy?

[ZarkoCA]

clang/lib/CodeGen/TargetInfo.cpp

4175

Does this wording of the comment work?

4232

Thanks, I've found it hard to shake the habits my Grade 8 typing teacher taught me.

clang/test/CodeGen/aix-vararg.c

4

I'm worried about legal/copyright issues with using contents from AIX system headers to LLVM testcases. But I can do that for sure once I understand what I am allowed to do.

10

Good suggestion, added.

[ZarkoCA]

Changed comments per suggestions.
Added va_copy in test case.

[hubert.reinterpretcast]

clang/lib/CodeGen/TargetInfo.cpp

4176

I would suggest using an "Oxford comma" before the "and". Also, colon instead of comma before "used".

clang/test/CodeGen/aix-vararg.c

4

I just mean:
__builtin_va_list
__builtin_va_start
__builtin_va_copy
__builtin_va_arg
__builtin_va_end

[sfertile]

clang/lib/CodeGen/TargetInfo.cpp

4175

Missed updating the class name. Suggested: PPC32_SVR4_ABIInfo --> PowerPC32ABIInfo.

[ZarkoCA]

clang/lib/CodeGen/TargetInfo.cpp

4175

Sorry, missed updating the class name the first time.

clang/test/CodeGen/aix-vararg.c

4

Thanks, I wasn't clear. Done as that now.

[ZarkoCA]

Renamed PPC32_SVR4ABIInfo class to PPC32ABIInfo.
Fixed test case to use builtins.
Changed comment.

[jasonliu]

clang/lib/CodeGen/TargetInfo.cpp

4204

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?

[sfertile]

clang/lib/CodeGen/TargetInfo.cpp

4204

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.

4231

Make sure to address the formatting here.

[jasonliu]

clang/lib/CodeGen/TargetInfo.cpp

4204

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.

[ZarkoCA]

Created PPCAIX32TargetCodeGenInfo class so that initDwarfEHRegSizeTable now returns true on AIX and added a test.

Fixed formatting.

[ZarkoCA]

clang/lib/CodeGen/TargetInfo.cpp

4204

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.

[jasonliu]

clang/lib/CodeGen/TargetInfo.cpp

9925

Does AIX have soft Float? If not, do we want to always pass in 'false'?

clang/test/CodeGen/aix-vararg.c

5

Any reason we don't use __builtin_va... any more?

[ZarkoCA]

Fixed test cases to use builtins again, set no soft float abi for AIX.

[ZarkoCA]

clang/lib/CodeGen/TargetInfo.cpp

9925

Thanks, missed changing this. I set it to hard.

clang/test/CodeGen/aix-vararg.c

5

My mistake, I somehow included the old version of the test in the diff.

[jasonliu]

clang/lib/CodeGen/TargetInfo.cpp

9925

I don't think CodeGenOpts.FloatABI == "hard" is what we want though. Currently it means if CodeGenOpts.FloatABI is really "hard", then it will pass in true for SoftFloatABI to indicate we are soft float ABI.

[ZarkoCA]

clang/lib/CodeGen/TargetInfo.cpp

9925

You're right. I wanted to keep the symmetry but that's not the correct thing to do.

[ZarkoCA]

Set isSoftFloat to return false for AIX.

[ZarkoCA]

Rebase.

[Xiangling_L]

clang/test/CodeGen/aix-vararg.c

16

#0, #1[the last three lines] are redundant, could you clean them up?

[Xiangling_L]

clang/lib/CodeGen/TargetInfo.cpp

4203

I have a question here. AIX32 falls into PPC32 target, so why we don't inherit from PPC32TargetCodeGenInfo instead?

4208

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.

4432

As simple as this function is, does it make sense to move the body of return true into the class definition?

[sfertile]

clang/lib/CodeGen/TargetInfo.cpp

4203

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.

[Xiangling_L]

clang/lib/CodeGen/TargetInfo.cpp

4203

Not adding a new class makes sense to me if we are sure that DwarfEHRegSizeTable will be identical/viable for AIX.

[ZarkoCA]

clang/lib/CodeGen/TargetInfo.cpp

4203

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.

4208

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.

[ZarkoCA]

Rebased on https://reviews.llvm.org/D73290 and this patch now depends on it.
Removed PPCAIX32TargetCodegenClass from previous diff.
Corrected behaviour PPC32TargetCodeGenInfo::initDwarfEHRegSizeTable() for 32BIT AIX, and added the expected output to the test case which has also been renamed to reflect that.

[sfertile]

A couple minor comments, but patch is almost ready otherwise.

clang/lib/CodeGen/TargetInfo.cpp

4227–4228

Pedantic nit: Can we emit a fatal error if -msvr4-struct-return is specified on AIX. Then we can add a lit test which checks for the error, which will change once we verify the options behavior on AIX and enable it.

Note: FWIW both Zarko and I have verified the option is accepted and work as expected by passing in r3/r4, I'm just not sure if any padding is handled the same way on AIX as described below.

4420

Minor nit:

Can we restructure like this:

 // 109: vrsave
// 110: vscr
AssignToArrayRange(Builder, Address, Four8, 109, 110);

  // AIX does not have SPE registers so we don't set 111 - 113.
  if (getABIInfo().getTarget().getTriple().isOSAIX()) 
  return false;

// 111: spe_acc
  // 112: spefscr
  // 113: sfp
  AssignToArrayRange(Builder, Address, Four8, 111, 113);

return false;

clang/test/CodeGen/ppc64-aix32-dwarf.c

1 ↗

(On Diff #256351)

Minor nit: How about naming the test file ppc-dwarf.c

[ZarkoCA]

Addressed comments
-added error for -msvr4-struct-return on AIX and modified appropriate test case
-changed code structure as per comment
-renamed test file

[sfertile]

LGTM.

[jasonliu]

clang/test/CodeGen/ppc32-struct-return.c

53 ↗

(On Diff #256398)

If certain front end option is not supported on certain target, I think it makes more sense to have a standard diagnostic in the driver component, instead of "crash" in the backend.
i.e. What if we specify this option on a Windows machine? Maybe we should pursue the same behavior.

[sfertile]

clang/test/CodeGen/ppc32-struct-return.c

53 ↗

(On Diff #256398)

Its not that we don't intend to support the option on AIX, but that support for the option takes further verification on AIX. Since there is a difference how AIX justifies subregister sized values in its argument passing, we can't just assume that this option will pack the return values the same way on AIX and Linux.

The focus of this patch was originally to enable and verify the proper IR generation of va-arg/va-lis/va-start, however the scope of the patch has kept growing. If there are codegen differences in packing the return register with the svr4-return option enabled it will grow this patch once again. The fatal error lets us limit the scope of this patch, while not silently emiting bad codegen if there is a difference in how gcc initializes the return registers. If during verification we decide we don't want to support the option on AIX, then adopting a standard diagnostic in the driver component becomes the appropriate behavior.

[jasonliu]

clang/test/CodeGen/ppc32-struct-return.c

53 ↗

(On Diff #256398)

It wasn't clear for me from the code that this is not a permanent thing. In that case, does it make sense to leave a TODO here and say we need to re-evaluate this in the future to decide if we want to support it or not on AIX?
On another note, I'm not sure if this is really needed on AIX target though. But I guess we could discuss it down the road.

[jasonliu]

clang/test/CodeGen/ppc32-struct-return.c

53 ↗

(On Diff #256398)

Just to avoid ambiguity, I meant I'm not sure if we really need this *option* on AIX.

[ZarkoCA]

clang/test/CodeGen/ppc32-struct-return.c

53 ↗

(On Diff #256398)

The focus of this patch was originally to enable and verify the proper IR generation of va-arg/va-lis/va-start, however the scope of the patch has kept growing. If there are codegen differences in packing the return register with the svr4-return option enabled it will grow this patch once again. The fatal error lets us limit the scope of this patch, while not silently emiting bad codegen if there is a difference in how gcc initializes the return registers. If during verification we decide we don't want to support the option on AIX, then adopting a standard diagnostic in the driver component becomes the appropriate behavior.

@sfertile basically articulated my reasoning quite well here. I just want to add, if verified to work on AIX, then we can simply remove the error in that one place and have the option working.

GCC on AIX has those options and they work in the same way as described in https://reviews.llvm.org/D73290. I think implementing a compatible GCC option in a fairly straightforward way doesn't hurt us.

I agree with your point made earlier, I will add a TODO to make it clearer that we need to completely verify the behaviour.

[ZarkoCA]

Added a TODO to remove the error for msvr4-struct-return on AIX when we verify it works as expected.

[ZarkoCA]

Rebased patch to include latest changes in trunk. Removed that it depended on https://reviews.llvm.org/D73290 in the summary, as that patch has been landed.

[ZarkoCA]

This work is included in https://reviews.llvm.org/D79035. Abandoning this revision.

clang/lib/Basic/Targets/PPC.h

if (Triple.isOSFreeBSD() || Triple.isOSNetBSD() || Triple.isOSOpenBSD() ||
       LongDoubleFormat = &llvm::APFloat::IEEEdouble();
     }
 
     // PPC32 supports atomics up to 4 bytes.
     MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 32;
   }
 
   BuiltinVaListKind getBuiltinVaListKind() const override {
-    // This is the ELF definition, and is overridden by the Darwin sub-target
+    // This is the ELF definition, and is overridden by the Darwin and AIX
+    // sub-target.
     return TargetInfo::PowerABIBuiltinVaList;
   }
 };
 
 // Note: ABI differences may eventually require us to have a separate
 // TargetInfo for little endian.
 class LLVM_LIBRARY_VISIBILITY PPC64TargetInfo : public PPCTargetInfo {
 public:

clang/lib/CodeGen/TargetInfo.cpp

Address WinX86_64ABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAListAddr,
   return emitVoidPtrVAArg(CGF, VAListAddr, Ty, IsIndirect,
                           CGF.getContext().getTypeInfoInChars(Ty),
                           CharUnits::fromQuantity(8),
                           /*allowHigherAlign*/ false);
 }
 
 // PowerPC-32
 namespace {
 /// PPC32_SVR4_ABIInfo - The 32-bit PowerPC ELF (SVR4) ABI information.
 class PPC32_SVR4_ABIInfo : public DefaultABIInfo {
   bool IsSoftFloatABI;
 
   CharUnits getParamTypeAlignment(QualType Ty) const;
 
 public:
   PPC32_SVR4_ABIInfo(CodeGen::CodeGenTypes &CGT, bool SoftFloatABI)
       : DefaultABIInfo(CGT), IsSoftFloatABI(SoftFloatABI) {}
 
int getDwarfEHStackPointer(CodeGen::CodeGenModule &M) const override {
     // This is recovered from gcc output.
     return 1; // r1 is the dedicated stack pointer
   }
 
   bool initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
                                llvm::Value *Address) const override;
 };
 }
 
 CharUnits PPC32_SVR4_ABIInfo::getParamTypeAlignment(QualType Ty) const {
   // Complex types are passed just like their elements
   if (const ComplexType *CTy = Ty->getAs<ComplexType>())
     Ty = CTy->getElementType();
 
   if (Ty->isVectorType())
     return CharUnits::fromQuantity(getContext().getTypeSize(Ty) == 128 ? 16
                                                                        : 4);
 
   // For single-element float/vector structs, we consider the whole type
   // to have the same alignment requirements as its single element.
   const Type *AlignTy = nullptr;
   if (const Type *EltType = isSingleElementStruct(Ty, getContext())) {
     const BuiltinType *BT = EltType->getAs<BuiltinType>();
     if ((EltType->isVectorType() && getContext().getTypeSize(EltType) == 128) ||
         (BT && BT->isFloatingPoint()))
       AlignTy = EltType;
   }
 
   if (AlignTy)
     return CharUnits::fromQuantity(AlignTy->isVectorType() ? 16 : 4);
   return CharUnits::fromQuantity(4);
 }
 
 // TODO: this implementation is now likely redundant with
 // DefaultABIInfo::EmitVAArg.
 Address PPC32_SVR4_ABIInfo::EmitVAArg(CodeGenFunction &CGF, Address VAList,
                                       QualType Ty) const {
-  if (getTarget().getTriple().isOSDarwin()) {
+  // TODO: Add AIX ABI Info.  Currently we are relying on PPC32_SVR4_ABIInfo to
+  // emit correct VAArg.
+  if (getTarget().getTriple().isOSDarwin() ||
+      getTarget().getTriple().isOSAIX()) {
     auto TI = getContext().getTypeInfoInChars(Ty);
     TI.second = getParamTypeAlignment(Ty);
 
     CharUnits SlotSize = CharUnits::fromQuantity(4);
     return emitVoidPtrVAArg(CGF, VAList, Ty,
                             classifyArgumentType(Ty).isIndirect(), TI, SlotSize,
                             /*AllowHigherAlign=*/true);
   }
PPC32TargetCodeGenInfo::initDwarfEHRegSizeTable(CodeGen::CodeGenFunction &CGF,
   AssignToArrayRange(Builder, Address, Four8, 64, 76);
 
   // 77-108: v0-31, the 16-byte vector registers
   AssignToArrayRange(Builder, Address, Sixteen8, 77, 108);
 
   // 109: vrsave
   // 110: vscr
   // 111: spe_acc
   // 112: spefscr
   // 113: sfp
   AssignToArrayRange(Builder, Address, Four8, 109, 113);
 
   return false;
 }
 
 // PowerPC-64
 
 namespace {
 /// PPC64_SVR4_ABIInfo - The 64-bit PowerPC ELF (SVR4) ABI information.
 class PPC64_SVR4_ABIInfo : public SwiftABIInfo {
 public:
   enum ABIKind {
     ELFv1 = 0,
     ELFv2
   };
 
 private:
   static const unsigned GPRBits = 64;
   ABIKind Kind;
else if (CodeGenOpts.FloatABI == "hard" ||
       Kind = ARMABIInfo::AAPCS_VFP;
 
     return SetCGInfo(new ARMTargetCodeGenInfo(Types, Kind));
   }
 
   case llvm::Triple::ppc:
     return SetCGInfo(
         new PPC32TargetCodeGenInfo(Types, CodeGenOpts.FloatABI == "soft" ||
                                    getTarget().hasFeature("spe")));
   case llvm::Triple::ppc64:
     if (Triple.isOSBinFormatELF()) {
       PPC64_SVR4_ABIInfo::ABIKind Kind = PPC64_SVR4_ABIInfo::ELFv1;
       if (getTarget().getABI() == "elfv2")
         Kind = PPC64_SVR4_ABIInfo::ELFv2;
       bool HasQPX = getTarget().getABI() == "elfv1-qpx";
       bool IsSoftFloat = CodeGenOpts.FloatABI == "soft";
 

clang/test/CodeGen/aix-vararg.c

+// REQUIRES: powerpc-registered-target
+// REQUIRES: asserts
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -emit-llvm -o - %s | FileCheck %s --check-prefix=32BIT
+#include <stdarg.h>
+
+void aix_varg(int a, ...) {
+  va_list arg;
+  va_start(arg, a);
+  va_arg(arg, int);
+  va_end(arg);
+}
+
+// 32BIT:           define void @aix_varg(i32 %a, ...) #0 {
+// 32BIT:           entry:
+// 32BIT-NEXT:        %a.addr = alloca i32, align 4
+// 32BIT-NEXT:        %arg = alloca i8*, align 4
+// 32BIT-NEXT:        store i32 %a, i32* %a.addr, align 4
+// 32BIT-NEXT:        %arg1 = bitcast i8** %arg to i8*
+// 32BIT-NEXT:        call void @llvm.va_start(i8* %arg1)
+// 32BIT-NEXT:        %argp.cur = load i8*, i8** %arg, align 4
+// 32BIT-NEXT:        %argp.next = getelementptr inbounds i8, i8* %argp.cur, i32 4
+// 32BIT-NEXT:        store i8* %argp.next, i8** %arg, align 4
+// 32BIT-NEXT:        %0 = bitcast i8* %argp.cur to i32*
+// 32BIT-NEXT:        %1 = load i32, i32* %0, align 4
+// 32BIT-NEXT:        %arg2 = bitcast i8** %arg to i8*
+// 32BIT-NEXT:        call void @llvm.va_end(i8* %arg2)
+// 32BIT-NEXT:        ret void
+// 32BIT-NEXT:      }
+// 32BIT:            declare void @llvm.va_start(i8*)
+// 32BIT:            declare void @llvm.va_end(i8*)