[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()) {
                 // We don't handle VA types which are not exactly twice the
                 // size, but can easily be done in future.
                 if (NewLLT.getNumElements() != OldLLT.getNumElements() * 2) {
                   LLVM_DEBUG(dbgs() << "Outgoing vector ret has too many elts");
                   return false;
                 }
                 auto Undef = MIRBuilder.buildUndef({OldLLT});
                 CurVReg =
                     MIRBuilder.buildMergeLikeInstr({NewLLT}, {CurVReg, Undef})
                         .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) {
               // We need to pad a <1 x S> type to <2 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.
               auto Undef = MIRBuilder.buildUndef({OldLLT});
               CurVReg =
                   MIRBuilder

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

   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 RegisterBank &RBDst = *RBI.getRegBank(DstReg, MRI, TRI);
-    if (RBDst.getID() != AArch64::GPRRegBankID) {
+    // The integer vector can be extend
+    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);
       I.getOperand(1).setReg(ExtSrc);
     }
     return selectCopy(I, TII, MRI, TRI, RBI);

 
         if (!RBI.constrainGenericRegister(DefReg, AArch64::GPR64RegClass,
                                           MRI)) {
           LLVM_DEBUG(dbgs() << "Failed to constrain G_ZEXT destination\n");
           return false;
         }
 
         if (!RBI.constrainGenericRegister(SrcReg, AArch64::GPR32RegClass,
                                           MRI)) {

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-ISEL-LABEL: extend_v2i16:
+; CHECK-ISEL:       // %bb.0:
+; CHECK-ISEL-NEXT:    movi v0.2d, #0000000000000000
+; CHECK-ISEL-NEXT:    ret
+;
+; CHECK-GLOBAL-LABEL: extend_v2i16:
+; CHECK-GLOBAL:       // %bb.0:
+; CHECK-GLOBAL-NEXT:    adrp x8, .LCPI42_0
+; CHECK-GLOBAL-NEXT:    ldr s0, [x8, :lo12:.LCPI42_0]
+; CHECK-GLOBAL-NEXT:    fmov w0, s0
+; CHECK-GLOBAL-NEXT:    fmov d0, x0
+; CHECK-GLOBAL-NEXT:    ret
+  ret <2 x i16> <i16 0, i16 0>
+}