This is an archive of the discontinued LLVM Phabricator instance.

[NFC][Clang][RISCV] Reduce boilerplate when determining prototype for segment loads
ClosedPublic

Authored by eopXD on Dec 25 2022, 9:30 AM.

Details

Summary

No functionality change of the RVV builtin and compiler intrinsics is intended
in this patch.

This patch gathers scattered comments for the segment load builtin/intrinsics
and its variants (e.g. segment unit-stride load, segment strided load) into a
single paragraph under riscv_vector.td.

This patch also tries to reduce one level of the if-statements as the push_back
are essentially the same actions but differs in index based on the the value
of the policy attributes and whether the intrinsic is masked.

Diff Detail

Event Timeline

eopXD created this revision.Dec 25 2022, 9:30 AM
eopXD requested review of this revision.Dec 25 2022, 9:30 AM
eopXD updated this revision to Diff 485270.Dec 25 2022, 11:01 PM

Rebase to latest main.

khchen added inline comments.Dec 26 2022, 6:42 AM
clang/include/clang/Basic/riscv_vector.td
895

After remove the builtins comment I don't have idea what's going on this piece of code. but I'm okay if there is no concern from the other reviewers.

897–909

nit: nit: there is no assert now, is it still NFC? I'm not sure.

craig.topper added inline comments.Dec 26 2022, 4:55 PM
clang/include/clang/Basic/riscv_vector.td
910

This doesn't look right to me. This append call appends PassThruOperand NF times. In the non-agnostic case, it needs to append NF different values.

eopXD edited the summary of this revision. (Show Details)Dec 28 2022, 11:27 PM
eopXD updated this revision to Diff 485589.Dec 29 2022, 1:14 AM

Update code based on landing of D140678.

The motivation of this NFC patch comes from the struggle while I was tracing through them. I think the scattered comments of boilerplates with only something slightly different makes the code hard to read, that is why I am gathering the comments together into a sole paragraph. The multi-level if-statement has duplicated actions and gets me confused from time-to-time.

eopXD edited the summary of this revision. (Show Details)Dec 29 2022, 1:20 AM

LGTM. Thanks for clean up code!

clang/include/clang/Basic/riscv_vector.td
820

nit: there is no unmasked builtin comment here.

1035

In fact, VLOperand is always coming from Ops.back()

eopXD updated this revision to Diff 485604.Dec 29 2022, 4:08 AM
eopXD marked 2 inline comments as done.
eopXD edited the summary of this revision. (Show Details)

Update comment section for segment load.

eopXD marked 2 inline comments as done.Dec 29 2022, 4:08 AM
eopXD updated this revision to Diff 485606.Dec 29 2022, 4:16 AM

Update comment section: remove mis-indented indents.

kito-cheng accepted this revision.Dec 29 2022, 10:40 PM

Go ahead, thanks for clean up!

This revision is now accepted and ready to land.Dec 29 2022, 10:40 PM
This revision was landed with ongoing or failed builds.Jan 2 2023, 6:50 PM
This revision was automatically updated to reflect the committed changes.