[RFC][Tablegen] Add CCIfSplitFrom and CCPassIndirectBySamePointer Calling Convention Interfaces

[shiva0217]

Summary

Hi,

For the target ABI have following calling convention

"Pass by indirect pointer if the primary type is f128"

If the target didn't support f128 register class, legalizer will split the arguments.
So it's hard to identify the argument is split from f128 by *CallingConv.td,
and we have to obtain custom calling convention handler to deal with.

To pass the split parts by the same indirect pointer would also need extra handler function.
One of the cases is "CC_SystemZ_I128Indirect" in SystemZCallingConv.h.

To simplify the porting effort, I'd like to add the interfaces as follow.

CCIfSplitFrom - If the current argument is split from one of the specified types, apply action A.

CCPassIndirectBySamePointer - Same as CCPassIndirect except pass by the same pointer if the arguments have been split by legalizer.

With the new calling convention interfaces, we could describe

"Pass by indirect pointer if primary type is f128" as

CCIfType<[i64], CCIfSplitFrom<[f128], CCPassIndirectBySamePointer<i64>>>

without writing custom handling function by CCCustom.

The Tablegen result will be

  if (LocVT == MVT::i64) {
        if (ArgFlags.getOrigVt() == MVT::f128) {
	      LocVT = MVT::i64;
	      LocInfo = CCValAssign::Indirect;
	      if (!ArgFlags.isSplit()) {
	             State.addLoc(State.lastLoc());
	             return false;
              }
         }
   }

Any suggestions?

Thanks,
Shiva

[rnk]

This needs tests, and it would surely be simpler if the frontend didn't emit illegal types as arguments. We normally handle this kind of thing in clang, so you would get an LLVM f128* argument.

[shiva0217]

In D38894#897090, @rnk wrote:

This needs tests, and it would surely be simpler if the frontend didn't emit illegal types as arguments. We normally handle this kind of thing in clang, so you would get an LLVM f128* argument.

Hi @rnk, Thanks for the quick reply!
Yes, most of the cases we could handle by clang.
One of the cases couldn't is that the function created by llvm. E.g. softfloat functions which clang will see it as floating operations.
So we still need to write a custom function to deal with.

Could you guide me how to add the test case for the calling convention interfaces?
Should I patch some backend and write MC testcases for the backend or there is better way ?

[shiva0217]

Hi @rnk,

I have updated SystemZCallingConv.td to use new interfaces for i128 type arguments.
SystemZ already have llvm/test/CodeGen/SystemZ/args-09.ll
for testing i128 type arguements passing.

Add SystemZ Owner @uweigand for reviewing

[uweigand]

The SystemZ parts LGTM. I agree that it's much preferable to have this handled in common code than in the back end ...

[rnk]

In D38894#897118, @shiva0217 wrote:

In D38894#897090, @rnk wrote:

This needs tests, and it would surely be simpler if the frontend didn't emit illegal types as arguments. We normally handle this kind of thing in clang, so you would get an LLVM f128* argument.

Hi @rnk, Thanks for the quick reply!
Yes, most of the cases we could handle by clang.
One of the cases couldn't is that the function created by llvm. E.g. softfloat functions which clang will see it as floating operations.
So we still need to write a custom function to deal with.

OK, that sounds like a great test case then. :) Seeing it will help clear up why this is necessary. I'm still not convinced that it is.

Could you guide me how to add the test case for the calling convention interfaces?
Should I patch some backend and write MC testcases for the backend or there is better way ?

Get clang to emit a .ll file that needs this handling and add it to tests/CodeGen/SystemZ.

[uweigand]

Well, the special handling of i128 arguments as such is certainly needed on SystemZ, this is why I added it in the first place in rev. 261325.

This was triggered by a bug report here: https://bugs.llvm.org/show_bug.cgi?id=26559 which in fact has C source code that clang translates to IR that exposes the problem.

