[PATCH] D117944: [AARCH64][NEON] Allow to sink operands for aarch64_neon_pmull.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12109
       }
       LLVM_FALLTHROUGH;
 
----------------
dmgreen wrote:
> 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.
IIUC there's still a test case missing to guard against the original issue mentioned by @dmgreen. 


================
Comment at: llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll:12
+; CHECK-NEXT:    tbz w0, #0, .LBB0_2
+; CHECK-NEXT:  // %bb.1: // %if.then
+; CHECK-NEXT:    pmull2 v0.8h, v0.16b, v1.16b
----------------
You could use ` -asm-verbose=0` to trim down the output a bit.


================
Comment at: llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll:20
+  %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:
----------------
it's slightly easier to read if there's a newline after the terminators.


================
Comment at: llvm/test/CodeGen/AArch64/neon-vmull-high-p8.ll:8
+
+define <8 x i16> @test_pmull2_sink(<16 x i8> %a, <16 x i8> %b, <8 x i16> %c, i1 %t) {
+entry:
----------------
sunho wrote:
> fhahn wrote:
> > 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.
> When would sinking not happen? I've read the code, but it seems it's always sinking unless the operand is phi nod .
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`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117944/new/

https://reviews.llvm.org/D117944