GlobalISel: Implement widenScalar for G_INSERT_VECTOR_ELT

[arsenm]

ping

[nhaehnle]

For the vector itself and the inserted element, shouldn't this be using G_ANYEXT instead? Looking at the corresponding test, the G_SHL/G_ASHR on $vgpr0 should be unnecessary based on the original code.

For the element index, shouldn't this be using G_ZEXT or G_ANYEXT? I don't recall whether insertelement is supposed to interpret this argument as a signed or unsigned integer, and couldn't quickly find a reference to it. Not that it usually matters, but I find G_SEXT here surprising as well.

[arsenm]

In D68309#1699422, @nhaehnle wrote:

For the vector itself and the inserted element, shouldn't this be using G_ANYEXT instead? Looking at the corresponding test, the G_SHL/G_ASHR on $vgpr0 should be unnecessary based on the original code.

For the element index, shouldn't this be using G_ZEXT or G_ANYEXT? I don't recall whether insertelement is supposed to interpret this argument as a signed or unsigned integer, and couldn't quickly find a reference to it. Not that it usually matters, but I find G_SEXT here surprising as well.

I was just copying what the extract does. Signed doesn't particularly make sense because a negative index would be an undefined / out of bounds result

[nhaehnle]

Yeah, it doesn't seem very relevant for the element argument, though precisely because of that I'd feel more comfortable with ANYEXT.

For the value argument it really does seem to make a codegen difference. It should be possible to avoid the SHL/ASHR sequence.

[arsenm]

Use G_ANYEXT for vector/value

[nhaehnle]

LGTM

[arsenm]

1a276d1e8c5da57a0c83d1b1d1a02ec0bcdb77d7

lib/CodeGen/GlobalISel/LegalizerHelper.cpp

if (TypeIdx == 0) {
       widenScalarDst(MI, WideTy, 0);
       Observer.changedInstr(MI);
       return Legalized;
     }
 
     if (TypeIdx != 2)
       return UnableToLegalize;
     Observer.changingInstr(MI);
+    // TODO: Probably should be zext
     widenScalarSrc(MI, WideTy, 2, TargetOpcode::G_SEXT);
     Observer.changedInstr(MI);
     return Legalized;
   }
+  case TargetOpcode::G_INSERT_VECTOR_ELT: {
+    if (TypeIdx == 1) {
+      Observer.changingInstr(MI);
+
+      Register VecReg = MI.getOperand(1).getReg();
+      LLT VecTy = MRI.getType(VecReg);
+      LLT WideVecTy = LLT::vector(VecTy.getNumElements(), WideTy);
+
+      widenScalarSrc(MI, WideVecTy, 1, TargetOpcode::G_ANYEXT);
+      widenScalarSrc(MI, WideTy, 2, TargetOpcode::G_ANYEXT);
+      widenScalarDst(MI, WideVecTy, 0);
+      Observer.changedInstr(MI);
+      return Legalized;
+    }
+
+    if (TypeIdx == 2) {
+      Observer.changingInstr(MI);
+      // TODO: Probably should be zext
+      widenScalarSrc(MI, WideTy, 3, TargetOpcode::G_SEXT);
+      Observer.changedInstr(MI);
+    }
+
+    return Legalized;
+  }
   case TargetOpcode::G_FADD:
   case TargetOpcode::G_FMUL:
   case TargetOpcode::G_FSUB:
   case TargetOpcode::G_FMA:
   case TargetOpcode::G_FMAD:
   case TargetOpcode::G_FNEG:
   case TargetOpcode::G_FABS:
   case TargetOpcode::G_FCANONICALIZE:

test/CodeGen/AMDGPU/GlobalISel/legalize-insert-vector-elt.mir

bb.0:
     ; CHECK: [[INSERT:%[0-9]+]]:_(<16 x s64>) = G_INSERT [[DEF]], [[COPY]](s64), 0
     ; CHECK: S_ENDPGM 0, implicit [[INSERT]](<16 x s64>)
     %0:_(s64) = COPY $vgpr0_vgpr1
     %1:_(<16 x s64>) = G_IMPLICIT_DEF
     %2:_(s32) = G_CONSTANT i32 0
     %3:_(<16 x s64>) = G_INSERT_VECTOR_ELT %1, %0, %2
     S_ENDPGM 0, implicit %3
 ...
+
+---
+name: insert_vector_elt_0_v2s32_s8
+
+body: |
+  bb.0:
+    liveins: $vgpr0_vgpr1, $vgpr2
+
+    ; CHECK-LABEL: name: insert_vector_elt_0_v2s32_s8
+    ; CHECK: [[COPY:%[0-9]+]]:_(<2 x s32>) = COPY $vgpr0_vgpr1
+    ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY $vgpr2
+    ; CHECK: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 0
+    ; CHECK: [[COPY2:%[0-9]+]]:_(s32) = COPY [[C]](s32)
+    ; CHECK: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 24
+    ; CHECK: [[SHL:%[0-9]+]]:_(s32) = G_SHL [[COPY2]], [[C1]](s32)
+    ; CHECK: [[ASHR:%[0-9]+]]:_(s32) = G_ASHR [[SHL]], [[C1]](s32)
+    ; CHECK: [[IVEC:%[0-9]+]]:_(<2 x s32>) = G_INSERT_VECTOR_ELT [[COPY]], [[COPY1]](s32), [[ASHR]](s32)
+    ; CHECK: $vgpr0_vgpr1 = COPY [[IVEC]](<2 x s32>)
+    %0:_(<2 x s32>) = COPY $vgpr0_vgpr1
+    %1:_(s32) = COPY $vgpr2
+    %2:_(s8) = G_CONSTANT i8 0
+    %3:_(<2 x s32>) = G_INSERT_VECTOR_ELT %0, %1, %2
+    $vgpr0_vgpr1 = COPY %3
+...
+
+---
+name: insert_vector_elt_0_v2i8_i32
+
+body: |
+  bb.0:
+    liveins: $vgpr0
+
+    ; CHECK-LABEL: name: insert_vector_elt_0_v2i8_i32
+    ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $vgpr0
+    ; CHECK: [[DEF:%[0-9]+]]:_(<2 x s32>) = G_IMPLICIT_DEF
+    ; CHECK: [[COPY1:%[0-9]+]]:_(<2 x s32>) = COPY [[DEF]](<2 x s32>)
+    ; CHECK: [[COPY2:%[0-9]+]]:_(s32) = COPY [[COPY]](s32)
+    ; CHECK: [[INSERT:%[0-9]+]]:_(<2 x s32>) = G_INSERT [[COPY1]], [[COPY2]](s32), 0
+    ; CHECK: [[COPY3:%[0-9]+]]:_(<2 x s32>) = COPY [[INSERT]](<2 x s32>)
+    ; CHECK: $vgpr0_vgpr1 = COPY [[COPY3]](<2 x s32>)
+    %0:_(s32) = COPY $vgpr0
+    %1:_(s8) = G_TRUNC %0
+    %2:_(<2 x s8>) = G_IMPLICIT_DEF
+    %3:_(s32) = G_CONSTANT i32 0
+    %4:_(<2 x s8>) = G_INSERT_VECTOR_ELT %2, %1, %3
+    %5:_(<2 x s32>) = G_ANYEXT %4
+    $vgpr0_vgpr1 = COPY %5
+...