[PATCH] D79118: Implement _ExtInt ABI for all ABIs in Clang, enable type for ABIs

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

A little cavalier with `byval`; I gave it a thorough audit, but you might want your own pass.



================
Comment at: clang/lib/CodeGen/ABIInfo.h:107
+    // only difference is that this considers _ExtInt as well.
+    bool isPromotableIntegerType(QualType Ty) const;
+
----------------
Maybe add a "ForABI" suffix here so that it's clearer that this is a specialized use-case.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:719
+         !getContext().getTargetInfo().hasInt128Type()))
+      return getNaturalAlignIndirect(Ty);
+
----------------
It's very weird for 64 and 128 to be showing up as constants in the default ABI rule.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1537
+  return (isPromotableIntegerType(RetTy) ? ABIArgInfo::getExtend(RetTy)
+                                         : ABIArgInfo::getDirect());
 }
----------------
Won't this treat literally all ExtInt types as either extend or direct?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4938
+    if (EIT->getNumBits() > 128)
+      return getNaturalAlignIndirect(Ty, /*ByVal=*/true);
+
----------------
Does this need to consider the aggregate-as-array logic below?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5014
+    if (EIT->getNumBits() > 128)
+      return getNaturalAlignIndirect(RetTy, /*ByVal=*/true);
+
----------------
`byval` is not legal on return values.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:6339
+      if (EIT->getNumBits() > 64)
+        return getNaturalAlignIndirect(RetTy, /*ByVal=*/true);
+
----------------
`byval` is not legal on return types.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:6658
 /// Checks if the type is unsupported directly by the current target.
 static bool isUnsupportedType(ASTContext &Context, QualType T) {
   if (!Context.getTargetInfo().hasFloat16Type() && T->isFloat16Type())
----------------
Ugh.  Please go ahead and rename this to make it clear that it's an NVPTX helper function.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:7921
+    if (Size > 64 && RetTy->isExtIntType())
+      return getNaturalAlignIndirect(RetTy, /*ByVal=*/true);
+
----------------
`byval` is not legal on a return.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:8988
 
-  uint64_t Size = getContext().getTypeSize(Ty);
+  uint64_t Size = getContext().getIntWidth(Ty);
 
----------------
Why this change?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10297
+           EIT->getNumBits() > 64))
+        return getNaturalAlignIndirect(Ty, /*ByVal=*/true);
+    }
----------------
Looks like this shouldn't be `byval`.


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

https://reviews.llvm.org/D79118