Now this patch does not fix any problem in SystemZ in itself, it just moves the required handling from the back end to common code, making life easier for other back ends that need the same handling.

[shiva0217]

Hi @rnk and @uweigand,
The bug report:https://bugs.llvm.org/show_bug.cgi?id=26559 which @uweigand mentioned is the case that softfloat function created by llvm which clang won't handle it.

In the bug report, clang will emit "uitofp i128 %0 to float" floating operation.
LLVM will soften the floating operation to softfloat function call.

So I add the bug report case as args-11.ll as @rnk suggestion.

The patch is for making backend life easier. RISCV32 have the similar ABI and could have benefit with it.

[shiva0217]

This comment was removed by shiva0217.

[shiva0217]

Hi @rnk ,
In the test case, clang doesn't know backend support the floating instruction or not, so clang will emit it as floating operation IR and won't treat it as function call.
LLVM know the backend not support the floating operation, so it would soften to softfloat function call.
It was transferred to function call by llvm, therefore, in this case, clang can't help for transfer the arguments passing to indirect.
Could the test case convinced you the patch could be helpful?
Or should I add/modify something to make the patch more complete/correct?

Thanks,
Shiva

[asb]

include/llvm/Target/TargetCallingConv.td

160

Can you make this comment clearer? i.e. explain that the difference in behaviour is when this is applied to split arguments.

[shiva0217]

Update comments of CCPassIndirectBySamePointer as @asb suggest.
Make it clear that the differences between CCPassIndirectBySamePointer and CCPassIndirect.
CCPassIndirectBySamePointer will assign the same pointer if the arguments have been split by legalizer.

[shiva0217]

PING

[mgrang]

include/llvm/CodeGen/CallingConvLower.h

263

Can you leave out the void from the empty parameter type?

test/CodeGen/SystemZ/args-11.ll

16 ↗

