[AArch64][GlobalISel] Legalize <2 x s8> and <4 x s8> for G_BUILD_VECTOR

[Allen]

Summary

Refer to commit ccffc27, the remaining types <2 x s8> and <4 x s8> should
also be promoted to <2 x s32> and <4 x s16>.

Fixes https://github.com/llvm/llvm-project/issues/58274

[dmgreen]

This test, whilst technically correct, doesn't look right. I don't think it can just generate a SUBREG_TO_REG in the same way it does for integer. Can you change the test to return <i16 0, i16 1>, and make sure the returned value would be the same as SDAG.

[Allen]

update test case according comment

[tschuett]

llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp

420

Could you instead query OldLLT and NewLLT whether they are isScalar()? Looks odd to query MVTs in GISel.

[tschuett]

llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp

420

It is about integer and floats?

[Allen]

llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp

420

Yes, It is about integer and floats (not about the scalar and vector).

[dmgreen]

If I'm reading the test correctly, then SDAG version is returning (as bytes) 0,0,0,0,1,0,0,0. i.e the v2i16 is promoted to a v2i32. The GlobalISel version is returning 0,0,1,0,0,0,0,0, as the v2i16 is widened to a v4i16.

I don't think we can have a difference in the calling convention between the two, it would mean they are ABI incompatible. Either they both need to change to widen (which looks like a lot of work, including an ABI break for SDAG), or the GISel code need return values in the same way as SDAG does.

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

3230

I don't think this will handle vector extends correctly.

[Allen]

In D153394#4452589, @dmgreen wrote:

If I'm reading the test correctly, then SDAG version is returning (as bytes) 0,0,0,0,1,0,0,0. i.e the v2i16 is promoted to a v2i32. The GlobalISel version is returning 0,0,1,0,0,0,0,0, as the v2i16 is widened to a v4i16.

I don't think we can have a difference in the calling convention between the two, it would mean they are ABI incompatible. Either they both need to change to widen (which looks like a lot of work, including an ABI break for SDAG), or the GISel code need return values in the same way as SDAG does.

Thanks, I find it seems to be an optimization in SDAG version. So GISel version needs to do something like
SelectionDAG::FoldConstantArithmetic and TryToFoldExtendOfConstant for const vector constant ?

[tschuett]

In D153394#4482586, @Allen wrote:

In D153394#4452589, @dmgreen wrote:

If I'm reading the test correctly, then SDAG version is returning (as bytes) 0,0,0,0,1,0,0,0. i.e the v2i16 is promoted to a v2i32. The GlobalISel version is returning 0,0,1,0,0,0,0,0, as the v2i16 is widened to a v4i16.

I don't think we can have a difference in the calling convention between the two, it would mean they are ABI incompatible. Either they both need to change to widen (which looks like a lot of work, including an ABI break for SDAG), or the GISel code need return values in the same way as SDAG does.

Thanks, I find it seems to be an optimization in SDAG version. So GISel version needs to do something like
SelectionDAG::FoldConstantArithmetic and TryToFoldExtendOfConstant for const vector constant ?

I believe that you are mixing optimizations with ABI. The SDAG ABI result has to win indepent of the inefficient GISel code.

[Allen]

Add tryToFoldExtendOfVectorConstant to adjust the ABI

[paquette]

Can you add a MIR testcase?

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

3212

Might be good to update this comment

Scalar G_ANYEXT on bank...

5608

Comment?

5615

Could use a comment explaining that you're looking for G_BUILD_VECTORs with all constant source operands?

5634

emitConstantVector should return a nullptr on failure, right?

So then we can save one LOC:

// Try to replace ExtI with a constant vector.
MachineInstr *MaybeCVec =
      emitConstantVector(ExtI.getOperand(0).getReg(), CV, MIB, MRI);
if (MaybeCVec)
  ExtI.eraseFromParent();
return MaybeCVec;

[Allen]

address comment and add a new mir test llvm/test/CodeGen/AArch64/GlobalISel/select-neon-vector-const.mir

[Allen]

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

3212

Done, thanks

3230

Thanks, add a new function tryToFoldExtendOfVectorConstant to handle this case.

5634

Thanks, apply your comment

[paquette]

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

5621

can you add a testcase that shows what happens when one of the G_BUILD_VECTOR sources is a constant?

e.g

%x = G_BUILD_VECTOR %constant, %not_a_constant

