Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent handling of padding space with bitcasts involving vectors of x86_fp80 #68566

Open
DaMatrix opened this issue Oct 9, 2023 · 3 comments

Comments

@DaMatrix
Copy link
Contributor

DaMatrix commented Oct 9, 2023

While working on #66894 I've run into a lot of difficulties dealing with vectors of x86_fp80. These are "weird" in that they're the only vector element type which contains padding space (Compiler Explorer example which shows that the 80-bit vector elements are 16 bytes wide on x86-64, with 6 uninitialized padding bytes between them). However, much of the existing code for dealing with these vectors in both LLVM and clang seems to assume that all vectors are tightly packed without additional padding or that the padding space is initialized to zero, which results in unexpected behavior when attempting to do bitcast operations on them. Some examples of relevant broken code:

This test assumes that the padding space is initialized to zero:
https://github.com/llvm/llvm-project/blob/8768741800aae37a825864e2ee782484ed073ce9/clang/test/CodeGen/const-init.c#L138-L148

This assumes that there is no padding space at all (which triggers some assertions):
https://github.com/llvm/llvm-project/blob/f05f52ed4e74cf857bf07aef5dbd5d0861591a14/llvm/lib/IR/Instructions.cpp#L3747-L3750

This assumes there is no padding space at all:
https://github.com/llvm/llvm-project/blob/f05f52ed4e74cf857bf07aef5dbd5d0861591a14/llvm/test/Transforms/InstCombine/extractelement.ll#L606-L620

Fixing this will require some rather far-reaching changes, for example CastInst::castIsValid would need to accept a DataLayout in order to determine the actual size of an x86_fp80 with padding.

@llvmbot
Copy link
Collaborator

llvmbot commented Oct 9, 2023

@llvm/issue-subscribers-backend-x86

While working on #66894 I've run into a lot of difficulties dealing with vectors of x86_fp80. These are "weird" in that they're the only vector element type which contains padding space ([Compiler Explorer](https://godbolt.org/#z:OYLghAFBqd5QCxAYwPYBMCmBRdBLAF1QCcAaPECAMzwBtMA7AQwFtMQByARg9KtQYEAysib0QXACx8BBAKoBnTAAUAHpwAMvAFYTStJg1DIApACYAQuYukl9ZATwDKjdAGFUtAK4sGe1wAyeAyYAHI%2BAEaYxCBmpAAOqAqETgwe3r56icmOAkEh4SxRMXF2mA6pQgRMxATpPn5ctpj2uQxVNQT5YZHRsbbVtfWZTQqDXcE9RX1mAJS2qF7EyOwcaAxjANS0AsCb6IsR9JtU8QAcGgD6CqIGxJsmAMwAIpsQO0b7h/Szm1ybAHo3h89gcvEdML9Hk8rBoAIImeFeZKfU4XS4AN3KDxe212X3Bx0ulyYBAIxDwES8BEwxKgWIcJGueAAXpgII85rNZjDEXC0VcGZtgJgCJjyhBfiYAOywuGbBWbYiipYME7nQXlGUWdXom5iGqkXVXfV3GXPXnw818vljYheBzG67kh0EB6yvGfMEQzZMI0RGHui2POUC532x0isV212S91yxVKlXENXap2mw3p241IOWhHSi3wjjzWicACsvD8HC0pFQnDc1msmwUi2WmAeZkePFIBE0xfmAGsQI9HgA6EcTydTgBs%2Bk4kkrfdrnF4ChAGh7ffmcFgSDQLHidGi5Eo%2B8P9BiyAMRkuLoYA9IWAxeBWADU8JgAO4AeXijE43Y0LQNLEGuEAREuETBDUACeAG8FBzDEDB34RNo5S9twvD7mwgjfgwtBwdWvBYFSwBuGItBrlhj6YCwhjAOIxGPngyoVFi1E1pgqjlNSqzdsENKlsxtCUsQsEeFgS7kngLDwfMVAGMACjvl%2Bv7/jR/CCCIYjsFIMiCIoKjqMxuhNNexiNpY%2BiUmukDzKg8RtNRvCoFixAUlgdmSs0rSpC4DDuJ4DT%2BIF3SFMUWRJCkAjDI0CTRW04W9DEowtBhlTjHFehlBUAgdLUyXTKlAydNlozjEVkVcPMLZLCsEgluWi7MXWHCbKoZzTgAtNOkibFeDFvHeA6/BAuCECQHZdrMvCYVo3KkEOI7jlOa0jrOwkLqQclcBoG5VjWbWruum7EduMCICgqAHkeZAUBAZ53SAynMPECgIKgBC0c%2Bb4fj%2Bf5VoBdAgWBEHMYhsHwaQkPIah6EONDOGMAQ%2BGEUupFeORlHUd2WD0UYTE1vgbGOBxS7cbxNLQ4JLRLqJETichkmrDWMlyVhClKSp/3qUDvBacItx6dIgtGWoS66HEFkoFZNgM95DlOakLm1u5nmYIrvkZc4ECuOVpCBJMEV9E02QxWkwUjAlOSpFVpva3l7RZVb8W5W0BUTAUKU5S7GRu5Vxs%2BzVCz1XpTUcBWpCHa5nAdV1vX9cAyDIMN9r3mNE1EPc5gzXNW6DsOY7retc4cNtMfLhwJ0bvN/Zl2YLVHSuZ0LfM7nJM4khAA%3D%3D%3D) example which shows that the 80-bit vector elements are 16 bytes wide on x86-64, with 6 uninitialized padding bytes between them). However, much of the existing code for dealing with these vectors in both LLVM and clang seems to assume that all vectors are tightly packed without additional padding or that the padding space is initialized to zero, which results in unexpected behavior when attempting to do bitcast operations on them. Some examples of relevant broken code:

