[AArch64] Change default legalization behavior of v1i32 to be widen to v2i32 instead of scalarization

[• HaoLiu]

Summary

Hi Tim and other reviewers,

Recently we found an assertion failure caused by scalarizing node like "v1i32 trunc v1i64". As v1i64 is legal, it will fail to scalarize v1i32.

This patch adds two hooks (like the existing hook shouldSplitVectorType()) to change the scalarization behavior:
(1) avoid scalarize v1i32 by shouldScalarizeVectorType().
(2) If avoid scalarization, the default behavior is changed to be promoted from v1i32 to v1i64, which may introduce extra operations like extend, truncate... Hook preferWidenVectorType() can change it to be widen from v1i32 to v2i32.

v1i8 and v1i16 are similar to v1i32. This patch can also have benefit on reducing FMOV.

BTW, I also think it's better to widen v2i8, v4i8, v2i16 to v8i8, v8i8, v4i16 than the default behavior of promote. But such change will cause several regression test case failures. Some of the test cases result in better code, but some of the test cases result in worse code. If we can improve the worse case, we will also have benefit.

Code review. Please.

Thanks,
-Hao

[• HaoLiu]

ping...

This issue is causing our daily test case failed. Hope it can be fixed ASAP.

Thanks,
-Hao

[t.p.northover]

Hi Hao,

This seems to leave us with a rather ugly mess of callbacks that cover all possible actions, at least on vector types (Scalarize, Split, Widen, Promote). There must be a tidier way to express the logic, both in Targets and in TargetLoweringBase.

If the callback was TargetLowering::getVectorAction instead, we could almost use a switch like this, with fallthroughs (provided v1iN is set to scalarize by default):

switch (getPreferredVectorAction()) {
case TypeWidenVector:
  // ...
  // FALLTHROUGH
case TypePromoteVector:
  // ...
  // FALLTHROUGH
case TypeSplitVector:
  // ...
  break;
case TypeScalarizeVector:
  // ...
  break;
}

I'm not sure I like the asymmetry there, but something must be possible.

The special accommodation later in the function for non-pow2 MVTs also seems completely redundant, at least now: there aren't any.

Cheers.

Tim.

[• HaoLiu]

Hi Tim,

Your suggestion is very good.
I've refactored the patch by using one hook to control all the legalization actions.

Thanks,
-Hao

[t.p.northover]

Hi Hao,

This looks reasonable now; I've just got some minor nits about the implementation of the callbacks. LGTM otherwise.

Cheers.

Tim.

lib/Target/AArch64/AArch64ISelLowering.cpp

7914–7916

Could you change this to "return TargetLoweringBase::getPreferredVectorAction(VT)"? It makes it clearer we're not overriding anything else.

lib/Target/NVPTX/NVPTXISelLowering.cpp

478–481 ↗

