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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 42845 Build 43427: arc lint + arc unit
Event Timeline
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.
llvm/include/llvm/IR/IntrinsicsRISCV.td | ||
---|---|---|
23 | 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. | |
23 | p0i8 and p200i32... |
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 | ||
---|---|---|
23 | I've updated all the comments, and moved the let so the indentation changes slightly to be a bit more reasonable. |
@jrtc27 I'm going to wait for the ok from you before I merge this, given you had feedback before.
I think this is good now (and definitely better now the big comment has moved outside of the let).
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.