This is an archive of the discontinued LLVM Phabricator instance.

MVT: Add v3i16/v3f16 vectors
ClosedPublic

Authored by arsenm on Aug 2 2019, 5:10 PM.

Details

Summary

AMDGPU has some buffer intrinsics which theoretically could use
this. Some of the generated tables include the 3 and 4 element vector
versions of these rounded to 64-bits, which is ambiguous. Add these to
help the table disambiguate these.

Assertion change is for the path odd sized vectors now take for R600.
v3i16 is widened to v4i16, which then needs to be promoted to v4i32.

Diff Detail

Event Timeline

arsenm created this revision.Aug 2 2019, 5:10 PM
craig.topper added inline comments.Aug 2 2019, 5:31 PM
test/CodeGen/X86/promote-vec3.ll
1

Can you explain why the X86 tests changed?

utils/TableGen/IntrinsicEmitter.cpp
224

Is this needed for this patch or a future patch that adds an intrinsic?

arsenm marked 2 inline comments as done.Aug 2 2019, 8:45 PM
arsenm added inline comments.
test/CodeGen/X86/promote-vec3.ll
1

That it’s now an MVT places it into the legalization action table, and now defaults to widen as per getPreferredVectorAction. As an EVT, it would default to Expand. computeRegisterProperties scans over all MVT values, so the existence of a new MVT can change the legalization default regardless of the legal register types

utils/TableGen/IntrinsicEmitter.cpp
224

I hit this I think due to a bug in updating the ValueTypes.td enum values and can probably be dropped. The only intrinsics that would use this use a mangled parameter

craig.topper added inline comments.Aug 2 2019, 9:29 PM
include/llvm/CodeGen/ValueTypes.td
126

Size should be 48?

include/llvm/Support/MachineValueType.h
635

Aren't these generally sorted by integer vs fp then element size?

craig.topper added inline comments.Aug 2 2019, 9:46 PM
test/CodeGen/X86/promote-vec3.ll
9

We seem to be changing how we're returning v3i16 here. Previously we returned it as 3 scalars. Now we're returning it as xmm0.

I think there's a bug in computeRegisterProperties for TypeWidenVector. For non-power2 vectors we need to go the next largest power of 2 vector. Currently its goes through the loop looking for a legal vector with more elements. For X86 it takes v3i16 all the way to v8i16. It should only take it to v4i16.

arsenm added a comment.Aug 4 2019, 9:36 AM

I think there's a bug in computeRegisterProperties for TypeWidenVector. For non-power2 vectors we need to go the next largest power of 2 vector. Currently its goes through the loop looking for a legal vector with more elements. For X86 it takes v3i16 all the way to v8i16. It should only take it to v4i16.

Why should it go to v4i16? I don't see that added as a legal type in x86? I only see v8i16, v16i16, and v32i16 as possible legal vectors

arsenm marked an inline comment as done.Aug 4 2019, 9:38 AM
arsenm added inline comments.
test/CodeGen/X86/promote-vec3.ll
9

I think the old behavior is broken. If you think maintaining ABI compatibility with this brokenness is important, getRegisterTypeForCallingConv can be used to workaround it

I think there's a bug in computeRegisterProperties for TypeWidenVector. For non-power2 vectors we need to go the next largest power of 2 vector. Currently its goes through the loop looking for a legal vector with more elements. For X86 it takes v3i16 all the way to v8i16. It should only take it to v4i16.

Why should it go to v4i16? I don't see that added as a legal type in x86? I only see v8i16, v16i16, and v32i16 as possible legal vectors

I’m only saying that because that’s what EVT did. It doesn’t seem like a good design to change behavior of other targets when one target needs a new MVT.

I think there's a bug in computeRegisterProperties for TypeWidenVector. For non-power2 vectors we need to go the next largest power of 2 vector. Currently its goes through the loop looking for a legal vector with more elements. For X86 it takes v3i16 all the way to v8i16. It should only take it to v4i16.

Why should it go to v4i16? I don't see that added as a legal type in x86? I only see v8i16, v16i16, and v32i16 as possible legal vectors

I’m only saying that because that’s what EVT did. It doesn’t seem like a good design to change behavior of other targets when one target needs a new MVT.

The MVT vs. EVT distinction is a broken design, which thankfully GlobalISel fixes. The question going forward is whether maintaining compatibility with the traditionally broken non-power-of-2 support is something worth maintaining

I'm fine with breaking ABI for the X86 tests for illegal types. This patch probably needs to be rebased after r367901 which changed the X86 type legalization strategy for illegal vector types.

arsenm updated this revision to Diff 214690.Aug 12 2019, 1:00 PM

Re-rebase, resort switch cases

craig.topper added inline comments.Aug 12 2019, 10:13 PM
include/llvm/CodeGen/ValueTypes.td
126

Size is still wrong here.

arsenm updated this revision to Diff 214841.Aug 13 2019, 8:05 AM

Fix size

This revision is now accepted and ready to land.Aug 15 2019, 11:32 AM
arsenm closed this revision.Aug 15 2019, 11:57 AM

r369038

uabelho added inline comments.
utils/TableGen/IntrinsicEmitter.cpp
224

But if it's kept (which it is in r369038) it needs to be added to the enum in Function.cpp too according to a comment above:

// NOTE: This must be kept in synch with the copy in lib/IR/Function.cpp!

Right now the two enums are not in sync.

bjope added a subscriber: bjope.Aug 16 2019, 11:27 PM