This is an archive of the discontinued LLVM Phabricator instance.

[VE][isel] Map EVT vectors to vector registers.
Needs ReviewPublic

Authored by simoll on Dec 23 2020, 3:09 AM.

Details

Summary
  1. Let targets specify their own conversion for EVTs.
  2. Use this to map (almost all) vector EVTs to vector register for the VE target. Also use this across function calls in the 'fastcc'.

Without this patch, isel scalarizes "weird" vector types such as <17 x i16>. This is not really an option for long-vector architectures such as SX-Aurora. This patch adds a custom callback to let the target decide to map also those EVTs to vector registers.

Diff Detail

Event Timeline

simoll created this revision.Dec 23 2020, 3:09 AM
simoll requested review of this revision.Dec 23 2020, 3:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2020, 3:09 AM
simoll updated this revision to Diff 313533.Dec 23 2020, 4:26 AM

NFC. Removed stray comment.

FYI i have tested this against check-all with clang and all (including experimental) targets enabled.

simoll added a comment.Jan 8 2021, 2:53 AM

This is one of those patches that has the potential to linger on without review for 1Y+ .. let me know if there is anything i can do to get this reviewed, as it really helps with bringing good vector architecture (long SIMD) support to LLVM. Thanks.

arsenm added a comment.Jan 8 2021, 6:18 AM

This is one of those patches that has the potential to linger on without review for 1Y+ .. let me know if there is anything i can do to get this reviewed, as it really helps with bringing good vector architecture (long SIMD) support to LLVM. Thanks.

Can I interest you in moving to GlobalISel instead of trying to teach the DAG about new types :)

llvm/lib/CodeGen/TargetLoweringBase.cpp
963

don't need hasValue()

964

*CustomLK

This is one of those patches that has the potential to linger on without review for 1Y+ .. let me know if there is anything i can do to get this reviewed, as it really helps with bringing good vector architecture (long SIMD) support to LLVM. Thanks.

Can I interest you in moving to GlobalISel instead of trying to teach the DAG about new types :)

Definitely! GlobalISel sure has my interest and i am following the development closely. However, we have to make do with what we've got, which is dag-based downstream implementation. I see it that way: the quicker we get this in a stable state and expose the traditional way's intrinsic limitations, the closer we are to switching/contributing to global isel :)

simoll updated this revision to Diff 316060.Jan 12 2021, 4:40 AM
simoll marked 2 inline comments as done.
simoll edited the summary of this revision. (Show Details)

NFC. Usage of Optional. Rebased onto committed D93709 .

This is actually not a VE-specific change. Adding more RISC-V people.. if you are interested in being able to map irregular vector EVTs to vector registers in your calling convention, this one is for you.

Can you rebase on main? I didn't realize this patch was open, and messed with some of the relevant code. See D105591

efriedma added inline comments.Aug 23 2021, 1:11 PM
llvm/lib/CodeGen/TargetLoweringBase.cpp
962

getTypeConversion is pretty closely tied to the conversions we actually support in type legalization. I'm not sure it's a good idea to let the target completely override the logic here. If there's some specific aspect of this function you'd like to customize, we can look at adding a more specific hook.

simoll added inline comments.Aug 24 2021, 8:20 AM
llvm/lib/CodeGen/TargetLoweringBase.cpp
962

getCustomTypeConversion returns a LegalizeKind with limited options for legalization strategies - we could even type check LegalizeKind in its constructor and assert if an illegal/unimplemented legalization step is attempted.

The fundamental issue with calling back from getTypeConversion is that it tries to peg all legalization steps to MVTs. That breaks down as soon as the intermediate step of legalization can only be an EVT. For example:

v17i17 -> v17i32 -> v256i32

efriedma added inline comments.Aug 24 2021, 12:14 PM
llvm/lib/CodeGen/TargetLoweringBase.cpp
962

I'm not quite sure I understand the issue here.

The logic in this function should be doing something like v17i17 -> v32i17 -> v32i32, then doing a lookup in ValueTypeActions, I think? If that isn't working, there's probably something wrong with the logic here. Looking at the code, maybe we miss the v32i17 -> v32i32 step if v32i32 isn't legal? But if that isn't working, we should probably just fix it for all targets.

