[PATCH] D26072: [PPC] add absolute difference altivec instructions and matching intrinsics

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.


================
Comment at: include/llvm/IR/IntrinsicsPowerPC.td:697
+def int_ppc_altivec_vabsdub :
+      PowerPC_Vec_Intrinsic<"vabsdub", [llvm_v16i8_ty],
+                            [llvm_v16i8_ty, llvm_v16i8_ty], [IntrNoMem]>;
----------------
Been a while since I looked at this, but do the PowerPC_Vec_BBB_Intrinsic and it's halfword and word equivalent not have the same definition as this (and therefore allow you to more concisely define these)? Basically what I'm getting at is using the most specialized class that has the right form.


================
Comment at: lib/Target/PowerPC/PPCInstrAltivec.td:454
 
+
 let PPC970_Unit = 5 in {  // VALU Operations.
----------------
Please don't add blank lines arbitrarily.


================
Comment at: lib/Target/PowerPC/PPCInstrAltivec.td:1410-1419
+// Absolute Difference
+def VABSDUB : VXForm_1<1027, (outs vrrc:$vD), (ins vrrc:$vA, vrrc:$vB),
+                      "vabsdub $vD, $vA, $vB", IIC_VecGeneral,
+                      [(set v16i8:$vD, (int_ppc_altivec_vabsdub v16i8:$vA, v16i8:$vB))]>;
+def VABSDUH : VXForm_1<1091, (outs vrrc:$vD), (ins vrrc:$vA, vrrc:$vB),
+                       "vabsduh $vD, $vA, $vB", IIC_VecGeneral,
+                       [(set v8i16:$vD, (int_ppc_altivec_vabsduh v8i16:$vA, v8i16:$vB))]>;
----------------
amehsan wrote:
> IIRC, when I was doing the patches that I sent you, there was a generic absolute value intrinsic, that I could use. That is better than the PPC intrinsics, because it is more likely that optimizations understand it. Could you use the generic one here?
I don't think there is a generic one for integer types, but I might be wrong. I think there's only llvm.fabs.


================
Comment at: lib/Target/PowerPC/PPCInstrAltivec.td:1412
+def VABSDUB : VXForm_1<1027, (outs vrrc:$vD), (ins vrrc:$vA, vrrc:$vB),
+                      "vabsdub $vD, $vA, $vB", IIC_VecGeneral,
+                      [(set v16i8:$vD, (int_ppc_altivec_vabsdub v16i8:$vA, v16i8:$vB))]>;
----------------
This is a very minor nit, but the very first characters on the continuation line should line up inside the angle bracket. So the double quote character should line up with the 1 character above.


================
Comment at: test/CodeGen/PowerPC/vec_absd.ll:1-2
+; RUN: llc -verify-machineinstrs -mcpu=pwr9 -mtriple=powerpc64-unknown-linux-gnu -mattr=+altivec < %s | FileCheck %s
+; RUN: llc -verify-machineinstrs -mcpu=pwr9 -mtriple=powerpc64le-unknown-linux-gnu -mattr=+altivec < %s | FileCheck %s
+
----------------
amehsan wrote:
> you should not need -mattr=+altivec when you have -mcpu=pwr9. Does something go wrong when you remove that?
I agree. If this test case behaves differently with and without -mattr=+altivec, we need to look into why.


================
Comment at: test/CodeGen/PowerPC/vec_absd.ll:20
+; CHECK-LABEL: @test_byte
+; CHECK: vabsdub
+; CHECK blr
----------------
Please add the register operands here. The ABI locks all 3.
`vabsdub 2, 2, 3`
The same goes for the other tests as well.


================
Comment at: test/MC/Disassembler/PowerPC/ppc64-encoding-p9vector.txt:1
+# RUN: llvm-mc --disassemble %s -triple powerpc64-unknown-unknown -mcpu=pwr9  | FileCheck %s
+
----------------
Please don't add this file or the one below. The assembler and disassembler do not care about predicates. If they understand an encoding, they will emit what it is. Just add these to the existing test cases.


Repository:
  rL LLVM

https://reviews.llvm.org/D26072