This test assumes that the padding space is initialized to zero:
https://github.com/llvm/llvm-project/blob/8768741800aae37a825864e2ee782484ed073ce9/clang/test/CodeGen/const-init.c#L138-L148

This assumes that there is no padding space at all (which triggers some assertions):
https://github.com/llvm/llvm-project/blob/f05f52ed4e74cf857bf07aef5dbd5d0861591a14/llvm/lib/IR/Instructions.cpp#L3747-L3750

This assumes there is no padding space at all:
https://github.com/llvm/llvm-project/blob/f05f52ed4e74cf857bf07aef5dbd5d0861591a14/llvm/test/Transforms/InstCombine/extractelement.ll#L606-L620

Fixing this will require some rather far-reaching changes, for example CastInst::castIsValid would need to accept a DataLayout in order to determine the actual size of an x86_fp80 with padding.

@nikic
Copy link
Contributor

nikic commented Oct 9, 2023

Vectors in LLVM are defined to be tightly packed, so <2 x x86_fp80> must have size 2*80 on the LLVM level at least.

https://godbolt.org/z/rcev7Ps1a shows that it does get interpreted this way by Clang at least in some cases. GCC uses the 16 byte representation.

Assuming we don't need ABI compatibility with GCC here, we should fix any cases where Clang assumes x86_fp80 in vectors to be padded.

@DaMatrix
Copy link
Contributor Author

DaMatrix commented Oct 9, 2023

Alright, I'd assumed that preserving the padding space between x86_fp80 elements would make sense, both for reasons of alignment (which is why the padding space exists in the first place), and the fact that vectors of all other element types (with the notable exception of OpenCL bool vectors) can be bitcast-ed/memcpy-ed directly to/from an appropriately sized array - wouldn't be possible if we omit the padding space.

I'll also assume that ABI compatibility isn't an issue here, and going through and removing padding space from these vectors on the clang side of things should be much easier than what I'd initially suggested. I'll give that a shot shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants