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

[simoll]

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.

[simoll]

NFC. Removed stray comment.

[simoll]

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

[simoll]

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]

In D93755#2486281, @simoll wrote:

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

[simoll]

In D93755#2486652, @arsenm wrote:

In D93755#2486281, @simoll wrote:

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]

NFC. Usage of Optional. Rebased onto committed D93709 .

[simoll]

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.

[efriedma]

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

[efriedma]

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]

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]

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]

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]

• Cleanup
• Rebased

[craig.topper]

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]

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]

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]

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]

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