Page MenuHomePhabricator

[ValueTypes] Define MVTs for v128i2/v64i4 as well as i2 and i4.
ClosedPublic

Authored by hgreving on May 9 2022, 10:48 AM.

Details

Summary

Adds MVT::v128i2, MVT::v64i4, and implied MVT::i2, MVT::i4.

Keeps MVT::i2, MVT::i4 lowering actions as expand, which should be
removed once targets set this explicitly.

Adjusts 11 lit tests to reflect slightly different behavior during
DAG combine.

Diff Detail

Event Timeline

hgreving created this revision.May 9 2022, 10:48 AM
hgreving requested review of this revision.May 9 2022, 10:48 AM
jrtc27 added inline comments.May 9 2022, 2:39 PM
llvm/include/llvm/Support/MachineValueType.h
288

We're starting to creep awfully close to this. Downstream we have 5 extra MVTs (though could get away with just 3), and Arm's Morello fork of CHERI LLVM adds another one (but we should really have more than one were we to have it in CHERI LLVM). Is there a plan for what to do when this limit finally gets hit? Because the slow but steady explosion of vector types is causing me some amount of worry for us downstream...

craig.topper added inline comments.May 9 2022, 3:10 PM
llvm/include/llvm/Support/MachineValueType.h
288

We can add 32 to it. I think we can bump it all the way to 256 without issue. It's used to reduce the size of some tables in TargetLowering. Bumping it only increases the size of those tables.

hgreving updated this revision to Diff 428406.May 10 2022, 9:21 AM

clang-format changes. I am aware that some of the changes look like they do not follow existing code. I'm not sure why clang-format does this.

clang-format changes. I am aware that some of the changes look like they do not follow existing code. I'm not sure why clang-format does this.

We've been ignoring clang-format on these files. Maybe we should add // clang-format off markers

Please upload patches with full context using git diff -U999999

clang-format changes. I am aware that some of the changes look like they do not follow existing code. I'm not sure why clang-format does this.

We've been ignoring clang-format on these files. Maybe we should add // clang-format off markers

Can do. TBD.

hgreving updated this revision to Diff 428411.May 10 2022, 9:45 AM

Full context update (excl clang-format off notations yet)

Please upload patches with full context using git diff -U999999

Sorry missed that, done.

hgreving updated this revision to Diff 428495.May 10 2022, 2:08 PM

Added a few clang-format on/off. I suggest to accept some of the enum changes in IntrinsicEmitter.cpp, Function.cpp, it feels unnecessary not to accept clang-format here. I wonder if these deltas are due to different clang-format versions in the past.

Friendly ping on this review.

This revision is now accepted and ready to land.May 21 2022, 5:48 PM
majnemer accepted this revision.Jun 1 2022, 10:37 AM
hgreving updated this revision to Diff 433477.Jun 1 2022, 11:46 AM

Rebase only.

hgreving edited the summary of this revision. (Show Details)Jun 1 2022, 11:53 AM

Reverted the patch in repo and re-pushing again. The only difference of the new push is s/enum_seq_inclusive/enum_seq/ in TargetLoweringBase.cpp.