Clang C ABI x64.. bugs?

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Clang C ABI x64.. bugs?

David Blaikie 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

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Clang C ABI x64.. bugs?

David Blaikie via cfe-dev
Hi Christoffer,

For question 1, I'm not sure size 1 can occur. I think a vector_size of 1 is going to round up to the nearest byte, but I'm not sure.

For question 2, I don't think Lo can ever be SSEUp. We really should extend the Lo/Hi logic to 8 values so we can handle 256 and 512 vectors without hacks. In that case we would have more than SSEUp at a time, but with the Lo/Hi we only have SSEUp in Hi.

For question 3, that looks like it might be a bug.

~Craig


On Wed, Nov 11, 2020 at 3:57 PM Christoffer Lernö via cfe-dev <[hidden email]> wrote:
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

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Clang C ABI x64.. bugs?

David Blaikie via cfe-dev
Thanks for the answers, I guess my reading of the code wasn’t completely off then.. I was getting a little worried.

Now I’m off trying to figure out in what cases one actually gets the ”only send the top 8 bytes” in x64. (Direct-with-offset), 

Christoffer
AEGIK / www.aegik.se

On 12 Nov 2020, at 07:18, Craig Topper <[hidden email]> wrote:

Hi Christoffer,

For question 1, I'm not sure size 1 can occur. I think a vector_size of 1 is going to round up to the nearest byte, but I'm not sure.

For question 2, I don't think Lo can ever be SSEUp. We really should extend the Lo/Hi logic to 8 values so we can handle 256 and 512 vectors without hacks. In that case we would have more than SSEUp at a time, but with the Lo/Hi we only have SSEUp in Hi.

For question 3, that looks like it might be a bug.

~Craig


On Wed, Nov 11, 2020 at 3:57 PM Christoffer Lernö via cfe-dev <[hidden email]> wrote:
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

_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev