Page MenuHomePhabricator

[RISCV][NFC] Deduplicate Atomic Intrinsic Definitions
ClosedPublic

Authored by lenary on Dec 20 2019, 10:52 AM.

Details

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.

Diff Detail

Event Timeline

lenary created this revision.Dec 20 2019, 10:52 AM
Jim added a subscriber: Jim.Dec 22 2019, 9:06 PM
This revision is now accepted and ready to land.Dec 23 2019, 11:56 AM

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

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 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 added inline comments.Dec 23 2019, 2:36 PM
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 updated this revision to Diff 236620.Jan 7 2020, 9:50 AM

Address review comments

lenary marked an inline comment as done.Jan 7 2020, 9:53 AM

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.

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

jrtc27 accepted this revision.Jan 14 2020, 3:07 AM

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

This revision was automatically updated to reflect the committed changes.