(On Diff #122392)

Check indentation.

[shiva0217]

include/llvm/CodeGen/CallingConvLower.h

263

Remove void from empty parameter type as @mgrang suggest.

test/CodeGen/SystemZ/args-11.ll

16 ↗

(On Diff #122392)

Fix indentation.

[apazos]

include/llvm/Target/TargetCallingConv.td

35

If the current argument is splited -->is split

[shiva0217]

include/llvm/Target/TargetCallingConv.td

35

Fixup comment.

[shiva0217]

I think I accidentally remove one subscriber, sorry about that, add back subscriber.

include/llvm/CodeGen/CallingConvLower.h

 public:
   CCState(CallingConv::ID CC, bool isVarArg, MachineFunction &MF,
           SmallVectorImpl<CCValAssign> &locs, LLVMContext &C);
 
   void addLoc(const CCValAssign &V) {
     Locs.push_back(V);
   }
 
+  CCValAssign lastLoc(void) {
+    return Locs.back();
+  }
+
   LLVMContext &getContext() const { return Context; }
   MachineFunction &getMachineFunction() const { return MF; }
   CallingConv::ID getCallingConv() const { return CallingConv; }
   bool isVarArg() const { return IsVarArg; }
 
   /// getNextStackOffset - Return the next stack offset such that all stack
   /// slots satisfy their alignment requirements.
   unsigned getNextStackOffset() const {

include/llvm/Target/TargetCallingConv.h

private:
     unsigned IsSecArgPass : 1; ///< Second argument
     unsigned ByValAlign : 4;   ///< Log 2 of byval alignment
     unsigned OrigAlign : 5;    ///< Log 2 of original alignment
     unsigned IsInConsecutiveRegsLast : 1;
     unsigned IsInConsecutiveRegs : 1;
     unsigned IsCopyElisionCandidate : 1; ///< Argument copy elision candidate
 
     unsigned ByValSize; ///< Byval struct size
+    EVT OrigVt; ///< Original value type before split
 
   public:
     ArgFlagsTy()
         : IsZExt(0), IsSExt(0), IsInReg(0), IsSRet(0), IsByVal(0), IsNest(0),
           IsReturned(0), IsSplit(0), IsInAlloca(0), IsSplitEnd(0),
           IsSwiftSelf(0), IsSwiftError(0), IsHva(0), IsHvaStart(0),
           IsSecArgPass(0), ByValAlign(0), OrigAlign(0),
           IsInConsecutiveRegsLast(0), IsInConsecutiveRegs(0),
           IsCopyElisionCandidate(0), ByValSize(0) {
-      static_assert(sizeof(*this) == 2 * sizeof(unsigned), "flags are too big");
+      static_assert(sizeof(*this) == 2 * sizeof(unsigned) + sizeof(EVT),
+                    "flags are too big");
     }
 
     bool isZExt() const { return IsZExt; }
     void setZExt() { IsZExt = 1; }
 
     bool isSExt() const { return IsSExt; }
     void setSExt() { IsSExt = 1; }
 
public:
     unsigned getOrigAlign() const { return (1U << OrigAlign) / 2; }
     void setOrigAlign(unsigned A) {
       OrigAlign = Log2_32(A) + 1;
       assert(getOrigAlign() == A && "bitfield overflow");
     }
 
     unsigned getByValSize() const { return ByValSize; }
     void setByValSize(unsigned S) { ByValSize = S; }
+
+    EVT getOrigVt() const { return OrigVt; }
+    void setOrigVt(EVT Vt) { OrigVt = Vt; }
   };
 
   /// InputArg - This struct carries flags and type information about a
   /// single incoming (formal) argument or incoming (from the perspective
   /// of the caller) return value virtual register.
   ///
   struct InputArg {
     ArgFlagsTy Flags;

include/llvm/Target/TargetCallingConv.td

 }
 
 /// CCIfType - If the current argument is one of the specified types, apply
 /// Action A.
 class CCIfType<list<ValueType> vts, CCAction A> : CCPredicateAction<A> {
   list<ValueType> VTs = vts;
 }
 
+/// CCIfSplitFrom - If the current argument is splited from one of the specified
+/// types, apply Action A.
+class CCIfSplitFrom<list<ValueType> vts, CCAction A> : CCPredicateAction<A> {
+  list<ValueType> VTs = vts;
+}
+
 /// CCIf - If the predicate matches, apply A.
 class CCIf<string predicate, CCAction A> : CCPredicateAction<A> {
   string Predicate = predicate;
 }
 
 /// CCIfByVal - If the current argument has ByVal parameter attribute, apply
 /// Action A.
 class CCIfByVal<CCAction A> : CCIf<"ArgFlags.isByVal()", A> {

 }
 
 /// CCPassIndirect - If applied, this stores the value to stack and passes the pointer
 /// as normal argument.
 class CCPassIndirect<ValueType destTy> : CCAction {
   ValueType DestTy = destTy;
 }
 
+/// CCPassIndirectBySamePointer - Just like CCPassIndirect except pass by the
+/// same pointer.
+class CCPassIndirectBySamePointer<ValueType destTy> : CCAction {
+  ValueType DestTy = destTy;
+}
+
 /// CCDelegateTo - This action invokes the specified sub-calling-convention.  It
 /// is successful if the specified CC matches.
 class CCDelegateTo<CallingConv cc> : CCAction {
   CallingConv CC = cc;
 }
 
 /// CallingConv - An instance of this is used to define each calling convention
 /// that the target supports.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

for (unsigned Value = 0, NumValues = ValueVTs.size(); Value != NumValues;
         if (NumParts > 1 && j == 0)
           MyFlags.Flags.setSplit();
         else if (j != 0) {
           MyFlags.Flags.setOrigAlign(1);
           if (j == NumParts - 1)
             MyFlags.Flags.setSplitEnd();
         }
 
+        MyFlags.Flags.setOrigVt(VT);
         CLI.Outs.push_back(MyFlags);
         CLI.OutVals.push_back(Parts[j]);
       }
 
       if (NeedsRegBlock && Value == NumValues - 1)
         CLI.Outs[CLI.Outs.size() - 1].Flags.setInConsecutiveRegsLast();
     }
   }
