[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 ↗

(On Diff #533117)

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 ↗

(On Diff #533117)

It is about integer and floats?

[Allen]

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

420 ↗

(On Diff #533117)

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 ↗

(On Diff #533117)

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 ↗

(On Diff #538557)

Might be good to update this comment

Scalar G_ANYEXT on bank...

5608 ↗

(On Diff #538557)

Comment?

5615 ↗

(On Diff #538557)

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

5634 ↗

(On Diff #538557)

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 ↗

(On Diff #538557)

Done, thanks

5634 ↗

(On Diff #538557)

Thanks, apply your comment

3230 ↗

(On Diff #533117)

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

[paquette]

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

5621 ↗

(On Diff #538896)

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 ↗

(On Diff #538896)

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 ↗

(On Diff #538896)

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 ↗

(On Diff #538896)

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

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

thanks for your detail suggestion, apply your comment.

[tschuett]

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

944

Nice! s/Ty/VectorSize/ .

[aemerson]

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

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

945

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

953

LLT::isScalable()

[Allen]

address comments

[Allen]

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

944

Done, thanks

945

Done, thanks

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

LegalizeRuleSet &minScalarOrEltIf(LegalityPredicate Predicate,
     using namespace LegalityPredicates;
     using namespace LegalizeMutations;
     return actionIf(LegalizeAction::WidenScalar,
                     all(Predicate, scalarOrEltNarrowerThan(
                                        TypeIdx, Ty.getScalarSizeInBits())),
                     changeElementTo(typeIdx(TypeIdx), Ty));
   }
 
+  /// Ensure the vector size is at least as wide as VectorSize by promoting the
+  /// element.
+  LegalizeRuleSet &widenVectorEltsToVectorMinSize(unsigned TypeIdx,
+                                                  unsigned VectorSize) {
+    using namespace LegalityPredicates;
+    using namespace LegalizeMutations;
+    return actionIf(
+        LegalizeAction::WidenScalar,
+        [=](const LegalityQuery &Query) {
+          const LLT VecTy = Query.Types[TypeIdx];
+          return VecTy.isVector() && !VecTy.isScalable() &&
+                 VecTy.getSizeInBits() < VectorSize;
+        },
+        [=](const LegalityQuery &Query) {
+          const LLT VecTy = Query.Types[TypeIdx];
+          unsigned NumElts = VecTy.getNumElements();
+          unsigned MinSize = VectorSize / NumElts;
+          LLT NewTy = LLT::fixed_vector(NumElts, LLT::scalar(MinSize));
+          return std::make_pair(TypeIdx, NewTy);
+        });
+  }
+
   /// Ensure the scalar is at least as wide as Ty.
   LegalizeRuleSet &minScalar(unsigned TypeIdx, const LLT Ty) {
     using namespace LegalityPredicates;
     using namespace LegalizeMutations;
     return actionIf(LegalizeAction::WidenScalar,
                     scalarNarrowerThan(TypeIdx, Ty.getSizeInBits()),
                     changeTo(typeIdx(TypeIdx), Ty));
   }

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

AArch64LegalizerInfo::AArch64LegalizerInfo(const AArch64Subtarget &ST)
   const LLT s8 = LLT::scalar(8);
   const LLT s16 = LLT::scalar(16);
   const LLT s32 = LLT::scalar(32);
   const LLT s64 = LLT::scalar(64);
   const LLT s128 = LLT::scalar(128);
   const LLT v16s8 = LLT::fixed_vector(16, 8);
   const LLT v8s8 = LLT::fixed_vector(8, 8);
   const LLT v4s8 = LLT::fixed_vector(4, 8);
+  const LLT v2s8 = LLT::fixed_vector(2, 8);
   const LLT v8s16 = LLT::fixed_vector(8, 16);
   const LLT v4s16 = LLT::fixed_vector(4, 16);
   const LLT v2s16 = LLT::fixed_vector(2, 16);
   const LLT v2s32 = LLT::fixed_vector(2, 32);
   const LLT v4s32 = LLT::fixed_vector(4, 32);
   const LLT v2s64 = LLT::fixed_vector(2, 64);
   const LLT v2p0 = LLT::fixed_vector(2, p0);
 
getActionDefinitionsBuilder(G_BUILD_VECTOR)
                  {v4s16, s16},
                  {v8s16, s16},
                  {v2s32, s32},
                  {v4s32, s32},
                  {v2p0, p0},
                  {v2s64, s64}})
       .clampNumElements(0, v4s32, v4s32)
       .clampNumElements(0, v2s64, v2s64)
       .minScalarOrElt(0, s8)
-      .minScalarOrEltIf(
-          [=](const LegalityQuery &Query) { return Query.Types[0] == v2s16; },
-          0, s32)
+      .widenVectorEltsToVectorMinSize(0, 64)
       .minScalarSameAs(1, 0);
 
   getActionDefinitionsBuilder(G_BUILD_VECTOR_TRUNC).lower();
 
   getActionDefinitionsBuilder(G_CTLZ)
       .legalForCartesianProduct(
           {s32, s64, v8s8, v16s8, v4s16, v8s16, v2s32, v4s32})
       .scalarize(1)
       .widenScalarToNextPow2(1, /*Min=*/32)
       .clampScalar(1, s32, s64)
       .scalarSameSizeAs(0, 1);
   getActionDefinitionsBuilder(G_CTLZ_ZERO_UNDEF).lower();
 

llvm/test/CodeGen/AArch64/GlobalISel/legalize-build-vector.mir

bb.0:
     ; CHECK-NEXT: RET_ReallyLR
     %0:_(s16) = COPY $h0
     %1:_(s16) = COPY $h1
     %2:_(<2 x s16>) = G_BUILD_VECTOR %0(s16), %1(s16)
     %ext:_(<2 x s32>) = G_ANYEXT %2(<2 x s16>)
     $d0 = COPY %ext(<2 x s32>)
     RET_ReallyLR
 ...
+
+---
+name:            widen_v2s8
+body: |
+  bb.0:
+    ; CHECK-LABEL: name: widen_v2s8
+    ; CHECK: [[DEF:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY [[DEF]](s32)
+    ; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<2 x s32>) = G_BUILD_VECTOR [[COPY]](s32), [[DEF]](s32)
+    ; CHECK-NEXT: $d0 = COPY [[BUILD_VECTOR]](<2 x s32>)
+    ; CHECK-NEXT: RET_ReallyLR
+    %0:_(s8) = G_IMPLICIT_DEF
+    %1:_(s8) = G_IMPLICIT_DEF
+    %2:_(<2 x s8>) = G_BUILD_VECTOR %0(s8), %1(s8)
+    %ext:_(<2 x s32>) = G_ANYEXT %2(<2 x s8>)
+    $d0 = COPY %ext(<2 x s32>)
+    RET_ReallyLR
+...
+
+---
+name:            widen_v4s8
+body: |
+  bb.0:
+    ; CHECK-LABEL: name: widen_v4s8
+    ; CHECK: [[DEF:%[0-9]+]]:_(s16) = G_IMPLICIT_DEF
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s16) = COPY [[DEF]](s16)
+    ; CHECK-NEXT: [[COPY1:%[0-9]+]]:_(s16) = COPY [[DEF]](s16)
+    ; CHECK-NEXT: [[COPY2:%[0-9]+]]:_(s16) = COPY [[DEF]](s16)
+    ; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<4 x s16>) = G_BUILD_VECTOR [[COPY]](s16), [[COPY1]](s16), [[COPY2]](s16), [[DEF]](s16)
+    ; CHECK-NEXT: $d0 = COPY [[BUILD_VECTOR]](<4 x s16>)
+    ; CHECK-NEXT: RET_ReallyLR
+    %0:_(s8) = G_IMPLICIT_DEF
+    %1:_(s8) = G_IMPLICIT_DEF
+    %2:_(s8) = G_IMPLICIT_DEF
+    %3:_(s8) = G_IMPLICIT_DEF
+    %4:_(<4 x s8>) = G_BUILD_VECTOR %0(s8), %1(s8), %2(s8), %3(s8)
+    %ext:_(<4 x s16>) = G_ANYEXT %4(<4 x s8>)
+    $d0 = COPY %ext(<4 x s16>)
+    RET_ReallyLR
+...

llvm/test/CodeGen/AArch64/GlobalISel/legalize-itofp.mir

 ---
 name:            test_uitofp_v2s64_v2i1
 body: |
   bb.0:
     liveins: $q0
     ; CHECK-LABEL: name: test_uitofp_v2s64_v2i1
     ; CHECK: liveins: $q0
     ; CHECK-NEXT: {{  $}}
-    ; CHECK-NEXT: [[DEF:%[0-9]+]]:_(s8) = G_IMPLICIT_DEF
-    ; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<2 x s8>) = G_BUILD_VECTOR [[DEF]](s8), [[DEF]](s8)
+    ; CHECK-NEXT: [[DEF:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY [[DEF]](s32)
+    ; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<2 x s32>) = G_BUILD_VECTOR [[COPY]](s32), [[DEF]](s32)
     ; CHECK-NEXT: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 1
     ; CHECK-NEXT: [[BUILD_VECTOR1:%[0-9]+]]:_(<2 x s64>) = G_BUILD_VECTOR [[C]](s64), [[C]](s64)
-    ; CHECK-NEXT: [[ANYEXT:%[0-9]+]]:_(<2 x s64>) = G_ANYEXT [[BUILD_VECTOR]](<2 x s8>)
+    ; CHECK-NEXT: [[ANYEXT:%[0-9]+]]:_(<2 x s64>) = G_ANYEXT [[BUILD_VECTOR]](<2 x s32>)
     ; CHECK-NEXT: [[AND:%[0-9]+]]:_(<2 x s64>) = G_AND [[ANYEXT]], [[BUILD_VECTOR1]]
     ; CHECK-NEXT: [[UITOFP:%[0-9]+]]:_(<2 x s64>) = G_UITOFP [[AND]](<2 x s64>)
     ; CHECK-NEXT: $q0 = COPY [[UITOFP]](<2 x s64>)
     %0:_(<2 x s1>) = G_IMPLICIT_DEF
     %1:_(<2 x s64>) = G_UITOFP %0
     $q0 = COPY %1
 ...
 
 ---
 name:            test_sitofp_v2s64_v2i1
 body: |
   bb.0:
     liveins: $q0
     ; CHECK-LABEL: name: test_sitofp_v2s64_v2i1
     ; CHECK: liveins: $q0
     ; CHECK-NEXT: {{  $}}
-    ; CHECK-NEXT: [[DEF:%[0-9]+]]:_(s8) = G_IMPLICIT_DEF
-    ; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<2 x s8>) = G_BUILD_VECTOR [[DEF]](s8), [[DEF]](s8)
-    ; CHECK-NEXT: [[ANYEXT:%[0-9]+]]:_(<2 x s64>) = G_ANYEXT [[BUILD_VECTOR]](<2 x s8>)
+    ; CHECK-NEXT: [[DEF:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY [[DEF]](s32)
+    ; CHECK-NEXT: [[BUILD_VECTOR:%[0-9]+]]:_(<2 x s32>) = G_BUILD_VECTOR [[COPY]](s32), [[DEF]](s32)
+    ; CHECK-NEXT: [[ANYEXT:%[0-9]+]]:_(<2 x s64>) = G_ANYEXT [[BUILD_VECTOR]](<2 x s32>)
     ; CHECK-NEXT: [[SEXT_INREG:%[0-9]+]]:_(<2 x s64>) = G_SEXT_INREG [[ANYEXT]], 1
     ; CHECK-NEXT: [[SITOFP:%[0-9]+]]:_(<2 x s64>) = G_SITOFP [[SEXT_INREG]](<2 x s64>)
     ; CHECK-NEXT: $q0 = COPY [[SITOFP]](<2 x s64>)
     %0:_(<2 x s1>) = G_IMPLICIT_DEF
     %1:_(<2 x s64>) = G_SITOFP %0
     $q0 = COPY %1
 ...