This is an archive of the discontinued LLVM Phabricator instance.

[AST] clang::VectorType supports any size (that fits in unsigned)
ClosedPublic

Authored by sammccall on Apr 2 2020, 1:53 PM.

Details

Summary

This matches llvm::VectorType.
It moves the size from the type bitfield into VectorType, increasing size by 8
bytes (including padding of 4). This is OK as we don't expect to create terribly
many of these types.

c.f. D77313 which enables large power-of-two sizes without growing VectorType.

Diff Detail

Event Timeline

sammccall created this revision.Apr 2 2020, 1:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2020, 1:53 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

If you're going to add code to check for it, we might as well add testcases for ridiculous sizes, like (__int128_t)1 << 100.

I think this makes sense.

sammccall updated this revision to Diff 254646.Apr 2 2020, 4:50 PM

Add 1<<100 testcase, and clang-format

If you're going to add code to check for it, we might as well add testcases for ridiculous sizes, like (__int128_t)1 << 100.

Done. Looks like this was previously an assertion failure.

efriedma accepted this revision.Apr 2 2020, 5:55 PM

LGTM

This revision is now accepted and ready to land.Apr 2 2020, 5:55 PM
hokein accepted this revision.Apr 3 2020, 12:15 AM

Thanks!

fhahn added a subscriber: fhahn.Apr 3 2020, 2:42 AM
fhahn added inline comments.
clang/include/clang/AST/Type.h
3238

I think you could keep NumElements in VectorTypeBitfields (just remove the bit specifier), as the size of those bitfields can be up to 8 bytes (see the static asserts around line 1775). That should be fine as long as NumTypeBits + 3 <= 32.

sammccall updated this revision to Diff 254769.Apr 3 2020, 6:18 AM

Store NumElements in the bitfield after all.

sammccall marked an inline comment as done.Apr 3 2020, 6:18 AM
sammccall added inline comments.
clang/include/clang/AST/Type.h
3238

Ah, you're right. I just assumed it was taking all the rest of the bits, but there are another 4 bytes free :-\
Done.

hokein accepted this revision.Apr 3 2020, 6:44 AM

still LG.

This revision was automatically updated to reflect the committed changes.