[cfe-dev] Clang C ABI x64.. bugs?

[Christoffer Lernö via cfe-dev]

Hi, I tried the Discord to get some answers but got the suggestion to try the mailing list instead.

1. I found the following in classification of types for x64:

> if (const VectorType *VT = Ty->getAs<VectorType>()) {
>     uint64_t Size = getContext().getTypeSize(VT);
>     if (Size == 1 || Size == 8 || Size == 16 || Size == 32) {
>       // gcc passes the following as integer:
>       // 4 bytes - <4 x char>, <2 x short>, <1 x int>, <1 x float>
>       // 2 bytes - <2 x char>, <1 x short>
>       // 1 byte  - <1 x char>
>       Current = Integer;

Size 8, 16, 32 seem reasonable, but 1? That seems inconsistent and at odds with the comments.

2. Looking at 
void X86_64ABIInfo::postMerge(unsigned AggregateSize, Class &Lo,
                              Class &Hi) const {

There is a comment: "(d) If SSEUP is not preceded by SSE or SSEUP, it is converted to SSE.”

This is the code:

> if (Hi == SSEUp && Lo != SSE)
>     Hi = SSE;

Seems to be missing a && Lo != SSEUP if that is a possibility (maybe it isn’t?)

3. In X86_64ABIInfo::classify

We have this code for constant arrays:

>     // Otherwise implement simplified merge. We could be smarter about
>     // this, but it isn't worth it and would be harder to verify.
>     Current = NoClass;
>     uint64_t EltSize = getContext().getTypeSize(AT->getElementType());
>     uint64_t ArraySize = AT->getSize().getZExtValue();
> 
>     // The only case a 256-bit wide vector could be used is when the array
>     // contains a single 256-bit element. Since Lo and Hi logic isn't extended
>     // to work for sizes wider than 128, early check and fallback to memory.
>     //
>     if (Size > 128 &&
>         (Size != EltSize || Size > getNativeVectorSizeForAVXABI(AVXLevel)))
>       return;


The return here is a bit suspicious together with the comment. It says ”fallback to memory” – which seems to assume that Current is set to Memory (happens earlier in the method), however it seems to overlook that we just set Current to ”NoClass” so the fallback would be ”NoClass”?

These confused me so I wonder if they are bugs or if I’m misunderstanding what the code does.

/Christoffer