[PATCH] D130548: [AArch64] Explicitly use v1i64 type for llvm.aarch64.neon.pmull64

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Thanks, this sounds good to me.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4523
+    SDValue Op2 = Op.getOperand(2);
+    if (Op1.getNode()->getOpcode() == ISD::LOAD &&
+        Op1.getNode()->getValueType(0) == MVT::i64) {
----------------
Can we do this for all operands, not just loads? We should end up adding the i64->v1i64 copy in either case.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:4524
+    if (Op1.getNode()->getOpcode() == ISD::LOAD &&
+        Op1.getNode()->getValueType(0) == MVT::i64) {
+      Op1 = DAG.getNode(ISD::BITCAST, dl, MVT::v1i64, Op1);
----------------
Op1.getValueType()


================
Comment at: llvm/test/CodeGen/AArch64/pmull-ldr-merge.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -verify-machineinstrs  -mtriple=aarch64-linux-gnu -mattr=+v8r,+neon,+crc,+crypto -o - %s| FileCheck %s --check-prefixes=CHECK,CHECK-SDAG
+
----------------
I think use -mattr=+aes. (Even is that is not technically correct, it is what the instruction uses)
And it doesn't need CHECK-SDAG


================
Comment at: llvm/test/CodeGen/AArch64/pmull-ldr-merge.ll:32
+
+8:
+  %9 = phi i64 [ %18, %8 ], [ 0, %5 ]
----------------
The test could be simpler if it wasn't in a loop.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130548