This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][RFC] Importing insert/extract vector element patterns
Needs ReviewPublic

Authored by Petar.Avramovic on Nov 29 2019, 7:41 AM.

Details

Summary

I am trying to import insert/extract vector element isel patterns. The important first question is what to GINodeEquiv ?
Here's prototype that is able to selectimpl some basic insert/extract vector patterns for MIPS:

  • Generic vector_insert def : GINodeEquiv<G_INSERT_VECTOR_ELT, vector_insert>; is used for both same and different "vector scalar" and "inserted/extracted elt scalar" size
  • Generic vector_extract def : GINodeEquiv<G_EXTRACT_VECTOR_ELT, vector_extract>; is used only for floating point vector extract (same "vector scalar" and "inserted/extracted elt scalar" size)
  • MipsVExtractSExt and MipsVExtractZExt are used for both same and different "vector scalar" and "inserted/extracted elt scalar" size, although same element size for MipsVExtractZExt gets selected like MipsVExtractSExt (the "same 32-bit element size" on 32-bit machine is extending when executed on 64-bit machine, and this type of extending has to be signextending).

@rovka, could neon use generic G_SEXT_EXTRACT_VECTOR_ELT and G_ZEXT_EXTRACT_VECTOR_ELT since it has similar target specific extending vector extract isd nodes? Probably something like this:
def : GINodeEquiv<G_SEXT_EXTRACT_VECTOR_ELT, ARMvgetlanes>;
def : GINodeEquiv<G_ZEXT_EXTRACT_VECTOR_ELT, ARMvgetlaneu>;
They cant't be imported yet because they have SDNodeXForm (like DSubReg_i8_reg) for subindex.

I think that MIPS has to GINodeEquiv globalisel opcodes without the:
"vector scalar size" has to be the same as "inserted/extracted elt scalar size"
constraint to match patterns from td files.

Is this a right direction or I need to GINodeEquiv something else ?
There are some size constraints in MachineIRBuilder for G_INSERT_VECTOR_ELT and G_EXTRACT_VECTOR_ELT, G_BUILD_VECTOR_TRUNC also exists, but I don't see what are their GINodeEquiv SDAG-nodes.

Some additional information about the implementation:
Combiner for extract vector is copy of zextload/sextload combiner.
For MIPS imm index for all insert/extract vector element opcodes is iptr, but that is scalar of pointer size, added ScalarOfPtrSizeMatcher.
Importing EXTRACT_SUBREG patterns:
when EXTRACT_SUBREG is in the middle there is a subregister copy to tmp register, added TempSubRegRenderer.
Figuring out classes for constraints of subreg copy operands.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
arsenm added inline comments.Dec 1 2019, 10:49 PM
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
583

I think this should assert isPointer. Using 0 address space if it's not a pointer is nonsensical

llvm/include/llvm/Target/GenericOpcodes.td
1027–1028

Adding these is a separate patch

llvm/include/llvm/Target/GlobalISel/SelectionDAGCompat.td
122

I think there should be no compatibility for vector_insert/vector_extract see this comment:
vector_extract/vector_insert are deprecated. extractelt/insertelt
are preferred.

Really we should migrate all the uses away from the old relaxed nodes

Attempt to switch to insertelt and extractelt for mips. Simplified a few lines.

Petar.Avramovic marked 4 inline comments as done.Dec 2 2019, 6:06 AM
Petar.Avramovic added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h
583

vector_insert has index operand of pointer type, but G_INSERT_VECTOR_ELT has most likely scalar index (G_CONSTANT or value in vreg).
That line was most probably wrong. I wanted to get pointer size but it is unlikely to find LLT pointer type in G_INSERT_VECTOR_ELT and ask for address space.

llvm/include/llvm/Target/GenericOpcodes.td
1016

If we def : GINodeEquiv<G_INSERT_VECTOR_ELT, insertelt>;
the we also need some dependency between type0 and type1 here (type0 = vNtype1)

llvm/include/llvm/Target/GlobalISel/SelectionDAGCompat.td
101

There were no regression tests for this.

llvm/lib/Target/Mips/MipsMSAInstrInfo.td
2279

It is fine to migrate from vector_insert to insertelt/extractelt as long as there is register class that can hold scalar size of vector. However there are no register classes of sizes 8 and 16 for mips (I think it is the same situation on arm).

Then maybe introduce another generic opcode G_INSERT_VECTOR_ELT_TRUNC. It appears when we widen scalar of element type only for G_INSERT_VECTOR_ELT. It shares remaining legalization rules with G_INSERT_VECTOR_ELT.
def : GINodeEquiv<G_INSERT_VECTOR_ELT_TRUNC, vector_insert>;

Rebase and ping.

Rebase and update.

Enforce same type of vector scalar and elt scalar in MachineVerifier for G_INSERT_VECTOR_ELT and G_EXTRACT_VECTOR_ELT.
Introducing two new similar opcodes without type constraint G_INSERT_VECTOR_ELT_TRUNC and G_ANYEXT_EXTRACT_VECTOR_ELT.
These opcodes have no corresponding llvm-IR opcodes and targets that have extending vector extract and truncating vector insert can generate them from G_INSERT_VECTOR_ELT and G_EXTRACT_VECTOR_ELT using minVectorInsOrExtScalar (minScalar respects MachineVerifier constraints for G_INSERT_VECTOR_ELT and G_EXTRACT_VECTOR_ELT).
Mips has only sign and zero extending vector extracts thus G_ANYEXT_EXTRACT_VECTOR_ELT is not selectable and is replaced with default extending vector extract instruction G_SEXT_EXTRACT_VECTOR_ELT in custom legalize.
Updated tests.

arsenm added inline comments.Jan 20 2020, 2:07 PM
llvm/include/llvm/Support/TargetOpcodes.def
564

Can you split out the new opcode handling into a separate patch?

arsenm added inline comments.Jan 20 2020, 2:13 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h
60–65 ↗(On Diff #239141)

Why is a new legalize action needed? Why can't this just specify the existing action on the scalar operand?

llvm/utils/TableGen/CodeGenRegisters.cpp
1011–1015

I have a different workaround for this in D72771

1057–1078

This all looks like more workaround for D72771. I think the mips register definitions should just be fixed to not have aliases

llvm/utils/TableGen/GlobalISelEmitter.cpp
2568

I reproduced approximately the same thing in D72788

Petar.Avramovic marked 4 inline comments as done.Jan 21 2020, 2:54 AM
Petar.Avramovic added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h
60–65 ↗(On Diff #239141)

I thought that targets might need different widen scalar of elt operand in vector insert extract. Now that I think of it custom should do just fine since it is action of only 2 opcodes. Did you have other existing action in mind ?

llvm/include/llvm/Support/TargetOpcodes.def
564

Sure, will wait for your patches in emitter first.

llvm/utils/TableGen/CodeGenRegisters.cpp
1011–1015

Looks great.

llvm/utils/TableGen/GlobalISelEmitter.cpp
2568

I see, then iptr immediate index is all that is left in this patch concerning emitter.

arsenm added inline comments.Feb 13 2020, 4:52 PM
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
84–91

This belongs in a separate patch

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
599–605

This is too ugly and needs to be broken up

656–660

Ditto

665

use_nodbg?

676

No reference

llvm/lib/Target/Mips/MipsLegalizerInfo.cpp
104–107

return the logical expression

113–114

Ditto

121–124

Ditto x3 more