for (unsigned Value = 0, NumValues = ValueVTs.size();
         if (NumRegs > 1 && i == 0)
           MyFlags.Flags.setSplit();
         // if it isn't first piece, alignment must be 1
         else if (i > 0) {
           MyFlags.Flags.setOrigAlign(1);
           if (i == NumRegs - 1)
             MyFlags.Flags.setSplitEnd();
         }
+        MyFlags.Flags.setOrigVt(VT);
         Ins.push_back(MyFlags);
       }
       if (NeedsRegBlock && Value == NumValues - 1)
         Ins[Ins.size() - 1].Flags.setInConsecutiveRegsLast();
       PartBase += VT.getStoreSize();
     }
   }
 

lib/Target/SystemZ/SystemZCallingConv.h

public:
   void AnalyzeCallOperands(const SmallVectorImpl<MVT> &Outs,
                            SmallVectorImpl<ISD::ArgFlagsTy> &Flags,
                            CCAssignFn Fn) = delete;
 
   bool IsFixed(unsigned ValNo) { return ArgIsFixed[ValNo]; }
   bool IsShortVector(unsigned ValNo) { return ArgIsShortVector[ValNo]; }
 };
 
-// Handle i128 argument types.  These need to be passed by implicit
-// reference.  This could be as simple as the following .td line:
-//    CCIfType<[i128], CCPassIndirect<i64>>,
-// except that i128 is not a legal type, and therefore gets split by
-// common code into a pair of i64 arguments.
-inline bool CC_SystemZ_I128Indirect(unsigned &ValNo, MVT &ValVT,
-                                    MVT &LocVT,
-                                    CCValAssign::LocInfo &LocInfo,
-                                    ISD::ArgFlagsTy &ArgFlags,
-                                    CCState &State) {
-  SmallVectorImpl<CCValAssign> &PendingMembers = State.getPendingLocs();
-
-  // ArgFlags.isSplit() is true on the first part of a i128 argument;
-  // PendingMembers.empty() is false on all subsequent parts.
-  if (!ArgFlags.isSplit() && PendingMembers.empty())
-    return false;
-
-  // Push a pending Indirect value location for each part.
-  LocVT = MVT::i64;
-  LocInfo = CCValAssign::Indirect;
-  PendingMembers.push_back(CCValAssign::getPending(ValNo, ValVT,
-                                                   LocVT, LocInfo));
-  if (!ArgFlags.isSplitEnd())
-    return true;
-
-  // OK, we've collected all parts in the pending list.  Allocate
-  // the location (register or stack slot) for the indirect pointer.
-  // (This duplicates the usual i64 calling convention rules.)
-  unsigned Reg = State.AllocateReg(SystemZ::ArgGPRs);
-  unsigned Offset = Reg ? 0 : State.AllocateStack(8, 8);
-
-  // Use that same location for all the pending parts.
-  for (auto &It : PendingMembers) {
-    if (Reg)
-      It.convertToReg(Reg);
-    else
-      It.convertToMem(Offset);
-    State.addLoc(It);
-  }
-
-  PendingMembers.clear();
-
-  return true;
-}
-
 } // end namespace llvm
 
 #endif

lib/Target/SystemZ/SystemZCallingConv.td

