[RISCV][NFC] Deduplicate Atomic Intrinsic Definitions

[lenary]

Summary

This is a slight cleanup, to use multiclasses to avoid the duplication between
the different atomic intrinsic definitions. The produced intrinsics are
unchanged, they're just generated in a more succinct way.

[luismarques]

LGTM.

[jrtc27]

I would personally rather this remain unindented, much like we do with namespace llvm etc.

[luismarques]

In D71777#1795194, @jrtc27 wrote:

I would personally rather this remain unindented, much like we do with namespace llvm etc.

I too was a bit surprised by the formatting (especially the indented section separator) but @lenary told me he just ran clang-format on the file. And I looked around and saw that some other tablegen files in LLVM also used this style of formatting, so taken together I deemed the patch style reasonable during the review. But it's true that not indenting is the most common style and has its advantages, so I would also favour keeping it unindented.

[jrtc27]

In D71777#1795201, @luismarques wrote:

In D71777#1795194, @jrtc27 wrote:

I would personally rather this remain unindented, much like we do with namespace llvm etc.

I too was a bit surprised by the formatting (especially the indented section separator) but @lenary told me he just ran clang-format on the file. And I looked around and saw that some other tablegen files in LLVM also used this style of formatting, so taken together I deemed the patch style reasonable during the review. But it's true that not indenting is the most common style and has its advantages, so I would also favour keeping it unindented.

I guess TableGen is close enough to C/C++ in syntax... but given our TableGen style in the project is generally less indented (and TableGen is often hand-formatted for clarity due to all the DAG nodes), I'd still be inclined to use the original unindented formatting.

[jrtc27]

llvm/include/llvm/IR/IntrinsicsRISCV.td

73

This (and other lines like it) is slightly misleading as, given the llvm_anyptr_ty, it's an overloaded intrinsic, whose canonical name includes the specialisation (eg llvm.foo.i8p0 for an i8 *, or llvm.foo.i32p200 for an i32 addrspace(200) *). But perhaps that's ok, especially since the textual IR format allows you to drop the suffix and will infer it from the arguments.

73

p0i8 and p200i32...

[lenary]

Address review comments

[lenary]

TableGen is an officially supported language for clang-format, which is why I hoped it wouldn't be so bad to use to format this file. I really wanted a nice way to avoid having to manually wrap the comments.

Hopefully I have addressed the major issues in the comments with my recent change, especially around the canonical/overloaded intrinsic names.

llvm/include/llvm/IR/IntrinsicsRISCV.td

73

I've updated all the comments, and moved the let so the indentation changes slightly to be a bit more reasonable.

[lenary]

@jrtc27 I'm going to wait for the ok from you before I merge this, given you had feedback before.

[jrtc27]

I think this is good now (and definitely better now the big comment has moved outside of the let).

llvm/include/llvm/IR/IntrinsicsRISCV.td

 //===- IntrinsicsRISCV.td - Defines RISCV intrinsics -------*- tablegen -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file defines all of the RISCV-specific intrinsics.
 //
 //===----------------------------------------------------------------------===//
 
