[PATCH] D70858: [GlobalISel][RFC] Importing insert/extract vector element patterns

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic marked 4 inline comments as done.
Petar.Avramovic added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h:583
+        MachineFunction *MF = State.MIs[InsnID]->getParent()->getParent();
+        const unsigned AddrSpace = Ty.isPointer() ? Ty.getAddressSpace() : 0;
+        SizeInBits = MF->getDataLayout().getPointerSizeInBits(AddrSpace);
----------------
arsenm wrote:
> I think this should assert isPointer. Using 0 address space if it's not a pointer is nonsensical
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.


================
Comment at: llvm/include/llvm/Target/GenericOpcodes.td:1016
   let OutOperandList = (outs type0:$dst);
   let InOperandList = (ins type0:$src, type1:$elt, type2:$idx);
   let hasSideEffects = 0;
----------------
If we def : GINodeEquiv<G_INSERT_VECTOR_ELT, insertelt>;
the we also need some dependency between type0 and type1 here (type0 = vNtype1)


================
Comment at: llvm/include/llvm/Target/GlobalISel/SelectionDAGCompat.td:101
 def : GINodeEquiv<G_CTPOP, ctpop>;
-def : GINodeEquiv<G_EXTRACT_VECTOR_ELT, vector_extract>;
 def : GINodeEquiv<G_CONCAT_VECTORS, concat_vectors>;
----------------
There were no regression tests for this.


================
Comment at: llvm/lib/Target/Mips/MipsMSAInstrInfo.td:2279
 class INSERT_H_VIDX_PSEUDO_DESC :
     MSA_INSERT_VIDX_PSEUDO_BASE<vector_insert, v8i16, MSA128HOpnd, GPR32Opnd, GPR32Opnd>;
 class INSERT_W_VIDX_PSEUDO_DESC :
----------------
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>;





CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70858/new/

https://reviews.llvm.org/D70858