This is an archive of the discontinued LLVM Phabricator instance.

[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

Unit TestsFailed

Event Timeline

python3kgae created this revision.Sep 9 2022, 10:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2022, 10:55 PM
python3kgae requested review of this revision.Sep 9 2022, 10:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2022, 10:55 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Update test.

Missing IR tests. No issues in codegen?

Very poor description. Why it was disabled? Why we can enable it now?

Missing IR tests. No issues in codegen?

Very poor description. Why it was disabled? Why we can enable it now?

I'm not sure why it is disabled.
I guess it is never enabled and just assert in ASTContext::getExtVectorType before.
Then https://github.com/llvm/llvm-project/commit/c9edf843fcf954132271214445857498fb47bb72 make it an error instead of assert.

I'm enabling it for HLSL to use _BitInt(16) as 16bit int at https://reviews.llvm.org/D133668
This PR only makes sure it does not fail at AST level.
https://reviews.llvm.org/D133668 will fix the fail in Mangling when codeGen.

python3kgae edited the summary of this revision. (Show Details)Sep 11 2022, 6:31 PM
aaron.ballman added a subscriber: erichkeane.

Missing IR tests. No issues in codegen?

Very poor description. Why it was disabled? Why we can enable it now?

I'm not sure why it is disabled.
I guess it is never enabled and just assert in ASTContext::getExtVectorType before.
Then https://github.com/llvm/llvm-project/commit/c9edf843fcf954132271214445857498fb47bb72 make it an error instead of assert.

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'm enabling it for HLSL to use _BitInt(16) as 16bit int at https://reviews.llvm.org/D133668
This PR only makes sure it does not fail at AST level.
https://reviews.llvm.org/D133668 will fix the fail in Mangling when codeGen.

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.

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.

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?

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?

Limit BitInt vector to byte-sized and power of 2 BitInt.

Thanks for all the comments.
I've limit BitInt vector to BitInt with byte-sized and power of 2 NumBits.

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?

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.

aaron.ballman added inline comments.Sep 16 2022, 8:25 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
3024 ↗(On Diff #460659)

We might as well spell out the failure conditions and make a more readable diagnostic.

Allow BitInt vector for Microsoftmangle and add codeGen tests for BitInt vector.

python3kgae marked an inline comment as done.Sep 16 2022, 7:34 PM

The CFE Changes look correct, but @eli.friedman/@craig.topper should comment as to whether this is acceptable.

This is acceptable to me.

clang/lib/AST/MicrosoftMangle.cpp
3111 ↗(On Diff #460958)

Why is an explicit cast needed here?

python3kgae marked an inline comment as done.Sep 16 2022, 11:45 PM
python3kgae added inline comments.
clang/lib/AST/MicrosoftMangle.cpp
3111 ↗(On Diff #460958)

Because ET and BitIntTy have incompatible types for the select. (error : incompatible operand types ('const clang::BuiltinType *' and 'const clang::BitIntType *')

cast to const Type * will make them agree as const Type *.

erichkeane accepted this revision.Sep 19 2022, 5:56 AM
This revision is now accepted and ready to land.Sep 19 2022, 5:56 AM
This revision was automatically updated to reflect the committed changes.
python3kgae marked an inline comment as done.