-let TargetPrefix = "riscv" in {
-
 //===----------------------------------------------------------------------===//
 // Atomics
 
-class MaskedAtomicRMW32Intrinsic
-    : Intrinsic<[llvm_i32_ty],
-                [llvm_anyptr_ty, llvm_i32_ty, llvm_i32_ty, llvm_i32_ty],
-                [IntrArgMemOnly, NoCapture<0>, ImmArg<3>]>;
-
-class MaskedAtomicRMW32WithSextIntrinsic
-    : Intrinsic<[llvm_i32_ty],
-                [llvm_anyptr_ty, llvm_i32_ty, llvm_i32_ty, llvm_i32_ty,
-                 llvm_i32_ty],
-                [IntrArgMemOnly, NoCapture<0>, ImmArg<4>]>;
+// Atomic Intrinsics have multiple versions for different access widths, which
+// all follow one of the following signatures (depending on how many arguments
+// they require). We carefully instantiate only specific versions of these for
+// specific integer widths, rather than using `llvm_anyint_ty`.
+//
+// In fact, as these intrinsics take `llvm_anyptr_ty`, the given names are the
+// canonical names, and the intrinsics used in the code will have a name
+// suffixed with the pointer type they are specialised for (denoted `<p>` in the
+// names below), in order to avoid type conflicts.
 
-def int_riscv_masked_atomicrmw_xchg_i32 : MaskedAtomicRMW32Intrinsic;
-def int_riscv_masked_atomicrmw_add_i32  : MaskedAtomicRMW32Intrinsic;
-def int_riscv_masked_atomicrmw_sub_i32  : MaskedAtomicRMW32Intrinsic;
-def int_riscv_masked_atomicrmw_nand_i32 : MaskedAtomicRMW32Intrinsic;
-def int_riscv_masked_atomicrmw_max_i32  : MaskedAtomicRMW32WithSextIntrinsic;
-def int_riscv_masked_atomicrmw_min_i32  : MaskedAtomicRMW32WithSextIntrinsic;
-def int_riscv_masked_atomicrmw_umax_i32 : MaskedAtomicRMW32Intrinsic;
-def int_riscv_masked_atomicrmw_umin_i32 : MaskedAtomicRMW32Intrinsic;
-
-def int_riscv_masked_cmpxchg_i32
-    : Intrinsic<[llvm_i32_ty], [llvm_anyptr_ty, llvm_i32_ty, llvm_i32_ty,
-                                llvm_i32_ty, llvm_i32_ty],
-                [IntrArgMemOnly, NoCapture<0>, ImmArg<4>]>;
+let TargetPrefix = "riscv" in {
 
-class MaskedAtomicRMW64Intrinsic
-    : Intrinsic<[llvm_i64_ty],
-                [llvm_anyptr_ty, llvm_i64_ty, llvm_i64_ty, llvm_i64_ty],
+  // T @llvm.<name>.T.<p>(any*, T, T, T imm);
+  class MaskedAtomicRMWFourArg<LLVMType itype>
+      : Intrinsic<[itype], [llvm_anyptr_ty, itype, itype, itype],
                   [IntrArgMemOnly, NoCapture<0>, ImmArg<3>]>;
-
-class MaskedAtomicRMW64WithSextIntrinsic
-    : Intrinsic<[llvm_i64_ty],
-                [llvm_anyptr_ty, llvm_i64_ty, llvm_i64_ty, llvm_i64_ty,
-                 llvm_i64_ty],
+  // T @llvm.<name>.T.<p>(any*, T, T, T, T imm);
+  class MaskedAtomicRMWFiveArg<LLVMType itype>
+      : Intrinsic<[itype], [llvm_anyptr_ty, itype, itype, itype, itype],
                   [IntrArgMemOnly, NoCapture<0>, ImmArg<4>]>;
 
-def int_riscv_masked_atomicrmw_xchg_i64 : MaskedAtomicRMW64Intrinsic;
-def int_riscv_masked_atomicrmw_add_i64  : MaskedAtomicRMW64Intrinsic;
-def int_riscv_masked_atomicrmw_sub_i64  : MaskedAtomicRMW64Intrinsic;
-def int_riscv_masked_atomicrmw_nand_i64 : MaskedAtomicRMW64Intrinsic;
-def int_riscv_masked_atomicrmw_max_i64  : MaskedAtomicRMW64WithSextIntrinsic;
-def int_riscv_masked_atomicrmw_min_i64  : MaskedAtomicRMW64WithSextIntrinsic;
-def int_riscv_masked_atomicrmw_umax_i64 : MaskedAtomicRMW64Intrinsic;
-def int_riscv_masked_atomicrmw_umin_i64 : MaskedAtomicRMW64Intrinsic;
-
-def int_riscv_masked_cmpxchg_i64
-    : Intrinsic<[llvm_i64_ty], [llvm_anyptr_ty, llvm_i64_ty, llvm_i64_ty,
-                                llvm_i64_ty, llvm_i64_ty],
-                [IntrArgMemOnly, NoCapture<0>, ImmArg<4>]>;
+  // We define 32-bit and 64-bit variants of the above, where T stands for i32
+  // or i64 respectively:
+  multiclass MaskedAtomicRMWFourArgIntrinsics {
+    // i32 @llvm.<name>.i32.<p>(any*, i32, i32, i32 imm);
+    def _i32 : MaskedAtomicRMWFourArg<llvm_i32_ty>;
+    // i64 @llvm.<name>.i32.<p>(any*, i64, i64, i64 imm);
+    def _i64 : MaskedAtomicRMWFourArg<llvm_i64_ty>;
+  }
+
+  multiclass MaskedAtomicRMWFiveArgIntrinsics {
+    // i32 @llvm.<name>.i32.<p>(any*, i32, i32, i32, i32 imm);
+    def _i32 : MaskedAtomicRMWFiveArg<llvm_i32_ty>;
+    // i64 @llvm.<name>.i64.<p>(any*, i64, i64, i64, i64 imm);
+    def _i64 : MaskedAtomicRMWFiveArg<llvm_i64_ty>;
+  }
+
+  // @llvm.riscv.masked.atomicrmw.*.{i32,i64}.<p>(...)
+  defm int_riscv_masked_atomicrmw_xchg : MaskedAtomicRMWFourArgIntrinsics;
+  defm int_riscv_masked_atomicrmw_add : MaskedAtomicRMWFourArgIntrinsics;
+  defm int_riscv_masked_atomicrmw_sub : MaskedAtomicRMWFourArgIntrinsics;
+  defm int_riscv_masked_atomicrmw_nand : MaskedAtomicRMWFourArgIntrinsics;
+  // Signed min and max need an extra operand to do sign extension with.
+  defm int_riscv_masked_atomicrmw_max : MaskedAtomicRMWFiveArgIntrinsics;
+  defm int_riscv_masked_atomicrmw_min : MaskedAtomicRMWFiveArgIntrinsics;
+  // Unsigned min and max don't need the extra operand.
+  defm int_riscv_masked_atomicrmw_umax : MaskedAtomicRMWFourArgIntrinsics;
+  defm int_riscv_masked_atomicrmw_umin : MaskedAtomicRMWFourArgIntrinsics;
+
+  // @llvm.riscv.masked.cmpxchg.{i32,i64}.<p>(...)
+  defm int_riscv_masked_cmpxchg : MaskedAtomicRMWFiveArgIntrinsics;
 
 } // TargetPrefix = "riscv"