Remove check which disable BitInt as element type for ext_vector.
Enabling it for HLSL to use _BitInt(16) as 16bit int at https://reviews.llvm.org/D133668
Paths
| Differential D133634
[clang] Allow vector of BitInt ClosedPublic Authored by python3kgae on Sep 9 2022, 10:55 PM.
Details Summary Remove check which disable BitInt as element type for ext_vector. Enabling it for HLSL to use _BitInt(16) as 16bit int at https://reviews.llvm.org/D133668
Diff Detail
Event TimelineComment Actions Missing IR tests. No issues in codegen? Very poor description. Why it was disabled? Why we can enable it now? Comment Actions
I'm not sure why it is disabled. I'm enabling it for HLSL to use _BitInt(16) as 16bit int at https://reviews.llvm.org/D133668 Comment Actions
It was disabled because it wasn't intentionally enabled -- we've not thought through the best way to expose vectors of _BitInt yet and we didn't want anyone to get tied into an accidental ABI by leaving them exposed.
I think this requires more consideration. I think we *want* to enable vectors of _BitInt types, but I think we need a more full-featured plan for supporting them. For example, do we want to allow a vector of _BitInt(5) objects, or do we want to support only powers-of-two sized _BitInts? How do we ensure ABI compatibility with GCC for these vector types? That sort of thing. CC @erichkeane who worked on the initial _ExtInt implementation in case he's got additional thoughts. Comment Actions There was no motivation for disabling this other than not wanting/caring enough to define the ABI and writing sufficient tests. While I don't think there is sufficient testing here around ABI/etc, I have no problem with this. Comment Actions I'm a little nervous about non-byte-sized non-power of 2 elements. The memory layout handling gets a little weird in the backend. @efriedma what do you think? Comment Actions I think the worst of the backend issues with vectors of non-byte-size integers have been resolved? Work on that has progressed slowly for a very long time. There have been a constant stream of issues. (See, for example, D129164.) But LangRef does define the memory layout, and CodeGen does essentially implement that layout. It's not clear to me we actually want to use that layout for vectors of non-power-of-two types. Load/store for bit-packed vectors isn't efficient. Maybe we can just enable this specifically for types which are a power of two multiple of 8 for now (where there's one obvious layout), and consider the other cases later? Comment Actions Thanks for all the comments. For the ABI, is it OK to let mangleType of VectorType treat BitInt like other element types, call mangleType of BitInt for the element type part? Comment Actions The CFE Changes look correct, but @eli.friedman/@craig.topper should comment as to whether this is acceptable. It still needs a few codegen tests however, to make sure the basic operations work, and that these are passed correctly.
Comment Actions
This is acceptable to me.
python3kgae added inline comments.
This revision is now accepted and ready to land.Sep 19 2022, 5:56 AM Closed by commit rG649a59712ffb: [clang] Allow vector of BitInt (authored by python3kgae). · Explain WhySep 19 2022, 9:27 AM This revision was automatically updated to reflect the committed changes. python3kgae marked an inline comment as done.
Revision Contents
Diff 461233 clang/include/clang/Basic/DiagnosticSemaKinds.td
clang/lib/AST/ASTContext.cpp
clang/lib/AST/MicrosoftMangle.cpp
clang/lib/Sema/SemaType.cpp
clang/test/CodeGenCXX/ext-int-vector-abi.cpp
clang/test/CodeGenCXX/ext-int.cpp
clang/test/Sema/builtin-classify-type.c
clang/test/SemaCXX/ext-int.cpp
|
We might as well spell out the failure conditions and make a more readable diagnostic.