(On Diff #11023)

Similarly, I think these might be clearer as "if (VT.getVectorNumElements() != 1 && VT.getScalarType() == MVT::i1)" followed by deferring to the base implementation.

[arsenm]

lib/Target/R600/SIISelLowering.cpp

274–280 ↗

(On Diff #11023)

I think this should be TypeSplitVector for any vector > 128 bits, and TypePromoteInteger for scalar types < 32-bits and no more than 4 elements. I can take care of that later though

[chandlerc]

Sorry if I'm stepping on any toes, but I'm going to commit this (having made Tim's requested changes) so that myself and some others can do stuff on top of this really excellent hook in various other backends.

Thanks so much to Hao for working on this and with Tim for building some really nice infrastructure. It's kind of amazing how many problems in different backends this will likely help us address!

lib/Target/AArch64/AArch64ISelLowering.cpp

7914–7916

FYI, I did this.

lib/Target/NVPTX/NVPTXISelLowering.cpp

478–481 ↗

(On Diff #11023)

And this.

lib/Target/R600/SIISelLowering.cpp

274–280 ↗

(On Diff #11023)

Similarly to other targets, I switched this to delegate to the base class. I'll let you add the custom logic you want for R600 in a follow-up patch.

[• HaoLiu]

Hi Chandler,

I'm fine with your commit. Thanks for your work.

Thanks,
-Hao

[ab]

Hi Hao, Chandler, and Tim,

This seems to be "causing" PR20778 : by widening v1i{32,16,8}, this pushes the original problem (scalarizer asserting because the trunc operand wasn't scalarized) to v1i1.
There was a similar problem for setcc+vselect, cf. r205625.
From what I see, there are a variety of possible solutions:

1- The obvious way is to also widen v1i1. However, widening wouldn't result in any legal type.

2- Promote v1i1, to v1i64. This produces a few (correct) changes (visible in test/CodeGen/AArch64/aarch64-neon-v1i1-setcc.ll).
Also, there is a long-standing FIXME regarding the issue:

// FIXME: Currently the type legalizer can't handle VSELECT having v1i1 as
// condition. If it can legalize "VSELECT v1i1" correctly, no need to combine
// such VSELECT.

Promoting v1i1 somewhat fixes this: we still need combining, but to avoid using v1i64 instead of i64 for vselect+setcc.

3- Change the generic legalizer logic to not assert, when an operation's operands haven't been legalized before (in this case the scalarizer expecting v1i64 to be transformed to i64). This was also done in r205625, but is a pretty ugly hack.

4- Make v1i64 not legal. I'm not sure why it is legal in the first place: why not scalarize to i64? This doesn't seem like a trivial change though. However, making v1i64 and v1f64 non-legal would cleanup a few parts of the AArch64 backend.

Also, a general problem with most of these is that v1i1 isn't a SimpleVT, and most of the legalizer logic is based on (enum SimpleVT)-indexed arrays. Adding v1i1 doesn't make perfect sense (it's not really legal on any target), but it seems sensible enough.

I have a few patches to implement the promotion, but that's problematic, because it then breaks the legalizer on this kind of input:

%rhs_t = trunc <1 x i16> %rhs to <1 x i1>

What do you think? I can put my experimental patches for #2 on phabricator if desired.

Thanks!
-Ahmed

[chandlerc]

FWIW, I'd rather not use promotion here.

My inclination is to widen where doing so produces a legal type (and we
really need to have the ability to represent vNi1s as legal types in x86
even if we don't in AArch64), and scalarizing due to type illegality when
we can't widen to a legal type.

I suppose I would be OK doing promotion *once widening fails to find a
legal type* as a lowering? But from your description, it isn't clear that
this helps.

I actually agree that having v1 vector types be legal seems quite strange.
It would seem better to avoid that long-term if possible, and so I'd vote
for moving in a direction that at least doesn't make that harder?

[• HaoLiu]

Hi Ahmed,

I think we have to promote v1i1 to v1i64 for AArch64 backend. As we
(1) can not widen it to any vNi1 types, as there isn't any vNi1 legal.
(2) can not scalarize it. Even if we add logic to scalarize sitofp, the GetScalarizedVector method still assumes all operands have been scalarized. But as v1i64 is legal, it will never scalarize this operand. So it will result in an assertion failure in GetScalarizedVector method.
(3) can not split it, as it only has one element.

So the only choice seems to be promoting it to v1i64 in AArch64 backend.

For the issue about

%rhs_t = trunc <1 x i16> %rhs to <1 x i1>

I don't try promoting v1i1 to v1i64, so I don't know the root cause. I just think we can still fix this.

Thanks,
-Hao

[ab]

I agree with Chandler, promotion isn't ideal, and from my quick experiments, it creates other problems by solving this one.
I guess we'll have to resort to working around the issue in the legalizer (Hao's solution #2, my #3, and the same as r205625).
See: http://reviews.llvm.org/differential/diff/15333/

Are you both fine with that?

(Also, I noticed a mistake in my comment: the problem is PR20777, not 778.)

-Ahmed

[• HaoLiu]

In D4322#23, @ab wrote:

I agree with Chandler, promotion isn't ideal, and from my quick experiments, it creates other problems by solving this one.
I guess we'll have to resort to working around the issue in the legalizer (Hao's solution #2, my #3, and the same as r205625).
See: http://reviews.llvm.org/differential/diff/15333/

Are you both fine with that?

Hi Ahmed,

Just one point about the following comments:

// For instance, this happens on AArch64, v1i1 is illegal but v1i{8,16,32}
// are widened to v1i64, which is legal.

v1i8, v1i16, v1i32 are widened to v8i8, v4i16, v2i32. If a type is legalized to v1i64, it is promoted.

I'm fine with that patch. But as I'm not quite familiar type legalize, it would be good to have someone expert like Chandler to also look it over.

Thanks,
-Hao

[ab]

Right, I meant "widened to 64-bit vectors of the same scalar type"; I corrected that comment and committed the workaround as r220937.

I'm open to alternative solutions!

Thanks,

• Ahmed

include/llvm/Target/TargetLowering.h

   /// legalization.
   virtual bool shouldSplitVectorType(EVT /*VT*/) const { return false; }
 
+  /// Return true if a vector of the given type should be scalarized during type
+  /// legalization.
+  virtual bool shouldScalarizeVectorType(EVT VT) const {
+    return VT.getVectorNumElements() == 1;
+  }
+
+  /// Return true if a vector of the given type is prefer to be widen.
+  virtual bool preferWidenVectorType(EVT VT) const { return false; }
+
   // There are two general methods for expanding a BUILD_VECTOR node:
   //  1. Use SCALAR_TO_VECTOR on the defined scalar values and then shuffle
   //     them together.

lib/CodeGen/TargetLoweringBase.cpp

     // that wider vector type.
     MVT EltVT = VT.getVectorElementType();
     unsigned NElts = VT.getVectorNumElements();
-    if (NElts != 1 && !shouldSplitVectorType(VT)) {
+    if (!shouldScalarizeVectorType(VT) && !shouldSplitVectorType(VT)) {
       bool IsLegalWiderType = false;
       // First try to promote the elements of integer vectors. If no legal
       // promotion was found, fallback to the widen-vector method.
-      for (unsigned nVT = i+1; nVT <= MVT::LAST_VECTOR_VALUETYPE; ++nVT) {
-        MVT SVT = (MVT::SimpleValueType)nVT;
-        // Promote vectors of integers to vectors with the same number
-        // of elements, with a wider element type.
-        if (SVT.getVectorElementType().getSizeInBits() > EltVT.getSizeInBits()
-            && SVT.getVectorNumElements() == NElts &&
-            isTypeLegal(SVT) && SVT.getScalarType().isInteger()) {
-          TransformToType[i] = SVT;
-          RegisterTypeForVT[i] = SVT;
-          NumRegistersForVT[i] = 1;
-          ValueTypeActions.setTypeAction(VT, TypePromoteInteger);
-          IsLegalWiderType = true;
-          break;
+      if (!preferWidenVectorType(VT)) {
+        for (unsigned nVT = i+1; nVT <= MVT::LAST_VECTOR_VALUETYPE; ++nVT) {
+          MVT SVT = (MVT::SimpleValueType)nVT;
+          // Promote vectors of integers to vectors with the same number
+          // of elements, with a wider element type.
+          if (SVT.getVectorElementType().getSizeInBits() > EltVT.getSizeInBits()
+              && SVT.getVectorNumElements() == NElts &&
+              isTypeLegal(SVT) && SVT.getScalarType().isInteger()) {
+            TransformToType[i] = SVT;
+            RegisterTypeForVT[i] = SVT;
+            NumRegistersForVT[i] = 1;
+            ValueTypeActions.setTypeAction(VT, TypePromoteInteger);
+            IsLegalWiderType = true;
+            break;
+          }
         }
-      }
 
         if (IsLegalWiderType) continue;
+      }
 
       // Try to widen the vector.
       for (unsigned nVT = i+1; nVT <= MVT::LAST_VECTOR_VALUETYPE; ++nVT) {

lib/Target/AArch64/AArch64ISelLowering.h

 
   bool shouldExpandAtomicInIR(Instruction *Inst) const override;
 
+  bool shouldScalarizeVectorType(EVT VT) const override;
+
+  bool preferWidenVectorType(EVT VT) const override;
+
 private:
   /// Subtarget - Keep a pointer to the AArch64Subtarget around so that we can
   /// make the right decision when generating code for different targets.

lib/Target/AArch64/AArch64ISelLowering.cpp

   return Inst->getType()->getPrimitiveSizeInBits() <= 128;
 }
 
+// During type legalization, v1i8, v1i16, v1i32, v1f32 should not be scalarized.
+bool AArch64TargetLowering::shouldScalarizeVectorType(EVT VT) const {
+  assert(VT.isVector() && "Expected vector type");
+  MVT SVT = VT.getSimpleVT();
+  return VT.getVectorNumElements() == 1 && SVT != MVT::v1i8 && SVT != MVT::v1i16
+         && SVT != MVT::v1i32 && SVT != MVT::v1f32;
+}
+
+// During type legalization, v1i8, v1i16, v1i32 are prefered be widen to v8i8,
+// v4i16, v2i32 instead of to be promoted.
+bool AArch64TargetLowering::preferWidenVectorType(EVT VT) const {
+  assert(VT.isVector() && "Expected vector type");
+  MVT SVT = VT.getSimpleVT();
+  return SVT == MVT::v1i8 || SVT == MVT::v1i16 || SVT == MVT::v1i32
+         || SVT == MVT::v1f32;
+}
+
 Value *AArch64TargetLowering::emitLoadLinked(IRBuilder<> &Builder, Value *Addr,
                                              AtomicOrdering Ord) const {
   Module *M = Builder.GetInsertBlock()->getParent()->getParent();

test/CodeGen/AArch64/arm64-neon-copy.ll

 
 define <8 x i8> @testDUP.v1i8(<1 x i8> %a) {
 ; CHECK-LABEL: testDUP.v1i8:
-; CHECK: dup {{v[0-9]+}}.8b, {{w[0-9]+}}
+; CHECK: dup v0.8b, v0.b[0]
   %b = extractelement <1 x i8> %a, i32 0
   %c = insertelement <8 x i8> undef, i8 %b, i32 0
   %d = insertelement <8 x i8> %c, i8 %b, i32 1

 
 define <8 x i16> @testDUP.v1i16(<1 x i16> %a) {
 ; CHECK-LABEL: testDUP.v1i16:
-; CHECK: dup {{v[0-9]+}}.8h, {{w[0-9]+}}
+; CHECK: dup v0.8h, v0.h[0]
   %b = extractelement <1 x i16> %a, i32 0
   %c = insertelement <8 x i16> undef, i16 %b, i32 0
   %d = insertelement <8 x i16> %c, i16 %b, i32 1

 
 define <4 x i32> @testDUP.v1i32(<1 x i32> %a) {
 ; CHECK-LABEL: testDUP.v1i32:
-; CHECK: dup {{v[0-9]+}}.4s, {{w[0-9]+}}
+; CHECK: dup v0.4s, v0.s[0]
   %b = extractelement <1 x i32> %a, i32 0
   %c = insertelement <4 x i32> undef, i32 %b, i32 0
   %d = insertelement <4 x i32> %c, i32 %b, i32 1

 
 define <4 x i16> @concat_vector_v4i16(<1 x i16> %a) {
 ; CHECK-LABEL: concat_vector_v4i16:
-; CHECK: dup {{v[0-9]+}}.4h, {{w[0-9]+}}
+; CHECK: dup v0.4h, v0.h[0]
  %r = shufflevector <1 x i16> %a, <1 x i16> undef, <4 x i32> zeroinitializer
  ret <4 x i16> %r
 }
 
 define <4 x i32> @concat_vector_v4i32(<1 x i32> %a) {
 ; CHECK-LABEL: concat_vector_v4i32:
-; CHECK: dup {{v[0-9]+}}.4s, {{w[0-9]+}}
+; CHECK: dup v0.4s, v0.s[0]
  %r = shufflevector <1 x i32> %a, <1 x i32> undef, <4 x i32> zeroinitializer
  ret <4 x i32> %r
 }
 
 define <8 x i8> @concat_vector_v8i8(<1 x i8> %a) {
 ; CHECK-LABEL: concat_vector_v8i8:
-; CHECK: dup {{v[0-9]+}}.8b, {{w[0-9]+}}
+; CHECK: dup v0.8b, v0.b[0]
  %r = shufflevector <1 x i8> %a, <1 x i8> undef, <8 x i32> zeroinitializer
  ret <8 x i8> %r
 }
 
 define <8 x i16> @concat_vector_v8i16(<1 x i16> %a) {
 ; CHECK-LABEL: concat_vector_v8i16:
-; CHECK: dup {{v[0-9]+}}.8h, {{w[0-9]+}}
+; CHECK: dup v0.8h, v0.h[0]
  %r = shufflevector <1 x i16> %a, <1 x i16> undef, <8 x i32> zeroinitializer
  ret <8 x i16> %r
 }
 
 define <16 x i8> @concat_vector_v16i8(<1 x i8> %a) {
 ; CHECK-LABEL: concat_vector_v16i8:
-; CHECK: dup {{v[0-9]+}}.16b, {{w[0-9]+}}
+; CHECK: dup v0.16b, v0.b[0]
  %r = shufflevector <1 x i8> %a, <1 x i8> undef, <16 x i32> zeroinitializer
  ret <16 x i8> %r
 }

test/CodeGen/AArch64/arm64-neon-select_cc.ll

 
 define <1 x float> @test_select_cc_v1f32(float %a, float %b, <1 x float> %c, <1 x float> %d ) {
 ; CHECK-LABEL: test_select_cc_v1f32:
-; CHECK: fcmp s0, s1
-; CHECK-NEXT: fcsel s0, s2, s3, eq
+; CHECK: fcmeq [[MASK:v[0-9]+]].2s, v0.2s, v1.2s
+; CHECK-NEXT: bsl [[MASK]].8b, v2.8b, v3.8b
   %cmp31 = fcmp oeq float %a, %b
   %e = select i1 %cmp31, <1 x float> %c, <1 x float> %d
   ret <1 x float> %e

test/CodeGen/AArch64/trunc-v1i64.ll

+; RUN: llc -mtriple=aarch64-none-linux-gnu -mattr=+neon -verify-machineinstrs < %s | FileCheck %s
+
+; An optimization in DAG Combiner to fold
+; (trunc (concat ... x ...)) -> (concat ..., (trunc x), ...))
+; will generate nodes like:
+;     v1i32 trunc v1i64, v1i16 trunc v1i64, v1i8 trunc v1i64.
+; And such nodes will be defaultly scalarized in type legalization. But such
+; scalarization will cause an assertion failure, as v1i64 is a legal type in
+; AArch64. We change the default behaviour from be scalarized to be widen.
+
+; FIXME: Currently XTN is generated for v1i32, but it can be optimized.
+; Just like v1i16 and v1i8, there is no XTN generated.
+
+define <2 x i32> @test_v1i32_0(<1 x i64> %in0) {
+; CHECK-LABEL: test_v1i32_0:
+; CHECK: xtn v0.2s, v0.2d
+  %1 = shufflevector <1 x i64> %in0, <1 x i64> undef, <2 x i32> <i32 0, i32 undef>
+  %2 = trunc <2 x i64> %1 to <2 x i32>
+  ret <2 x i32> %2
+}
+
+define <2 x i32> @test_v1i32_1(<1 x i64> %in0) {
+; CHECK-LABEL: test_v1i32_1:
+; CHECK: xtn v0.2s, v0.2d
+; CHECK-NEXT: dup v0.2s, v0.s[0]
+  %1 = shufflevector <1 x i64> %in0, <1 x i64> undef, <2 x i32> <i32 undef, i32 0>
+  %2 = trunc <2 x i64> %1 to <2 x i32>
+  ret <2 x i32> %2
+}
+
+define <4 x i16> @test_v1i16_0(<1 x i64> %in0) {
+; CHECK-LABEL: test_v1i16_0:
+; CHECK-NOT: xtn
+  %1 = shufflevector <1 x i64> %in0, <1 x i64> undef, <4 x i32> <i32 0, i32 undef, i32 undef, i32 undef>
+  %2 = trunc <4 x i64> %1 to <4 x i16>
+  ret <4 x i16> %2
+}
+
+define <4 x i16> @test_v1i16_1(<1 x i64> %in0) {
+; CHECK-LABEL: test_v1i16_1:
+; CHECK-NOT: xtn
+; CHECK: dup v0.4h, v0.h[0]
+  %1 = shufflevector <1 x i64> %in0, <1 x i64> undef, <4 x i32> <i32 undef, i32 undef, i32 0, i32 undef>
+  %2 = trunc <4 x i64> %1 to <4 x i16>
+  ret <4 x i16> %2
+}
+
+define <8 x i8> @test_v1i8_0(<1 x i64> %in0) {
+; CHECK-LABEL: test_v1i8_0:
+; CHECK-NOT: xtn
+  %1 = shufflevector <1 x i64> %in0, <1 x i64> undef, <8 x i32> <i32 0, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef>
+  %2 = trunc <8 x i64> %1 to <8 x i8>
+  ret <8 x i8> %2
+}
+
+define <8 x i8> @test_v1i8_1(<1 x i64> %in0) {
+; CHECK-LABEL: test_v1i8_1:
+; CHECK-NOT: xtn
+; CHECK: dup v0.8b, v0.b[0]
+  %1 = shufflevector <1 x i64> %in0, <1 x i64> undef, <8 x i32> <i32 undef, i32 undef, i32 0, i32 undef, i32 undef, i32 undef, i32 undef, i32 undef>
+  %2 = trunc <8 x i64> %1 to <8 x i8>
+  ret <8 x i8> %2
+}
 No newline at end of file