5638

can you add a test to the MIR testcase that shows what happens when emitConstantVector returns nullptr?

llvm/test/CodeGen/AArch64/GlobalISel/select-neon-vector-const.mir

3 ↗

(On Diff #538896)

you can delete the IR portion

17 ↗

(On Diff #538896)

you can delete the registers section

22 ↗

(On Diff #538896)

if you delete the IR section, then this will need to be renamed so that it does not reference the IR

[Allen]

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

5621

it will crash before regbankselect, so it seems another independent issue, https://gcc.godbolt.org/z/e3Wq9Mdar, so I fire a issue https://github.com/llvm/llvm-project/issues/63826

5638

I add this function refer to above function tryOptConstantBuildVec, and I don't have the idea how to construct a constant that meets the scenario where returns null , do you have any suggestion ?

[aemerson]

I haven't forgotten about this patch, I just need to find some time to look into this issue. In the mean time: as a rule, anything that's produced by the translator must be correct by itself, it can't rely on any optimizations to run in order to generate the correct code. It's easiest to check this by writing an additional MIR test for the irtranslator change. It should be clear from that test whether or not the change is correct. The optimizations, if needed and appropriate, can be a separate patch.

[Allen]

I'm glad to know about your plans，thank you for your time.

[aemerson]

So I had a look at this particular test case, and from what I can tell there's nothing we're doing wrong in the IRTranslator. lowerReturn() is correctly widening the <2 x i16> return type to <2 x i32>. This leaves the following MIR:

body:             |
  bb.1 (%ir-block.0):
    %1:_(s16) = G_CONSTANT i16 0
    %2:_(s16) = G_CONSTANT i16 1
    %0:_(<2 x s16>) = G_BUILD_VECTOR %1(s16), %2(s16)
    %3:_(<2 x s32>) = G_ANYEXT %0(<2 x s16>)
    $d0 = COPY %3(<2 x s32>)
    RET_ReallyLR implicit $d0

I think the problem is that G_BUILD_VECTOR of <2 x i16> needs to be widened to a supported type. Since this was a trivial change, I went ahead and did it in ccffc2705054

Thanks anyway for taking a look at it!

[Allen]

Thank you for your guidance

[aemerson]

llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp

723–729 ↗

(On Diff #542866)

Now that we're going to do this multiple times, I think it's worth factoring out the logic to make it easier to re-use. The underlying logic is I believe: "vectors must be at least 64 bits wide", right? I think we could make this easier by adding a new action/predicates in LegalizerInfo.h, so that we could do something like:

.promoteVectoreEltsToVectorMinSize(0, 64)

I think there are other places in this file that could also use this new action to simplify the code.

P.S. please attach more context to your diffs (-U9999 works).

[Allen]

address comment, add new function promoteVectorEltsToVectorMinSize

[Allen]

llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp

723–729 ↗

(On Diff #542866)

thanks for your detail suggestion, apply your comment.

[tschuett]

llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h

944 ↗

(On Diff #543173)

Nice! s/Ty/VectorSize/ .

[aemerson]

LGTM with a few nits. Thanks for working on this!

llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h

945 ↗

(On Diff #543173)

Sorry, the name I suggested didn't fit with the rest of the naming scheme. I think widenVectorEltsToVectorMinSize is better.

953 ↗

(On Diff #543173)

LLT::isScalable()

[Allen]

address comments

[Allen]

llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h

944 ↗

(On Diff #543173)

Done, thanks

945 ↗

(On Diff #543173)

Done, thanks

llvm/lib/Target/AArch64/GISel/AArch64CallLowering.cpp

         if (EVT(NewVT) != SplitEVTs[i]) {
           unsigned ExtendOp = TargetOpcode::G_ANYEXT;
           if (F.getAttributes().hasRetAttr(Attribute::SExt))
             ExtendOp = TargetOpcode::G_SEXT;
           else if (F.getAttributes().hasRetAttr(Attribute::ZExt))
             ExtendOp = TargetOpcode::G_ZEXT;
 
           LLT NewLLT(NewVT);
-          LLT OldLLT(MVT::getVT(CurArgInfo.Ty));
+          MVT OldVT = MVT::getVT(CurArgInfo.Ty);
+          LLT OldLLT(OldVT);
           CurArgInfo.Ty = EVT(NewVT).getTypeForEVT(Ctx);
           // Instead of an extend, we might have a vector type which needs
           // padding with more elements, e.g. <2 x half> -> <4 x half>.
           if (NewVT.isVector()) {
             if (OldLLT.isVector()) {
               if (NewLLT.getNumElements() > OldLLT.getNumElements()) {
 
                 CurVReg =
                     MIRBuilder.buildPadVectorWithUndefElements(NewLLT, CurVReg)
                         .getReg(0);
-              } else {
+              } else if (OldVT.isInteger() && NewVT.isInteger()) {
                 // Just do a vector extend.
                 CurVReg = MIRBuilder.buildInstr(ExtendOp, {NewLLT}, {CurVReg})
                               .getReg(0);
+              } else {
+                LLVM_DEBUG(dbgs() << "Could not handle float type\n");
+                return false;
               }
             } else if (NewLLT.getNumElements() >= 2 &&
                        NewLLT.getNumElements() <= 8) {
               // We need to pad a <1 x S> type to <2/4/8 x S>. Since we don't
               // have <1 x S> vector types in GISel we use a build_vector
               // instead of a vector merge/concat.
               CurVReg =
                   MIRBuilder.buildPadVectorWithUndefElements(NewLLT, CurVReg)

llvm/lib/Target/AArch64/GISel/AArch64InstructionSelector.cpp

   /// otherwise.
   MachineInstr *emitConstantVector(Register Dst, Constant *CV,
                                    MachineIRBuilder &MIRBuilder,
                                    MachineRegisterInfo &MRI);
 
   bool selectInsertElt(MachineInstr &I, MachineRegisterInfo &MRI);
   bool tryOptConstantBuildVec(MachineInstr &MI, LLT DstTy,
                               MachineRegisterInfo &MRI);
+  MachineInstr *tryToFoldExtendOfVectorConstant(MachineInstr &MI, LLT DstTy,
+                              MachineRegisterInfo &MRI);
   /// \returns true if a G_BUILD_VECTOR instruction \p MI can be selected as a
   /// SUBREG_TO_REG.
   bool tryOptBuildVecToSubregToReg(MachineInstr &MI, MachineRegisterInfo &MRI);
   bool selectBuildVector(MachineInstr &I, MachineRegisterInfo &MRI);
   bool selectMergeValues(MachineInstr &I, MachineRegisterInfo &MRI);
   bool selectUnmergeValues(MachineInstr &I, MachineRegisterInfo &MRI);
 
   bool selectShuffleVector(MachineInstr &I, MachineRegisterInfo &MRI);

     return false;
   }
 
   case TargetOpcode::G_ANYEXT: {
     if (selectUSMovFromExtend(I, MRI))
       return true;
 
     const Register DstReg = I.getOperand(0).getReg();
-    const Register SrcReg = I.getOperand(1).getReg();
+    const LLT DstTy = MRI.getType(DstReg);
+    if (tryToFoldExtendOfVectorConstant(I, DstTy, MRI))
+      return true;
 
     const RegisterBank &RBDst = *RBI.getRegBank(DstReg, MRI, TRI);
-    if (RBDst.getID() != AArch64::GPRRegBankID) {
+    const Register SrcReg = I.getOperand(1).getReg();
+    if (RBDst.getID() != AArch64::GPRRegBankID &&
+        !MRI.getType(DstReg).isVector()) {
       LLVM_DEBUG(dbgs() << "G_ANYEXT on bank: " << RBDst
                         << ", expected: GPR\n");
       return false;
     }
 
     const RegisterBank &RBSrc = *RBI.getRegBank(SrcReg, MRI, TRI);
-    if (RBSrc.getID() != AArch64::GPRRegBankID) {
+    if (RBSrc.getID() != AArch64::GPRRegBankID &&
+        !MRI.getType(SrcReg).isVector()) {
       LLVM_DEBUG(dbgs() << "G_ANYEXT on bank: " << RBSrc
                         << ", expected: GPR\n");
       return false;
     }
 
     const unsigned DstSize = MRI.getType(DstReg).getSizeInBits();
 
     if (DstSize == 0) {
       LLVM_DEBUG(dbgs() << "G_ANYEXT operand has no size, not a gvreg?\n");
       return false;
     }
 
     if (DstSize != 64 && DstSize > 32) {
       LLVM_DEBUG(dbgs() << "G_ANYEXT to size: " << DstSize
                         << ", expected: 32 or 64\n");
       return false;
     }
     // At this point G_ANYEXT is just like a plain COPY, but we need
     // to explicitly form the 64-bit value if any.
     if (DstSize > 32) {
       Register ExtSrc = MRI.createVirtualRegister(&AArch64::GPR64allRegClass);
       BuildMI(MBB, I, I.getDebugLoc(), TII.get(AArch64::SUBREG_TO_REG))
           .addDef(ExtSrc)
           .addImm(0)
           .addUse(SrcReg)
           .addImm(AArch64::sub_32);

   }
   Constant *CV = ConstantVector::get(Csts);
   if (!emitConstantVector(I.getOperand(0).getReg(), CV, MIB, MRI))
     return false;
   I.eraseFromParent();
   return true;
 }
 
+// Fold the vector const similar to SelectionDAG::FoldConstantArithmetic.
+MachineInstr *AArch64InstructionSelector::tryToFoldExtendOfVectorConstant(
+    MachineInstr &ExtI, LLT DstTy, MachineRegisterInfo &MRI) {
+  assert(ExtI.getOpcode() == TargetOpcode::G_ANYEXT);
+  const Register SrcReg = ExtI.getOperand(1).getReg();
+  MachineInstr *SrcMI = MRI.getUniqueVRegDef(SrcReg);
+  if (SrcMI->getOpcode() != TargetOpcode::G_BUILD_VECTOR)
+    return nullptr;
+  unsigned DstSize = DstTy.getSizeInBits();
+  assert(DstSize <= 128 && "Unexpected build_vec type!");
+  if (DstSize < 32)
+    return nullptr;
+  // Check if we're building a constant vector, in which case we want to
+  // generate a constant pool load instead of a vector insert sequence.
+  SmallVector<Constant *, 16> Csts;
+  LLT SVT = DstTy.getElementType();
+  unsigned OpTySize = DstTy.getScalarSizeInBits();
+  for (unsigned Idx = 1; Idx < SrcMI->getNumOperands(); ++Idx) {
+    // Try to find G_CONSTANT
+    auto *OpMI = getOpcodeDef(TargetOpcode::G_CONSTANT,
+                              SrcMI->getOperand(Idx).getReg(), MRI);
+    if (!OpMI)
+      return nullptr;
+
+    // Extend the type for each const value element.
+    APInt Immed =
+        APInt(OpTySize, OpMI->getOperand(1).getCImm()->getZExtValue());
+    auto C = MIB.buildConstant(SVT, Immed);
+    MRI.setRegClass(C.getReg(0), &AArch64::GPR64RegClass);
+    Csts.emplace_back(const_cast<ConstantInt *>(C->getOperand(1).getCImm()));
+  }
+  Constant *CV = ConstantVector::get(Csts);
+  MachineInstr *CVec =
+      emitConstantVector(ExtI.getOperand(0).getReg(), CV, MIB, MRI);
+  if (!CVec)
+    return nullptr;
+  ExtI.eraseFromParent();
+  return CVec;
+}
+
 bool AArch64InstructionSelector::tryOptBuildVecToSubregToReg(
     MachineInstr &I, MachineRegisterInfo &MRI) {
   // Given:
   //  %vec = G_BUILD_VECTOR %elt, %undef, %undef, ... %undef
   //
   // Select the G_BUILD_VECTOR as a SUBREG_TO_REG from %elt.
   Register Dst = I.getOperand(0).getReg();
   Register EltReg = I.getOperand(1).getReg();
   LLT EltTy = MRI.getType(EltReg);

llvm/test/CodeGen/AArch64/extract-sext-zext.ll

 ; CHECK-NEXT:    eor x0, x8, x8, lsl #56
 ; CHECK-NEXT:    ret
   %e = extractelement <8 x i8> %x, i64 2
   %s = sext i8 %e to i64
   %t = shl i64 %s, 56
   %u = xor i64 %s, %t
   ret i64 %u
 }
+
+define <2 x i16> @extend_v2i16() {
+; CHECK-LABEL: extend_v2i16:
+; CHECK:       // %bb.0:
+; CHECK-NEXT:    adrp x8, .LCPI42_0
+; CHECK-NEXT:    ldr d0, [x8, :lo12:.LCPI42_0]
+; CHECK-NEXT:    ret
+  ret <2 x i16> <i16 0, i16 1>
+}