[GlobalISel] Add G_ABDS and G_ABDU instructions #118122

tschuett · 2024-11-29T19:21:09Z

The DAG has the same instructions: the signed and unsigned absolute difference of it's input. For AArch64, they map to uabd and sabd for Neon and SVE. The Neon and SVE instructions will require custom patterns.

They are pseudo opcodes and are not imported by the IRTranslator. We need combines to create them.

PowerPC, ARM, and AArch64 have native instructions.

/// i.e trunc(abs(sext(Op0) - sext(Op1))) becomes abds(Op0, Op1)
/// or trunc(abs(zext(Op0) - zext(Op1))) becomes abdu(Op0, Op1)

For GlobalISel, we are going to write the combines in MIR patterns.

see:
llvm/test/CodeGen/AArch64/abd-combine.ll

combine into abd
legalize and add td patterns

Fixes llvm#118085 The DAG has the same instructions: the signed and unsigned absulute difference of it's input. For AArch64, they map to uabd and sabd for Neon and SVE. The SVE instructions will require custom patterns. /// i.e trunc(abs(sext(Op0) - sext(Op1))) becomes abds(Op0, Op1) /// or trunc(abs(zext(Op0) - zext(Op1))) becomes abdu(Op0, Op1) For GlobalISel, we are going to write the combines in MIR patterns. see: llvm/test/CodeGen/AArch64/abd-combine.ll

llvmbot · 2024-11-29T19:21:42Z

@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-llvm-globalisel

@llvm/pr-subscribers-backend-aarch64

Author: Thorsten Schütt (tschuett)

Changes

Fixes #118085

The DAG has the same instructions: the signed and unsigned absulute difference of it's input. For AArch64, they map to uabd and sabd for Neon and SVE. The SVE instructions will require custom patterns.

/// i.e trunc(abs(sext(Op0) - sext(Op1))) becomes abds(Op0, Op1)
/// or trunc(abs(zext(Op0) - zext(Op1))) becomes abdu(Op0, Op1)

For GlobalISel, we are going to write the combines in MIR patterns.

see:
llvm/test/CodeGen/AArch64/abd-combine.ll

Full diff: https://github.com/llvm/llvm-project/pull/118122.diff

8 Files Affected:

(modified) llvm/docs/GlobalISel/GenericOpcode.rst (+7)
(modified) llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h (+28)
(modified) llvm/include/llvm/Support/TargetOpcodes.def (+6)
(modified) llvm/include/llvm/Target/GenericOpcodes.td (+14)
(modified) llvm/lib/CodeGen/MachineVerifier.cpp (+35)
(modified) llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir (+8)
(modified) llvm/test/CodeGen/RISCV/GlobalISel/legalizer-info-validation.mir (+8)
(added) llvm/test/MachineVerifier/test_abd_su.mir (+35)

diff --git a/llvm/docs/GlobalISel/GenericOpcode.rst b/llvm/docs/GlobalISel/GenericOpcode.rst
index 076dc7fa93e565..718f99373d28c1 100644
--- a/llvm/docs/GlobalISel/GenericOpcode.rst
+++ b/llvm/docs/GlobalISel/GenericOpcode.rst
@@ -474,6 +474,13 @@ undefined.
   %2:_(s33) = G_CTLZ_ZERO_UNDEF %1
   %2:_(s33) = G_CTTZ_ZERO_UNDEF %1
 
+Compute the absolute difference (signed and unsigned), e.g. abs(x-y).
+
+.. code-block:: none
+
+  %0:_(s33) = G_ABDS %2, %3
+  %1:_(s33) = G_ABDU %4, %5
+
 Floating Point Operations
 -------------------------
 
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index 3516065f9b6cb3..8c1f5e0f4135cb 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -1767,6 +1767,34 @@ class MachineIRBuilder {
     return buildInstr(TargetOpcode::G_MUL, {Dst}, {Src0, Src1}, Flags);
   }
 
