Page MenuHomePhabricator

type legalization for vp_load and vp_store.
Needs ReviewPublic

Authored by hussainjk on Sep 7 2021, 10:27 AM.

Details

Summary

Implemented functions in the SelectionDAG legalizer for integer type promotion, vector widening, and vector splitting for vp_load and vp_store nodes.

Diff Detail

Event Timeline

hussainjk created this revision.Sep 7 2021, 10:27 AM
hussainjk requested review of this revision.Sep 7 2021, 10:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2021, 10:27 AM

Tests?

llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
1920

Just write the word "or" here instead of the ||.

1922

PromoteTargetBoolean would be incorrect for EVL wouldn't it? EVL should always ZERO_EXTEND right?

1940

Same question as above.

craig.topper added inline comments.Sep 7 2021, 10:43 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
748

Should we choose the extend type from N if it was already an extending VP load? Similar to PromoteIntRes_LOAD.

hussainjk updated this revision to Diff 371156.Sep 7 2021, 12:48 PM

Addressing review comments.

Tests?

Hm how test? Selection doesn't work at this point.

Tests?

Hm how test? Selection doesn't work at this point.

Didn't D108999 add Selection for RISCV?

Hm how test? Selection doesn't work at this point.

Didn't D108999 add Selection for RISCV?

Yes you should be able to test this with RISCV. Anything that's not i1/i8/i16/i32 but is less than i64 will need promoting. Also see D108288 for examples.

llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
1921

I don't think we expect EVL to need promoting - do we? It should always be a legal target-specific type, reported by TLI.getVPExplicitVectorLengthTy(). I don't know how well we're asserting on that right now.

hussainjk updated this revision to Diff 371657.Sep 9 2021, 10:51 AM

Fixing some miner logic issues.

hussainjk updated this revision to Diff 371660.Sep 9 2021, 10:54 AM

Adding tests using RISCV lowering

craig.topper added inline comments.Sep 9 2021, 11:07 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
1924

GetPromotedInteger -> ZExtPromotedInteger. We need to force the upper bits to 0.

1925

5 -> 6 if I've counted the number of operands correctly.

1933

Should we assert that the store isn't indexed since this call would be incorrect if it was?

1944

GetPromotedInteger -> ZExtPromotedInteger.

hussainjk updated this revision to Diff 371780.Sep 9 2021, 9:56 PM

Added more legalization to this patch rather than putting it into another patch. Added another form of getLoadVP that was needed by the legalization. Expanded tests to cover vector splitting.

hussainjk updated this revision to Diff 371782.Sep 9 2021, 10:15 PM

Addressing review comments.

craig.topper added inline comments.Sep 9 2021, 10:29 PM
llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp
1933

Why does this use isIndexed() but load uses N->getAddressingMode() == ISD::UNINDEXED. Are the interfaces for checking indexed different for loads and stores?

llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
7700

Why should this be different than the assert in getLoad?

hussainjk updated this revision to Diff 371791.Sep 9 2021, 11:22 PM

More assertion fixes.

hussainjk retitled this revision from Integer type legalization for vp_load and vp_store. to type legalization for vp_load and vp_store..Sep 9 2021, 11:32 PM
hussainjk edited the summary of this revision. (Show Details)
frasercrmck added inline comments.Sep 10 2021, 2:02 AM
llvm/test/CodeGen/Generic/fixed-vector-vp-mem-legalization.ll
3

We shouldn't put RISC-V tests in Generic unless we know the target is built. So either this needs to use REQUIRES: or we put it in CodeGen/RISCV. I prefer the latter as that's where I've seen all other target-specific legalization tests go.

hussainjk added inline comments.Sep 10 2021, 10:48 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
7700

These assertions fail on cases that masked loads do not.

Addressing review comments.

craig.topper added inline comments.Sep 10 2021, 1:16 PM
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
7700

But why? Seems weird to have an extending load that doesn't extend.