At the very least, the getCustomTypeConversion() should go after the VT.isSimple() check; there isn't any reason for the ValueTypeActions table to be missing entries.

simoll added inline comments.Aug 25 2021, 2:44 AM
llvm/lib/CodeGen/TargetLoweringBase.cpp
962

[..] Looking at the code, maybe we miss the v32i17 -> v32i32 step if v32i32 isn't legal? But if that isn't working, we should probably just fix it for all targets.

Not quite. getTypeConversion(v17i17) promotes the element type to i17. Conversion stops here.
getVectorTypeBreakdown sees that v17i32 is not a power-of-two vector and decides to scalarize.

At the very least, the getCustomTypeConversion() should go after the VT.isSimple() check; there isn't any reason for the ValueTypeActions table to be missing entries.

The ValueTypeActions table is an implementation detail of getTypeConversion. It is intentional that getCustomTypeConversion overrides this.

simoll updated this revision to Diff 368602.Aug 25 2021, 4:04 AM
  • Cleanup
  • Rebased
craig.topper added inline comments.Aug 25 2021, 9:06 AM
llvm/lib/CodeGen/TargetLoweringBase.cpp
962

[..] Looking at the code, maybe we miss the v32i17 -> v32i32 step if v32i32 isn't legal? But if that isn't working, we should probably just fix it for all targets.

Not quite. getTypeConversion(v17i17) promotes the element type to i17. Conversion stops here.
getVectorTypeBreakdown sees that v17i32 is not a power-of-two vector and decides to scalarize.

I see that as a bug in getVectorTypeBreakdown. X86 recently had to add a hack to getRegisterTypeForCallingConv/getNumRegistersForCallingConv to make v3f16 work correctly to follow the ABI for a struct of 3 halfs.

simoll added inline comments.Aug 26 2021, 2:56 AM
llvm/lib/CodeGen/TargetLoweringBase.cpp
962

[..] Looking at the code, maybe we miss the v32i17 -> v32i32 step if v32i32 isn't legal? But if that isn't working, we should probably just fix it for all targets.

Not quite. getTypeConversion(v17i17) promotes the element type to i17. Conversion stops here.
getVectorTypeBreakdown sees that v17i32 is not a power-of-two vector and decides to scalarize.

I see that as a bug in getVectorTypeBreakdown. X86 recently had to add a hack to getRegisterTypeForCallingConv/getNumRegistersForCallingConv to make v3f16 work correctly to follow the ABI for a struct of 3 halfs.

We have a similar fix here.
VETargetLowering::getVectorTypeBreakdownForCallingConv calls getCustomTypeConversion in a loop to make parameter passing consistent with type conversion. That way, the problem goes away. eg, v3f32 becomes v256f32

962

[..] Looking at the code, maybe we miss the v32i17 -> v32i32 step if v32i32 isn't legal? But if that isn't working, we should probably just fix it for all targets.

Not quite. getTypeConversion(v17i17) promotes the element type to i17. Conversion stops here.
getVectorTypeBreakdown sees that v17i32 is not a power-of-two vector and decides to scalarize.

I see that as a bug in getVectorTypeBreakdown. X86 recently had to add a hack to getRegisterTypeForCallingConv/getNumRegistersForCallingConv to make v3f16 work correctly to follow the ABI for a struct of 3 halfs.

efriedma added inline comments.Aug 26 2021, 2:18 PM
llvm/lib/CodeGen/TargetLoweringBase.cpp
962

At the very least, the getCustomTypeConversion() should go after the VT.isSimple() check; there isn't any reason for the ValueTypeActions table to be missing entries.

The ValueTypeActions table is an implementation detail of getTypeConversion. It is intentional that getCustomTypeConversion overrides this.

Sort of? We have target hooks that control the contents of the table; see getPreferredVectorAction() and softPromoteHalfType().

simoll added inline comments.Aug 26 2021, 11:35 PM
llvm/lib/CodeGen/TargetLoweringBase.cpp
962

I don't disagree.
But isn't it better to have one concise function that let's you specify exactly what you want, than a bunch of hooks that are all over the place?

simoll added a subscriber: efocht.May 20 2022, 5:41 AM

@kaz7 @efocht Does either of you want to commandeer this patch and become the patch author?

Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2022, 5:41 AM
Herald added a subscriber: StephenFan. · View Herald Transcript