+  /// Build and insert \p Res = G_ABDS \p Op0, \p Op1
+  ///
+  /// G_ABDS return the signed absolute difference of \p Op0 and \p Op1.
+  ///
+  /// \pre setBasicBlock or setMI must have been called.
+  /// \pre \p Res, \p Op0 and \p Op1 must be generic virtual registers
+  ///      with the same (scalar or vector) type).
+  ///
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  MachineInstrBuilder buildAbds(const DstOp &Dst, const SrcOp &Src0,
+                                const SrcOp &Src1) {
+    return buildInstr(TargetOpcode::G_ABDS, {Dst}, {Src0, Src1});
+  }
+
+  /// Build and insert \p Res = G_ABDU \p Op0, \p Op1
+  ///
+  /// G_ABDS return the unsigned absolute difference of \p Op0 and \p Op1.
+  ///
+  /// \pre setBasicBlock or setMI must have been called.
+  /// \pre \p Res, \p Op0 and \p Op1 must be generic virtual registers
+  ///      with the same (scalar or vector) type).
+  ///
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  MachineInstrBuilder buildAbdu(const DstOp &Dst, const SrcOp &Src0,
+                                const SrcOp &Src1) {
+    return buildInstr(TargetOpcode::G_ABDU, {Dst}, {Src0, Src1});
+  }
+
   MachineInstrBuilder buildUMulH(const DstOp &Dst, const SrcOp &Src0,
                                  const SrcOp &Src1,
                                  std::optional<unsigned> Flags = std::nullopt) {
diff --git a/llvm/include/llvm/Support/TargetOpcodes.def b/llvm/include/llvm/Support/TargetOpcodes.def
index 17987935ed3cf4..5ef3707b81fe91 100644
--- a/llvm/include/llvm/Support/TargetOpcodes.def
+++ b/llvm/include/llvm/Support/TargetOpcodes.def
@@ -289,6 +289,12 @@ HANDLE_TARGET_OPCODE(G_OR)
 /// Generic bitwise exclusive-or instruction.
 HANDLE_TARGET_OPCODE(G_XOR)
 
+/// Generic absolute difference signed instruction.
+HANDLE_TARGET_OPCODE(G_ABDS)
+
+/// Generic absolute difference unsigned instruction.
+HANDLE_TARGET_OPCODE(G_ABDU)
+
 
 HANDLE_TARGET_OPCODE(G_IMPLICIT_DEF)
 
diff --git a/llvm/include/llvm/Target/GenericOpcodes.td b/llvm/include/llvm/Target/GenericOpcodes.td
index 60606db078b374..ab18bfa6b0f8bf 100644
--- a/llvm/include/llvm/Target/GenericOpcodes.td
+++ b/llvm/include/llvm/Target/GenericOpcodes.td
@@ -386,6 +386,20 @@ def G_ASHR : GenericInstruction {
   let hasSideEffects = false;
 }
 
+// Generic absolute difference signed.
+def G_ABDS : GenericInstruction {
+  let OutOperandList = (outs type0:$dst);
+  let InOperandList = (ins type0:$src1, type0:$src2);
+  let hasSideEffects = false;
+}
+
+// Generic absolute difference unsigned.
+def G_ABDU : GenericInstruction {
+  let OutOperandList = (outs type0:$dst);
+  let InOperandList = (ins type0:$src1, type0:$src2);
+  let hasSideEffects = false;
+}
+
 /// Funnel 'double' shifts take 3 operands, 2 inputs and the shift amount.
 /// fshl(X,Y,Z): (X << (Z % bitwidth)) | (Y >> (bitwidth - (Z % bitwidth)))
 def G_FSHL : GenericInstruction {
diff --git a/llvm/lib/CodeGen/MachineVerifier.cpp b/llvm/lib/CodeGen/MachineVerifier.cpp
index b08a93ae9a6d58..7f02637dfde1a3 100644
--- a/llvm/lib/CodeGen/MachineVerifier.cpp
+++ b/llvm/lib/CodeGen/MachineVerifier.cpp
@@ -1585,6 +1585,41 @@ void MachineVerifier::verifyPreISelGenericInstruction(const MachineInstr *MI) {
 
     break;
   }
+  case TargetOpcode::G_ABDS:
+  case TargetOpcode::G_ABDU: {
+    LLT DstTy = MRI->getType(MI->getOperand(0).getReg());
+    LLT SrcTy = MRI->getType(MI->getOperand(1).getReg());
+    LLT SrcTy2 = MRI->getType(MI->getOperand(2).getReg());
+
+    if (SrcTy.isPointerOrPointerVector() || SrcTy2.isPointerOrPointerVector()) {
+      report("Generic abds/abdu does not support pointers as operands", MI);
+      break;
+    }
+
+    if (DstTy.isPointerOrPointerVector()) {
+      report("Generic abds/abdu does not support pointers as a result", MI);
+      break;
+    }
+
+    if ((DstTy.isVector() != SrcTy.isVector()) ||
+        (DstTy.isVector() &&
+         DstTy.getElementCount() != SrcTy.getElementCount())) {
+      report("Generic vector abds/abdu must preserve number of lanes", MI);
+      break;
+    }
+
+    if (SrcTy != SrcTy2) {
+      report("Generic abds/abdu must have same input types", MI);
+      break;
+    }
+
+    if (DstTy != SrcTy) {
+      report("Generic abds/abdu must have same input and output types", MI);
+      break;
+    }
+
+    break;
+  }
   case TargetOpcode::G_SCMP:
   case TargetOpcode::G_UCMP: {
     LLT DstTy = MRI->getType(MI->getOperand(0).getReg());
diff --git a/llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir b/llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir
index d35bface7cb48b..7c9c958b5a8189 100644
--- a/llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir
+++ b/llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir
@@ -70,6 +70,14 @@
 # DEBUG-NEXT: .. the first uncovered type index: 1, OK
 # DEBUG-NEXT: .. the first uncovered imm index: 0, OK
 #
+# DEBUG-NEXT: G_ABDS (opcode 65): 1 type index, 0 imm indices
+# DEBUG-NEXT:.. type index coverage check SKIPPED: no rules defined
+# DEBUG-NEXT:.. imm index coverage check SKIPPED: no rules defined
+#
+# DEBUG-NEXT:G_ABDU (opcode 66): 1 type index, 0 imm indices
+# DEBUG-NEXT:.. type index coverage check SKIPPED: no rules defined
+# DEBUG-NEXT:.. imm index coverage check SKIPPED: no rules defined
+#
 # DEBUG-NEXT: G_IMPLICIT_DEF (opcode {{[0-9]+}}): 1 type index, 0 imm indices
 # DEBUG-NEXT: .. the first uncovered type index: {{[0-9]+}}, OK
 # DEBUG-NEXT: .. the first uncovered imm index: {{[0-9]+}}, OK
diff --git a/llvm/test/CodeGen/RISCV/GlobalISel/legalizer-info-validation.mir b/llvm/test/CodeGen/RISCV/GlobalISel/legalizer-info-validation.mir
index 719ea38cbb9c52..e2ba4f4b7651d3 100644
--- a/llvm/test/CodeGen/RISCV/GlobalISel/legalizer-info-validation.mir
+++ b/llvm/test/CodeGen/RISCV/GlobalISel/legalizer-info-validation.mir
@@ -73,6 +73,14 @@
 # DEBUG-NEXT: .. type index coverage check SKIPPED: user-defined predicate detected
 # DEBUG-NEXT: .. imm index coverage check SKIPPED: user-defined predicate detected
 #
+# DEBUG-NEXT: G_ABDS (opcode 65): 1 type index, 0 imm indices
+# DEBUG-NEXT:.. type index coverage check SKIPPED: no rules defined
+# DEBUG-NEXT:.. imm index coverage check SKIPPED: no rules defined
+#
+# DEBUG-NEXT:G_ABDU (opcode 66): 1 type index, 0 imm indices
+# DEBUG-NEXT:.. type index coverage check SKIPPED: no rules defined
+# DEBUG-NEXT:.. imm index coverage check SKIPPED: no rules defined
+#
 # DEBUG-NEXT: G_IMPLICIT_DEF (opcode {{[0-9]+}}): 1 type index, 0 imm indices
 # DEBUG-NEXT: .. type index coverage check SKIPPED: user-defined predicate detected
 # DEBUG-NEXT: .. imm index coverage check SKIPPED: user-defined predicate detected
diff --git a/llvm/test/MachineVerifier/test_abd_su.mir b/llvm/test/MachineVerifier/test_abd_su.mir
new file mode 100644
index 00000000000000..d8347ff3e5a519
--- /dev/null
+++ b/llvm/test/MachineVerifier/test_abd_su.mir
@@ -0,0 +1,35 @@
+# RUN: not --crash llc -verify-machineinstrs -mtriple=arm64 -run-pass none -o /dev/null %s 2>&1 | FileCheck %s
+# REQUIRES: aarch64-registered-target
+
+---
+name:            g_abd_su
+body: |
+  bb.0:
+
+    %2:_(p0) = G_IMPLICIT_DEF
+    %3:_(p0) = G_IMPLICIT_DEF
+    ; CHECK: Generic abds/abdu does not support pointers as operands
+    %4:_(s1) = G_ABDS %2, %3
+
+    %12:_(s64) = G_IMPLICIT_DEF
+    %13:_(s64) = G_IMPLICIT_DEF
+    ; CHECK: Generic abds/abdu does not support pointers as a result
+    %14:_(p0) = G_ABDS %12, %13
+
+    %23:_(<2 x s32>) = G_IMPLICIT_DEF
+    %24:_(<2 x s32>) = G_IMPLICIT_DEF
+    ; CHECK: Generic vector abds/abdu must preserve number of lanes
+    %5:_(s1) = G_ABDU  %23, %24
+
+    %15:_(s32) = G_CONSTANT i32 0
+    %16:_(s64) = G_CONSTANT i64 2
+    ; CHECK: Generic abds/abdu must have same input types
+    %17:_(s1) = G_ABDU %15, %16
+
+    %18:_(s64) = G_CONSTANT i64 0
+    %19:_(s64) = G_CONSTANT i64 2
+    ; CHECK: Generic abds/abdu must have same input and output types
+    %20:_(s1) = G_ABDU %18, %19
+
+...
+

tschuett · 2024-11-29T19:34:23Z

The DAG combine:
https://github.com/llvm/llvm-project/blob/be75a14207ab0dac6ad9f9097957f14a918ac611/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L11081

tschuett · 2024-11-30T07:14:14Z

def combine_abds : GICombineRule<
  (defs root:$root, build_fn_matchinfo:$matchinfo),
  (match (G_SEXT $lhs, $x),
         (G_SEXT $rhs, $y),
         (G_SUB $sub, $lhs, $rhs),
         (G_ABS $abs, $sub),
         (G_TRUNC $root, $abs):$trunc,
         [{ return Helper.matchAbds(*${trunc}, ${matchinfo}); }]),
  (apply [{ Helper.applyBuildFn(*${trunc}, ${matchinfo}); }])>;

tschuett · 2024-11-30T09:43:22Z

X86 lowers them sometimes into selects. We could add the reverse combine:
// abds(lhs, rhs) -> select(slt(lhs,rhs),sub(rhs,lhs),sub(lhs,rhs))
// abdu(lhs, rhs) -> select(ult(lhs,rhs),sub(rhs,lhs),sub(lhs,rhs))

tschuett · 2024-11-30T14:36:10Z

RISCV lowering:
// abds(lhs, rhs) -> sub(smax(lhs,rhs), smin(lhs,rhs))
// abdu(lhs, rhs) -> sub(umax(lhs,rhs), umin(lhs,rhs))

tschuett · 2024-12-02T13:19:33Z

Should we follow the DAG or add a fleet of pseudo opcodes that model AArch64 instructions?

tschuett · 2024-12-02T18:00:04Z

Alternative / teaser:
Add 20 pseudo opcodes, map them to AArch64ISDs (AArch64udot), and add a ton of combines into the pseudos.

arsenm · 2024-12-03T18:30:26Z

llvm/lib/CodeGen/MachineVerifier.cpp

+    if (SrcTy.isPointerOrPointerVector() || SrcTy2.isPointerOrPointerVector()) {
+      report("Generic abds/abdu does not support pointers as operands", MI);
+      break;
+    }
+
+    if (DstTy.isPointerOrPointerVector()) {
+      report("Generic abds/abdu does not support pointers as a result", MI);
+      break;
+    }


Does this not get verified by default? I thought there were separate type and ptype to distinguish them

Good point. It was copy-pasted/inspired by the G_SCMP and G_USCMP nearby.

arsenm · 2024-12-03T18:31:52Z

llvm/test/MachineVerifier/test_abd_su.mir

+    %19:_(s64) = G_CONSTANT i64 2
+    ; CHECK: Generic abds/abdu must have same input and output types
+    %20:_(s1) = G_ABDU %18, %19
+


Maybe check vector of pointer since that manages to always break

remove pointer verification and test

arsenm · 2024-12-03T21:38:56Z

llvm/test/MachineVerifier/test_abd_su.mir

@@ -6,16 +6,6 @@ name:            g_abd_su
 body: |
  bb.0:

-    %2:_(p0) = G_IMPLICIT_DEF
-    %3:_(p0) = G_IMPLICIT_DEF
-    ; CHECK: Generic abds/abdu does not support pointers as operands


You would still keep these tests even if the generic logic handles it

arsenm · 2024-12-04T13:14:49Z

llvm/test/MachineVerifier/test_abd_su.mir

@@ -6,6 +6,14 @@ name:            g_abd_su
 body: |
  bb.0:

+    %2:_(p0) = G_IMPLICIT_DEF
+    %3:_(p0) = G_IMPLICIT_DEF
+    %4:_(s1) = G_ABDS %2, %3


Missing the error check?

I removed the C++ error checking code under the assumption that the parameters are types and not ptypes and some other part of the system checks for misbehaviour.

But that does not remove the need to check that the correct error is produced. I also see the case with a pointer type in each operand is not rejected

tschuett requested review from aemerson, arsenm and davemgreen November 29, 2024 19:21

llvmbot added backend:AArch64 llvm:globalisel llvm:support labels Nov 29, 2024

Thorsten Schütt added 2 commits November 29, 2024 21:42

fix typo

fb0454a

global isel emiiter

07cce36

They are isCommutable=true.

9bb15cb

tschuett self-assigned this Dec 1, 2024

arsenm approved these changes Dec 3, 2024

View reviewed changes

address review comments

c640bea

remove pointer verification and test

arsenm reviewed Dec 3, 2024

View reviewed changes

restore etests

ad8b237

tschuett merged commit 148fdc5 into llvm:main Dec 4, 2024
9 checks passed

tschuett deleted the gisel-absd-su branch December 4, 2024 11:53

arsenm reviewed Dec 4, 2024

View reviewed changes

davemgreen mentioned this pull request Dec 12, 2024

[AArch64][GlobalISel] HADD operations and combines #118083

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[GlobalISel] Add G_ABDS and G_ABDU instructions #118122

[GlobalISel] Add G_ABDS and G_ABDU instructions #118122

Uh oh!

tschuett commented Nov 29, 2024 •

edited

Loading

Uh oh!

llvmbot commented Nov 29, 2024 •

edited

Loading

Uh oh!

tschuett commented Nov 29, 2024

Uh oh!

tschuett commented Nov 30, 2024

Uh oh!

tschuett commented Nov 30, 2024

Uh oh!

tschuett commented Nov 30, 2024

Uh oh!

tschuett commented Dec 2, 2024

Uh oh!

tschuett commented Dec 2, 2024 •

edited

Loading

Uh oh!

arsenm Dec 3, 2024

Uh oh!

tschuett Dec 3, 2024

Uh oh!

arsenm Dec 3, 2024

Uh oh!

arsenm Dec 3, 2024

Uh oh!

Uh oh!

arsenm Dec 4, 2024

Uh oh!

tschuett Dec 4, 2024

Uh oh!

arsenm Dec 4, 2024

Uh oh!

Uh oh!

[GlobalISel] Add G_ABDS and G_ABDU instructions #118122

[GlobalISel] Add G_ABDS and G_ABDU instructions #118122

Uh oh!

Conversation

tschuett commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tschuett commented Nov 29, 2024

Uh oh!

tschuett commented Nov 30, 2024

Uh oh!

tschuett commented Nov 30, 2024

Uh oh!

tschuett commented Nov 30, 2024

Uh oh!

tschuett commented Dec 2, 2024

Uh oh!

tschuett commented Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

tschuett Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Dec 3, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

arsenm Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

tschuett Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Dec 4, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tschuett commented Nov 29, 2024 •

edited

Loading

llvmbot commented Nov 29, 2024 •

edited

Loading

tschuett commented Dec 2, 2024 •

edited

Loading