[AARCH64][NEON] Allow to sink operands for aarch64_neon_pmull.

[sunho]

Summary

This teaches AArch64TargetLowering::shouldSinkOperands to sink the operands of aarch64_neon_pmull intrinsic.

[sunho]

Tidy up testcases.

[fhahn]

Thanks for the patch! Can the test be stripped down? Does there need an actual loop or would just 2 different basic blocks be enough? I realise the test of a related patch is also similar, but ideally we would have more compact tests.

[sunho]

Make test case compact.

[sunho]

@fhahn I've shrinked the test case. Could you take a look?

[dmgreen]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

12108

This falls through to trying to treat the instruction like an indexed pmul, which isn't an instruction available in AArch64. This could do with a test case and possibly being guarded against.

llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll

2

Can you run update_llc_test_checks on the file. It's useful in cases like this to show all the output

27

I think you can likely remove this too.

[fhahn]

llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll

9

it would also be good to add a few negative tests, where sinking won't happen, e.g. because there are users in other blocks. Also cases where the other or both shuffles need sinking.

15

it think nowadays this should use poison here

[sunho]

llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll

9

When would sinking not happen? I've read the code, but it seems it's always sinking unless the operand is phi nod .

[sunho]

@fhahn @dmgreen I've accounted for your comments. Please take a look again!

[sunho]

.

[sunho]

Remove unused line

[fhahn]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

12108

IIUC there's still a test case missing to guard against the original issue mentioned by @dmgreen.

llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll

9

You are right, users in different BBs do not limit sinking, because the instructions are duplicated in that case. One case where sinking won't happen is if one operand has a high mask and one a low mask for example. But it might be better to test for something like that in llvm/test/Transforms/CodeGenPrepare/AArch64/sink-free-instructions.ll

13

You could use -asm-verbose=0 to trim down the output a bit.

21

it's slightly easier to read if there's a newline after the terminators.

[sunho]

Account for comments

[sunho]

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

12108

I've added a test case that makes sure splat vector is not sunk for pmull in "sink-free-instructions.ll."

llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll

9

I've added some negative tests in the file you mentioned.

[dmgreen]

Looks like a good patch, from what I can see. Thanks

LGTM

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

12121

You don't need brackets around single statements in llvm. I would personally also invert the condition - if (thing) doathing() seems clearer in intent to me, but I do see that other cases here work the other way arounds already.

[sunho]

Thanks! Can you commit on my behalf?

Author name: sunho
Author email: [e-mail]
Commit message: [AARCH64][NEON] Allow to sink operands for aarch64_neon_pmull.

[dmgreen]