def CC_SystemZ : CallingConv<[
   // A SwiftSelf is passed in callee-saved R10.
   CCIfSwiftSelf<CCIfType<[i64], CCAssignToReg<[R10D]>>>,
 
   // A SwiftError is passed in callee-saved R9.
   CCIfSwiftError<CCIfType<[i64], CCAssignToReg<[R9D]>>>,
 
   // Force long double values to the stack and pass i64 pointers to them.
   CCIfType<[f128], CCPassIndirect<i64>>,
-  // Same for i128 values.  These are already split into two i64 here,
-  // so we have to use a custom handler.
-  CCIfType<[i64], CCCustom<"CC_SystemZ_I128Indirect">>,
+  // Same for i128 values.  These are already split into two i64 here.
+  CCIfType<[i64], CCIfSplitFrom<[i128], CCPassIndirectBySamePointer<i64>>>,
 
   // The first 5 integer arguments are passed in R2-R6.  Note that R6
   // is call-saved.
   CCIfType<[i32], CCAssignToReg<[R2L, R3L, R4L, R5L, R6L]>>,
   CCIfType<[i64], CCAssignToReg<[R2D, R3D, R4D, R5D, R6D]>>,
 
   // The first 4 float and double arguments are passed in even registers F0-F6.
   CCIfType<[f32], CCAssignToReg<[F0S, F2S, F4S, F6S]>>,

utils/TableGen/CallingConvEmitter.cpp

if (Action->isSubClassOf("CCPredicateAction")) {
     
     if (Action->isSubClassOf("CCIfType")) {
       ListInit *VTs = Action->getValueAsListInit("VTs");
       for (unsigned i = 0, e = VTs->size(); i != e; ++i) {
         Record *VT = VTs->getElementAsRecord(i);
         if (i != 0) O << " ||\n    " << IndentStr;
         O << "LocVT == " << getEnumName(getValueType(VT));
       }
-
+    } else if (Action->isSubClassOf("CCIfSplitFrom")) {
+      ListInit *VTs = Action->getValueAsListInit("VTs");
+      for (unsigned i = 0, e = VTs->size(); i != e; ++i) {
+        Record *VT = VTs->getElementAsRecord(i);
+        if (i != 0) O << " ||\n    " << IndentStr;
+        O << "ArgFlags.getOrigVt() == " << getEnumName(getValueType(VT));
+      }
     } else if (Action->isSubClassOf("CCIf")) {
       O << Action->getValueAsString("Predicate");
     } else {
       errs() << *Action;
       PrintFatalError("Unknown CCPredicateAction!");
     }
     
     O << ") {\n";
if (Action->isSubClassOf("CCDelegateTo")) {
           << IndentStr << IndentStr << "LocInfo = CCValAssign::ZExtUpper;\n"
           << IndentStr << "else\n"
           << IndentStr << IndentStr << "LocInfo = CCValAssign::AExtUpper;\n";
       }
     } else if (Action->isSubClassOf("CCBitConvertToType")) {
       Record *DestTy = Action->getValueAsDef("DestTy");
       O << IndentStr << "LocVT = " << getEnumName(getValueType(DestTy)) <<";\n";
       O << IndentStr << "LocInfo = CCValAssign::BCvt;\n";
+    } else if (Action->isSubClassOf("CCPassIndirectBySamePointer")) {
+      Record *DestTy = Action->getValueAsDef("DestTy");
+      O << IndentStr << "LocVT = " << getEnumName(getValueType(DestTy)) <<";\n";
+      O << IndentStr << "LocInfo = CCValAssign::Indirect;\n";
+      O << IndentStr << "if (!ArgFlags.isSplit()) {\n"
+        << IndentStr << IndentStr << "State.addLoc(State.lastLoc());\n"
+        << IndentStr << IndentStr << "return false;\n"
+        << IndentStr << "}\n";
     } else if (Action->isSubClassOf("CCPassIndirect")) {
       Record *DestTy = Action->getValueAsDef("DestTy");
       O << IndentStr << "LocVT = " << getEnumName(getValueType(DestTy)) <<";\n";
       O << IndentStr << "LocInfo = CCValAssign::Indirect;\n";
     } else if (Action->isSubClassOf("CCPassByVal")) {
       int Size = Action->getValueAsInt("Size");
       int Align = Action->getValueAsInt("Align");
       O << IndentStr