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

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 15 inline comments as done.
erichkeane added a comment.

Thanks for the review @rjmccall .  Updated patch coming momentarily.  I audited my 'byval' use as well.  I still have 1 open, the 'array' comment which I wasn't clear what you meant.



================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:719
+         !getContext().getTargetInfo().hasInt128Type()))
+      return getNaturalAlignIndirect(Ty);
+
----------------
rjmccall wrote:
> It's very weird for 64 and 128 to be showing up as constants in the default ABI rule.
Good point.  I rewrote this in terms of the Context.LongLongTy and Context.Int128Ty.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:1537
+  return (isPromotableIntegerType(RetTy) ? ABIArgInfo::getExtend(RetTy)
+                                         : ABIArgInfo::getDirect());
 }
----------------
rjmccall wrote:
> Won't this treat literally all ExtInt types as either extend or direct?
Yes it does, and I justified doing it that way at one point, but I cannot remember why.

NVPTX seems to pass everything by value in return anyway(including large structures!), so I'm doing that one on purpose.

SparcV9 actually has a 256 bit limit, so I added a test to explicitly check it.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:4938
+    if (EIT->getNumBits() > 128)
+      return getNaturalAlignIndirect(Ty, /*ByVal=*/true);
+
----------------
rjmccall wrote:
> Does this need to consider the aggregate-as-array logic below?
I'm not sure what you mean by this?  Are you suggesting I could/should pass this as an array instead of indirectly?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5014
+    if (EIT->getNumBits() > 128)
+      return getNaturalAlignIndirect(RetTy, /*ByVal=*/true);
+
----------------
rjmccall wrote:
> `byval` is not legal on return values.
Ah, neat! I didn't realize that.  Thanks.


================
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())
----------------
rjmccall wrote:
> Ugh.  Please go ahead and rename this to make it clear that it's an NVPTX helper function.
:)  Moved to a member function.  Also, coerceToIntArrayWithLimit made sense to do so as well, since it was the caller of this.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:8988
 
-  uint64_t Size = getContext().getTypeSize(Ty);
+  uint64_t Size = getContext().getIntWidth(Ty);
 
----------------
rjmccall wrote:
> Why this change?
So that line 9000 will 'extend' 63 bit sizes.  getTypeSize rounds to power of 2, getIntWidth doesn't. 

As it was unclear, I've instead added this as an explicit case below.


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

https://reviews.llvm.org/D79118