search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.32k stars 11.69k forks source link

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

Open DaMatrix opened 12 months ago

DaMatrix commented 12 months ago

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)%3B%0Ausing+int16_vec+%3D+short+attribute((vector_size(32)))%3B%0A%0Afp80_vec+bit_cast_vec(int16_vec+vec)+%7B%0A++++return+(fp80_vec)vec%3B%0A%7D%0A'),l:'5',n:'0',o:'C%2B%2B+source+%231',t:'0')),k:37.545691906005224,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:clang170assert,deviceViewOpen:'1',filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'1',intel:'0',libraryCode:'0',trim:'1'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,libs:!(),options:'-emit-llvm+-march%3Dskylake',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+clang+17.0.1+(assertions)+(Editor+%231)',t:'0')),k:50,l:'4',m:31.542857142857144,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'x86-64+clang+17.0.1+(assertions)',editorid:1,fontScale:14,fontUsePx:'0',j:1,wrap:'1'),l:'5',n:'0',o:'Output+of+x86-64+clang+17.0.1+(assertions)+(Compiler+%231)',t:'0')),header:(),l:'4',m:68.45714285714286,n:'0',o:'',s:0,t:'0')),k:62.45430809399477,l:'3',n:'0',o:'',t:'0')),l:'2',n:'0',o:'',t:'0')),version:4)):
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 commented 12 months ago

@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://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,selection:(endColumn:1,endLineNumber:3,positionColumn:1,positionLineNumber:3,selectionStartColumn:1,selectionStartLineNumber:3,startColumn:1,startLineNumber:3),source:'using+fp80_vec+%3D+long+double+__attribute__((vector_size(32)))%3B%0Ausing+int16_vec+%3D+short+__attribute__((vector_size(32)))%3B%0A%0Afp80_vec+bit_cast_vec(int16_vec+vec)+%7B%0A++++return+(fp80_vec)vec%3B%0A%7D%0A'),l:'5',n:'0',o:'C%2B%2B+source+%231',t:'0')),k:37.545691906005224,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:clang170assert,deviceViewOpen:'1',filters:(b:'0',binary:'1',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'0',execute:'1',intel:'0',libraryCode:'0',trim:'1'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,libs:!(),options:'-emit-llvm+-march%3Dskylake',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+clang+17.0.1+(assertions)+(Editor+%231)',t:'0')),k:50,l:'4',m:31.542857142857144,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'x86-64+clang+17.0.1+(assertions)',editorid:1,fontScale:14,fontUsePx:'0',j:1,wrap:'1'),l:'5',n:'0',o:'Output+of+x86-64+clang+17.0.1+(assertions)+(Compiler+%231)',t:'0')),header:(),l:'4',m:68.45714285714286,n:'0',o:'',s:0,t:'0')),k:62.45430809399477,l:'3',n:'0',o:'',t:'0')),l:'2',n:'0',o:'',t:'0')),version:4)): 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 commented 12 months ago

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 commented 12 months ago

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.