[PATCH] D25204: Register Calling Convention, Clang changes

[Reid Kleckner via cfe-commits]

rnk added inline comments.


> erichkeane wrote in ItaniumMangle.cpp:1236-1237
> Right, good catch.  I looked at Mangle.cpp which does something very similar, and assumes that FunctionType is a valid cast here, so I've switched this here too, please let me know if that is a wrong assumption.

IMO this would be simpler if we did:

  if (IsRegCall)
    mangleRegCallName(II); // implement this elsewhere
  else
    mangleSourceName(II);

I don't like that the standard mangleSourceName as you have it has to be concerned with IsRegCall. Without your change, it is a simple function that clearly expresses that the basic unit of Itanium name mangling is decimal length prefixed strings.

> ItaniumMangle.cpp:1203
>    switch (Name.getNameKind()) {
>    case DeclarationName::Identifier: {
>      const IdentifierInfo *II = Name.getAsIdentifierInfo();

What mangling should happen for operator overloads and all other kinds of DeclarationName? Please add tests for these cases

> TargetInfo.cpp:1954
> +  ABIArgInfo classifyRegCallStructTypeImpl(QualType Ty, 
> +                                       unsigned &neededInt, 
> +                                       unsigned &neededSSE) const;

format

> TargetInfo.cpp:3297
> +ABIArgInfo X86_64ABIInfo::classifyRegCallStructTypeImpl(
> +  QualType Ty, unsigned &neededInt, unsigned &neededSSE) const {
> +  auto RT = Ty->getAs<RecordType>();

Please name variables in LLVM style

> TargetInfo.cpp:3321
> +    } else {
> +      unsigned localNeededInt, localNeededSSE;
> +      if (classifyArgumentType(FD->getType(), std::numeric_limits<unsigned>::max(),

variable naming

> TargetInfo.cpp:3352-3354
> +  unsigned freeIntRegs = IsRegCall ? 11 : 6;
> +  unsigned freeSSERegs = IsRegCall ? 16 : 8;
> +  unsigned neededInt, neededSSE;

Since you're touching most of this, can you make this code standardize on LLVM's variable naming convention: http://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

> erichkeane wrote in TargetInfo.cpp:3306
> Hmmm... I'm not sure what behavior would be expected in that case.  Also, I just looked at a bunch of similar 'for' loops for various other reasons, and I don't really see where that is tested elsewhere (so I don't really see how to, besides I.isVirtual?).  In that case, I would suspect that the class is not put into a register?

You don't need to check if each base is dynamic, just check CXXRD->isDynamicClass() and make it return indirectly if so.

https://reviews.llvm.org/D25204