Sure thing. I'll remove the brackets whilst I do.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
     switch (II->getIntrinsicID()) {
     case Intrinsic::aarch64_neon_smull:
     case Intrinsic::aarch64_neon_umull:
       if (areExtractShuffleVectors(II->getOperand(0), II->getOperand(1))) {
         Ops.push_back(&II->getOperandUse(0));
         Ops.push_back(&II->getOperandUse(1));
         return true;
       }
       LLVM_FALLTHROUGH;
 
     case Intrinsic::aarch64_neon_sqdmull:
     case Intrinsic::aarch64_neon_sqdmulh:
     case Intrinsic::aarch64_neon_sqrdmulh:
       // Sink splats for index lane variants
       if (isSplatShuffle(II->getOperand(0)))
         Ops.push_back(&II->getOperandUse(0));
       if (isSplatShuffle(II->getOperand(1)))
         Ops.push_back(&II->getOperandUse(1));
       return !Ops.empty();
 
+    case Intrinsic::aarch64_neon_pmull:
+      if (!areExtractShuffleVectors(II->getOperand(0), II->getOperand(1))) {
+        return false;
+      }
+      Ops.push_back(&II->getOperandUse(0));
+      Ops.push_back(&II->getOperandUse(1));
+      return true;
     case Intrinsic::aarch64_neon_pmull64:
       if (!areOperandsOfVmullHighP64(II->getArgOperand(0),
                                      II->getArgOperand(1)))
         return false;
       Ops.push_back(&II->getArgOperandUse(0));
       Ops.push_back(&II->getArgOperandUse(1));
       return true;
 

llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll

+; RUN: llc < %s -verify-machineinstrs -mtriple=aarch64-linux-gnu -mattr=+neon | FileCheck %s
+
+; Check that pmull2 instruction is used for vmull_high_p8 intrinsic
+; even if shufflevector instructions are located in different basic blocks,
+; which can happen when vmull_high_p8 is used inside a loop body.
+;
+
+define <8 x i16> @test_pmull2_sink(<16 x i8> %a, <16 x i8> %b, <8 x i16> %c, i1 %t) {
+; CHECK-LABEL: test_pmull2_sink:
+; CHECK:       // %bb.0: // %entry
+; CHECK-NEXT:    tbz w0, #0, .LBB0_2
+; CHECK-NEXT:  // %bb.1: // %if.then
+; CHECK-NEXT:    pmull2 v0.8h, v0.16b, v1.16b
+; CHECK-NEXT:    ret
+; CHECK-NEXT:  .LBB0_2: // %cleanup
+; CHECK-NEXT:    mov v0.16b, v2.16b
+; CHECK-NEXT:    ret
+entry:
+  %0 = shufflevector <16 x i8> %a, <16 x i8> poison, <8 x i32> <i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15>
+  br i1 %t, label %if.then, label %cleanup
+if.then:
+  %1 = shufflevector <16 x i8> %b, <16 x i8> poison, <8 x i32> <i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15>
+  %res = tail call <8 x i16> @llvm.aarch64.neon.pmull.v8i16(<8 x i8> %0, <8 x i8> %1)
+  br label %cleanup
+cleanup:
+  %retval = phi <8 x i16> [ %res, %if.then ], [ %c, %entry ]
+  ret <8 x i16> %retval
+}
+
+define <8 x i16> @test_pmull2_sink2(<16 x i8> %a, <16 x i8> %b, <8 x i16> %c, i1 %t) {
+; CHECK-LABEL: test_pmull2_sink2:
+; CHECK:       // %bb.0: // %entry
+; CHECK-NEXT:    tbz w0, #0, .LBB1_2
+; CHECK-NEXT:  // %bb.1: // %if.then
+; CHECK-NEXT:    pmull2 v0.8h, v0.16b, v0.16b
+; CHECK-NEXT:    ret
+; CHECK-NEXT:  .LBB1_2: // %cleanup
+; CHECK-NEXT:    mov v0.16b, v2.16b
+; CHECK-NEXT:    ret
+entry:
+  %0 = shufflevector <16 x i8> %a, <16 x i8> poison, <8 x i32> <i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15>
+  %1 = shufflevector <16 x i8> %a, <16 x i8> poison, <8 x i32> <i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15>
+  br i1 %t, label %if.then, label %cleanup
+if.then:
+  %res = tail call <8 x i16> @llvm.aarch64.neon.pmull.v8i16(<8 x i8> %0, <8 x i8> %1)
+  br label %cleanup
+cleanup:
+  %retval = phi <8 x i16> [ %res, %if.then ], [ %c, %entry ]
+  ret <8 x i16> %retval
+}
+
+define <8 x i16> @test_pmull2_sink3(<16 x i8> %a, <16 x i8> %b, <8 x i16> %c, i1 %t, i1 %t2) {
+; CHECK-LABEL: test_pmull2_sink3:
+; CHECK:       // %bb.0: // %entry
+; CHECK-NEXT:    tbz w0, #0, .LBB2_2
+; CHECK-NEXT:  // %bb.1: // %if.then
+; CHECK-NEXT:    tbz w1, #0, .LBB2_3
+; CHECK-NEXT:  .LBB2_2: // %if.then.2
+; CHECK-NEXT:    pmull2 v0.8h, v0.16b, v1.16b
+; CHECK-NEXT:    ret
+; CHECK-NEXT:  .LBB2_3: // %cleanup
+; CHECK-NEXT:    pmull2 v0.8h, v0.16b, v0.16b
+; CHECK-NEXT:    ret
+entry:
+  %0 = shufflevector <16 x i8> %a, <16 x i8> poison, <8 x i32> <i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15>
+  br i1 %t, label %if.then, label %if.then.2
+if.then:
+  %res = tail call <8 x i16> @llvm.aarch64.neon.pmull.v8i16(<8 x i8> %0, <8 x i8> %0)
+  %1 = trunc <8 x i16> %res to <8 x i8>
+  br i1 %t2, label %if.then.2, label %cleanup
+if.then.2:
+  %2 = shufflevector <16 x i8> %b, <16 x i8> poison, <8 x i32> <i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15>
+  %res2 = tail call <8 x i16> @llvm.aarch64.neon.pmull.v8i16(<8 x i8> %0, <8 x i8> %2)
+  br label %cleanup
+cleanup:
+  %retval = phi <8 x i16> [ %res2, %if.then.2 ], [ %res, %if.then ]
+  ret <8 x i16> %retval
+}
+
+declare <8 x i16> @llvm.aarch64.neon.pmull.v8i16(<8 x i8>, <8 x i8>)